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

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

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