RFR(s): 8145000: TestOptionsWithRanges.java failure for XX:+UseNUMA -XX:+UseNUMAInterleaving -XX:NUMAInterleaveGranularity=65536

[Jesper Wilhelmsson]

Den 22/12/15 kl. 02:36, skrev sangheon:
> Hi Jesper,
>
> On 12/21/2015 04:38 PM, Jesper Wilhelmsson wrote:
>> Hi Sangheon,
>>
>> Just a comment about the assert in protect_pages_individually(). To me it
>> looks strange to have an assert and then on the next line handle the case that
>> was just asserted that it can't happen. Either we assume it can happen and
>> handle it, or we say that it can't happen and assert that it doesn't.
> I agree, assert is wrong.
> And looking at current usage of os::protect_memory(), caller is handling the
> situation.
> So I think 'warning' seems correct as the log is intended to show which chunk is
> making problem.
>
> -  assert(ret, "Failed protecting chunk #%u", count);
> +  warning("Failed protecting chunk #%u", count);
>
> What do you think?

Yes, a warning seems appropriate.
Ship it!
/Jesper

>
> Thanks,
> Sangheon
>
>
>>
>> It would be nice if you either remove the assert or add a comment that
>> explains why it is there. I do not need to re-review if you decide to do either.
>>
>> Besides that, looks good!
>> /Jesper
>>
>> Den 21/12/15 kl. 22:43, skrev sangheon:
>>> Hi all,
>>>
>>> Tom and I did some discussion(previously I misunderstood Tom's comment) and
>>> here's the next webrev which includes:
>>>
>>> 1. Call protect_pages_individually() when UseNUMAInterleaving is enabled (Tom)
>>> 2. Added a comment why we need to call protect_pages_individually(). (Tom)
>>> 3. Changed VirtualQuery() handling code. (Tom)
>>>
>>> http://cr.openjdk.java.net/~sangheki/8145000/webrev.03
>>> http://cr.openjdk.java.net/~sangheki/8145000/webrev.03_to_02
>>>
>>> Thanks,
>>> Sangheon
>>>
>>>
>>> On 12/21/2015 11:19 AM, Tom Benson wrote:
>>>> Hi,
>>>>
>>>> On 12/21/2015 1:57 PM, Jesper Wilhelmsson wrote:
>>>>> Hi Sangheon,
>>>>>
>>>>> I was under the impression that Tom wanted to keep the UseNUMAInterleaving
>>>>> test and not always call protect_pages_individually().
>>>>
>>>> Yes, I'd said that was my vote, but wasn't adamant about it. If we agree,
>>>> let's keep it.  8^)
>>>> Tom
>>>>
>>>>> At least that's what I would prefer. If you revert that change I think we
>>>>> have something here :)
>>>>> /Jesper
>>>>>
>>>>>
>>>>> Den 21/12/15 kl. 18:48, skrev sangheon:
>>>>>> Hi Jesper and Tom,
>>>>>>
>>>>>> Here's next webrev which includes below:
>>>>>> - Check return value of VirtualQuery. (Tom, Jesper)
>>>>>> - Always call protect_pages_individually() from os::protect_memory(). (Tom)
>>>>>>
>>>>>> Webrev:
>>>>>> http://cr.openjdk.java.net/~sangheki/8145000/webrev.02
>>>>>> http://cr.openjdk.java.net/~sangheki/8145000/webrev.02_to_01/
>>>>>>
>>>>>> Test:
>>>>>> JPRT, RBT (hotspot/test/:hotspot_all,testlist,noncolo.testlist
>>>>>> --add-tonga-keyword quick for Windows only)
>>>>>>
>>>>>> Thanks,
>>>>>> Sangheon
>>>>>>
>>>>>>
>>>>>> On 12/21/2015 03:40 AM, Jesper Wilhelmsson wrote:
>>>>>>> Hi Sangheon,
>>>>>>>
>>>>>>> I like this version a lot better.
>>>>>>> I have no further comments except for what Tom already mentioned about
>>>>>>> checking VirtualQuery return value.
>>>>>>> /Jesper
>>>>>>>
>>>>>>>
>>>>>>> Den 19/12/15 kl. 00:39, skrev Tom Benson:
>>>>>>>> Hi Sangheon,
>>>>>>>>
>>>>>>>> On 12/18/2015 6:02 PM, sangheon wrote:
>>>>>>>>> Hi Tom,
>>>>>>>>>
>>>>>>>>> On 12/17/2015 04:23 PM, sangheon wrote:
>>>>>>>>>> Hi Tom,
>>>>>>>>>>
>>>>>>>>>> Thank you for reviewing this!
>>>>>>>>>>
>>>>>>>>>> On 12/17/2015 01:28 PM, Tom Benson wrote:
>>>>>>>>>>> Hi Sangheon,
>>>>>>>>>>> I like the new approach, but just have a couple of comments.
>>>>>>>>>> Thanks.
>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> I think you should check the VirtualQuery return status and return false
>>>>>>>>>>> from protect_pages_individually if zero.
>>>>>>>>>> Right.
>>>>>>>>>> I will fix this.
>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> I don't *think* you need to have the "!UseLargePages" restriction
>>>>>>>>>>> anymore
>>>>>>>>>>> with this approach, do you?
>>>>>>>>>> protect_pages_individually() doesn't have previous restriction on its
>>>>>>>>>> usage.
>>>>>>>>>> However I wanted to remain the caller(os::protect_memory) as is
>>>>>>>>>> because, as
>>>>>>>>>> you already mentioned below, I didn't want to have an additional call of
>>>>>>>>>> VirtualQuery() for simpler code.
>>>>>>>>>> I don't have strong opinion on this.
>>>>>>>>>>
>>>>>>>>>> Let me post next webrev after concluding this.
>>>>>>>>> Tom, do you prefer to always use protect_pages_individually()?
>>>>>>>>> Does anyone have opinion on this?
>>>>>>>>>
>>>>>>>>
>>>>>>>> I'd vote for leaving the UseNUMAInterleaving test in.
>>>>>>>> Tom
>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>> Sangheon
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Thanks,
>>>>>>>>>> Sangheon
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Actually, I think you could just always use protect_pages_individually
>>>>>>>>>>> regardless of whether UseNUMAInterleaving was enabled or not, and the
>>>>>>>>>>> right
>>>>>>>>>>> thing would happen.  But this way, you save an unnecessary system call
>>>>>>>>>>> plus
>>>>>>>>>>> some overhead.
>>>>>>>>>>> Tom
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>
>>>>>>
>>>>
>>>
>