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

[Jon Masamitsu]

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"); }

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/20160829/8a416434/attachment.htm>