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

[Tom Benson]

Hi Sangheon,
I like the new approach, but just have a couple of comments.

I think you should check the VirtualQuery return status and return false 
from protect_pages_individually if zero.

I don't *think* you need to have the "!UseLargePages" restriction 
anymore with this approach, do you?

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


On 12/17/2015 12:34 AM, sangheon wrote:
> Hi Jesper,
>
> Thank you for looking at this!
>
> On 12/16/2015 06:46 AM, Jesper Wilhelmsson wrote:
>> Hi Sangheon,
>>
>> * It seems to me that protect_pages_individually() can only be called 
>> if UseLargePages is false. Still it looks at UseLargePages to decide 
>> page_size. I would prefer if there was an assert(!UseLargePages) at 
>> the start of protect_pages_individually() to emphasize that this code 
>> has not been used or tested with large pages.
>>
>> * I'm not sure if this matters, but it seems to me that if the size 
>> of the area is not an even multiple of pages, you will start 
>> protecting the smaller part, and then continue with page sized areas. 
>> E.g. If page size is 4k and we want to protect a 10k area, we will 
>> start with a 2k block and then take two 4k blocks. This has the 
>> effect that if we pass in a page aligned address, the areas we 
>> protect will not cover one page at a time. This may be a non-issue 
>> though. It depends on if we usually get page aligned addresses and if 
>> it matters if we protect one page at a time or not.
> Your second comment made me to change the function which maybe doesn't 
> need an assert now and  removed potential alignment problem.
> Currently protect_pages_individually() is only called when 
> "UseNUMAInterleaving && !UseLargePages" and 
> protect_pages_indivisually() will always have aligned size as 
> allocate_pages_individually() allocate aligned size. This means, as 
> you pointed, we need an assert to prevent wrong use and would have 
> alignment issue.
>
> So I changed the function to read currently allocated chunk size(via 
> Windows API) rather than calculating chunk size and then try to 
> protect the chunk.
> With this change, we don't need to care about the flags neither align 
> matter.
>
>>
>> * You use UINT32_FORMAT to print an uint. %u is the recommended way 
>> to print an uint.
> Fixed.
>
>>
>> * if (!ret) return false;  We should always use { }
> Fixed.
>
> Here's a next webrev.
> http://cr.openjdk.java.net/~sangheki/8145000/webrev.01/
> http://cr.openjdk.java.net/~sangheki/8145000/webrev.01_to_00/
>
> Testing: JPRT (TestOptionsWithRanges.java), RBT 
> (hotspot_all,testlist,noncolo.testlist for Windows only)
>
> * Added more tests.
>
> Thanks,
> Sangheon
>
>
>>
>> Thanks,
>> /Jesper
>>
>>
>>
>> Den 16/12/15 kl. 01:53, skrev sangheon:
>>> Hi all,
>>>
>>> Could I get a couple of reviews for Windows 
>>> NUMAInterleaveGranularity change?
>>>
>>> Current Windows implementation can't handle correctly when we 
>>> reserve a heap
>>> with disjoint heap base mode with NUMAInterleaveGranularity.
>>>
>>> Windows, os::protect_memory fails in above case, as we are trying to 
>>> protect the
>>> whole reserved heap at one time. But we reserved that area with
>>> separate/contiguous chunks based on NUMAInterleaveGranularity size. 
>>> MSDN
>>> describes to call the API separately.
>>> I changed protect API to be called multiple times if needed.
>>>
>>> Skipped adding a test as TestOptionsWithRanges.java is enough.
>>> 'java -XX:+UseNUMA -XX:+UseNUMAInterleaving' is enough to reproduce 
>>> on large
>>> memory Windows machine. However we need to specify heap size if the 
>>> machine
>>> doesn't have large memory. e.g. -Xms30g -Xmx30g
>>>
>>> This patch is based on the patch for JDK-8144949 which includes the 
>>> change of
>>> the max range of NUMAInterleaveGranularity to 2G(32bit), 8192G(64bit).
>>>
>>> CR: https://bugs.openjdk.java.net/browse/JDK-8145000
>>> Webrev: http://cr.openjdk.java.net/~sangheki/8145000/webrev.00
>>> Testing: JPRT (with TestOptionsWithRanges.java enabled), manual 
>>> tests on Windows
>>> machine(to test on large memory).
>>>
>>> Thanks,
>>> Sangheon
>>>
>>>
>