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

[sangheon]

Hi Jesper and Tom,

Thank you for reviewing this.

Sangheon


On 12/22/2015 07:33 AM, Tom Benson wrote:
> Looks OK to me as well.
> Tom
>
> On 12/22/2015 8:14 AM, Jesper Wilhelmsson wrote:
>> 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
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>
>>>>>>
>>>>>
>>>
>