RFR: 8256155: os::Linux Populate all large_page_sizes, select smallest page size in reserve_memory_special_huge_tlbfs* [v27]

Thomas Stuefe stuefe at openjdk.java.net
Wed Apr 21 15:31:41 UTC 2021


On Wed, 21 Apr 2021 12:46:21 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:

>>> Hi @kstefanj,
>>> 
>>> (@mgkwill : there are multiple Stefans, it can get confusing)
>> 
>> Thanks for including me @tstuefe and noting the highlighting of the wrong Stefan. Whoops :P . Updated.
>>> 
>>> CSR looks good overall. Thanks for doing this. I comment here to give Marcus a way to participate (not sure if he has JBS access). Some points:
>>> 
>> 
>> I don't have access yet. Still working towards my authorship. I have a few patches in but not sure of the threshold/process. 
>> 
>>> * does it have to have the same title as the issue it is associated with? A title like "Change definition of LargePageSizeInBytes" would be easier to understand and search for.
>> 
>> +1 
>> 
>>> * Can the CSR please state that it only affects Linux?
>>> 
>> 
>> +1
>> 
>>> Side note, I had a look at the Windows implementation. It processes LargePageSizeInBytes, but I am not sure it has any effect. On Windows, one obtains the "minimum large page size" and uses that value to align the size requested from VirtualAlloc. Thats it, there is no way to specify an explicit large page size when reserving. LargePageSizeInBytes overwrites this "minimum large page size", but all this does is it somewhat affects the allocated size. So it may be that we could just remove this switch from Windows altogether.
>>> 
>> 
>> Can this be handled in the same CSR?
>> 
>>> * "The Java man pages already defines " -> "The Java man page already defines "
>> 
>> +1
>> 
>>> * Do we allow explicit -XX:LargePageSizeInBytes=0 ? Seems somewhat pointless. Do we handle it today?
>> 
>> Default is `LargePageSizeInBytes=0` - so it seems we would treat `-XX:LargePageSizeInBytes=0` as the same as default. We don't have a specific case for 0 except default handling as far as I can see.
>> 
>> `  product(size_t, LargePageSizeInBytes, 0,                                  \
>>           "Large page size (0 to let VM choose the page size)")             \
>>           range(0, max_uintx)                                               \
>>                                                                             `
>> 
>>> 
>>> Cheers, Thomas
>
>> Hi @mgkwill and @tstuefe!
>> 
>> > A modification of the code is needed to accomplish what @tstuefe suggested:
>> > > Default (`LargePageSizeInBytes=0`)
>> > > 
>> > > * old behavior - use the systems default large page size
>> > > * new behavior - use all configured page sizes up to and including the systems default large page size
>> > 
>> > 
>> > I'll update with a suggested route in the next couple of days.
>> 
>> Nice, an easy approach should be to just remove all large page sizes larger than the decided maximum from _page_sizes.
>> 
>> > CSR looks good overall. Thanks for doing this. I comment here to give Marcus a way to participate (not sure if he has JBS access). Some points:
>> > 
>> > * does it have to have the same title as the issue it is associated with? A title like "Change definition of LargePageSizeInBytes" would be easier to understand and search for.
>> 
>> I found some other CSRs that had different titels compared to the issues, so change it. Maybe we should also change the name of this issue as well. To something like:
>> Allow multiple large page sizes to be used on Linux
>> 
>> > * Can the CSR please state that it only affects Linux?
>> 
>> Not sure we should say only affects Linux, but maybe that only Linux implements support for the new definition (allowing multiple page sizes to be used). But the definition for the flag is the same for all OSes. On Windows we won't make use of the somewhat relaxed definition and it will continue to mean maximum large page size (and only). Is this ok with you?
>> 
>> > Side note, I had a look at the Windows implementation. It processes LargePageSizeInBytes, but I am not sure it has any effect. On Windows, one obtains the "minimum large page size" and uses that value to align the size requested from VirtualAlloc. Thats it, there is no way to specify an explicit large page size when reserving. LargePageSizeInBytes overwrites this "minimum large page size", but all this does is it somewhat affects the allocated size. So it may be that we could just remove this switch from Windows altogether.
>> 
>> Interesting, well it doesn't really go against what we say in the CSR right?
> 
> I am fine with leaving Windows implicitly covered.
> 
>> 
>> > * "The Java man pages already defines " -> "The Java man page already defines "
>> 
>> Fixed.
>> 
>> > * Do we allow explicit -XX:LargePageSizeInBytes=0 ? Seems somewhat pointless. Do we handle it today?
>> 
>> Actually we don't, but we probably should handle it. Would be kind of strange preventing someone from setting the default.
> 
> I marked the CSR as reviewed. 
> 
> Thanks & Cheers, Thomas

> Thanks for reviewing the the CSR @tstuefe. Internal discussions around the text in the CSR pointed out the "system" might not be the best wording when it comes to default size. I will update the CSR using "for the environment" instead, for example:
> 
> > “By default, the size is set to 0, meaning that the JVM will use the default large page size for the environment as the maximum size for large pages.”
> 
> Hope this is still good.

Still good. I was vaguely concerned about the "default" in "default system page size" because that is a Linux concept; on other Unices which have multiple page sizes there is no concept of a default (eg AIX, HPUX). But this is just idle nitpicking, the CSR is clear the way you wrote it.

-------------

PR: https://git.openjdk.java.net/jdk/pull/1153



More information about the hotspot-gc-dev mailing list