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

[Stefan Johansson]

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.

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/7d1e4a70/attachment.htm>