RFR(s): 8023905: Failing to initialize VM with small initial heap when NUMA and large pages are enabled

[sangheon]

Hi Stefan,

On 08/30/2016 02:33 AM, Stefan Johansson wrote:
> Hi Sangheon,
>
> On 2016-08-30 02:08, sangheon wrote:
>> Hi Jon,
>>
>> Thanks for reviewing this.
>>
>> On 08/29/2016 02:55 PM, Jon Masamitsu wrote:
>>>
>>>
>>> On 08/29/2016 01:31 AM, Stefan Johansson wrote:
>>>> ...
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Another thing is the use of #ifdef to make this conditional 
>>>>>>>>>> for Linux. Is this needed? Isn't the return value for 
>>>>>>>>>> can_commit_large_page_memory() the conditional we should care 
>>>>>>>>>> about? 
>>>>>>>>> We can know whether we are using 'pin region' from the return 
>>>>>>>>> value of can_commit_large_page_memory() and UseLargePages flag.
>>>>>>>>>
>>>>>>>>> However the condition of comparison with page size in Linux 
>>>>>>>>> version of os::pd_free_memory() makes this problem. For other 
>>>>>>>>> platforms such as Windows, AIX and BSD which have empty 
>>>>>>>>> os::pd_free_memory(), it doesn't matter. And Solaris doesn't 
>>>>>>>>> have such condition. This means for other platforms we don't 
>>>>>>>>> need to exit because of reverting to default page size.
>>>>>>>>>
>>>>>>>>> I mean if can_commit_large_page_memory() returns false and 
>>>>>>>>> UseLargePages enabled, we will try to use pin region. But NUMA 
>>>>>>>>> could try to use default page size. It is okay in general 
>>>>>>>>> except on Linux because of above reason.
>>>>>>>>>
>>>>>>>>>> Or will we fail some platform too early. If so, we could add 
>>>>>>>>>> another capability method to the os class and use that to 
>>>>>>>>>> avoid having the #ifdef in the code.
>>>>>>>>> I also considered shortly to add a new method to decide.
>>>>>>>>> And I agree that not using #ifdef is better in general. But 
>>>>>>>>> I'm not sure for this case as it is too Linux implementation 
>>>>>>>>> specific. i.e. Linux version is implemented pd_free_memory() 
>>>>>>>>> to conditionally commit after comparing with page size. If 
>>>>>>>>> Linux pd_free_memory() becomes blank or the condition is 
>>>>>>>>> changed, the decision method also should be changed which 
>>>>>>>>> seems not worth for me. This is why I stopped considering it.
>>>>>>>>>
>>>>>>>> Ok, I don't see things changing as a big problem, you could let 
>>>>>>>> the new capability just return the same as 
>>>>>>>> can_commit_large_page_memory() for Linux and have the other 
>>>>>>>> platforms return true. This would have the same maintenance 
>>>>>>>> requirements as the current solution in my eyes.
>>>>>>> I think we have to consider not only the maintenance but also 
>>>>>>> the necessity of the method. I don't think we could re-use it 
>>>>>>> and this is why I described it as too Linux implementation 
>>>>>>> specific.
>>>>>>>
>>>>>>> If you strongly want to introduce a new method, may I ask what 
>>>>>>> is expected name/roll of the method?
>>>>>>>
>>>>>> I see your point and I'll leave it up to a second reviewer to 
>>>>>> decide which way to go.
>>>>> OK.
>>>
>>> Stefan and Sangheon,
>>>
>>> I agree that having the #ifdef is unfortunate but I'm really 
>>> hesitant about
>>> adding an new API at the os:: level.   We could give it a good name 
>>> but it
>>> seems like it would just be there for GC.
>>>
>>> A little better I think would be to add a field to MuntableNUMASpace
>>>
>>> bool _must_use_large_pages;
>>>
>>> and add the #ifdef  in MutableNUMASpace::MutableNUMASpace() : ..., 
>>> _must_use_large_pages(false) ...
>>>
>>> 582 #ifdef LINUX
>>> 583 // Changing the page size below can lead to freeing of memory. 
>>> When using large pages
>>> 584 // and the memory has been both reserved and committed, Linux 
>>> does not support
>>> 585 // freeing parts of it. So we fail initialization.
>>> 586 if (UseLargePages && !os::can_commit_large_page_memory()) {
>>> 587 _must_use_large_pages = true;
>>> 588 }
>>> 589 #endif // LINUX
>>> There is not much code in the constructor and adding the #ifdef is less
>>> distracting.
>>>
>>> Then the code in initialize() could be
>>>   580   if (base_space_size_pages / lgrp_spaces()->length() == 0
>>>   581       && page_size() > (size_t)os::vm_page_size()) {
>>>
>>> if (_must_use_large_pages) {
>>> 587 vm_exit_during_initialization("Failed initializing NUMA with 
>>> large pages. Too small heap size"); }
>> I think this would give better readability on 'initialize' method.
>>
>> Webrev:
>> http://cr.openjdk.java.net/~sangheki/8023905/webrev.2
>> http://cr.openjdk.java.net/~sangheki/8023905/webrev.2_to_1
>>
>> Stefan, what do you think?
>> webrev.2 seems better for me.
>>
> This looks better to me as well, but the indentation in the 
> constructor seems to be a bit off. Please fix this before pushing.
Okay, I will fix before pushing it.

Thanks,
Sangheon


>
> Thanks,
> Stefan
>
>> Thanks,
>> Sangheon
>>
>>
>>>
>>> If you don't care for that suggestion, the patch as is looks fine.
>>>
>>> Jon
>>>
>>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20160830/608bf27e/attachment.htm>