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

[Tom Benson]

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