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

[sangheon]

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.

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