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

[sangheon]

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.

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