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

[sangheon]

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?

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