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

[Marcus G K Williams]

On Tue, 20 Apr 2021 14:07:48 GMT, Stefan Johansson <sjohanss at openjdk.org> wrote:

>> Marcus G K Williams has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 44 commits:
>> 
>>  - Merge branch 'master' into update_hlp
>>  - Rebase on pull/3073
>>    
>>    Signed-off-by: Marcus G K Williams <marcus.williams at intel.com>
>>  - Merge branch 'pull/3073' into update_hlp
>>  - Thomas review.
>>    
>>    Changed commit_memory_special to return bool to signal if the request succeeded or not.
>>  - Self review.
>>    
>>    Update helper name to better match commit_memory_special().
>>  - Marcus review.
>>    
>>    Updated comments.
>>  - Ivan review
>>    
>>    Renamed helper to commit_memory_special and updated the comments.
>>  - 8262291: Refactor reserve_memory_special_huge_tlbfs
>>  - Merge branch 'master' into update_hlp
>>  - Addressed kstefanj review suggestions
>>    
>>    Signed-off-by: Marcus G K Williams <marcus.williams at intel.com>
>>  - ... and 34 more: https://git.openjdk.java.net/jdk/compare/f60e81bf...f5bfe224
>
> Just created a CSR for this: [JDK-8265517](https://bugs.openjdk.java.net/browse/JDK-8265517)
> Please take a look at it and provide feedback if you have any. 
> 
> @mgkwill, please update the patch with the text from the CSR for `LargePageSizeInBytes`.

> 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

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

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