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

[Jesper Wilhelmsson]

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.

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
>>>>>>>>>
>>>>>>>>>
>>>>>>
>>>>
>>
>