RFR: 8243315: ParallelScavengeHeap::initialize() passes GenAlignment as page size to os::trace_page_sizes instead of actual page size

[Stefan Johansson]

On Wed, 25 Nov 2020 12:12:19 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:

>> Hi guys,
>> 
>> I was just reviewing this but see that it was already pushed in the meantime.
>> 
>> I have some concerns about this change.
>> 
>> Most importantly, it perpetuates the notion that alignment has anything to do with pages, which is just wrong. I know we have still some coding in the hotspot which assumes this but it is not true. We should remove any coding assuming that, instead of building onto it.
>> 
>> ReservedSpace::alignment is just that, the alignment of the start address of the ReservedSpace. Arguably, there is no need to even keep it as a member in ReservedSpace, it is needed for reservation and should not be needed beyond that.
>> 
>> So, `page_size = MIN2(rs.alignment(), os::large_page_size());` is wrong, e.g. if I create a space with 128M aligned base, that does say nothing about the underlying page size. Unfortunately we do not have tests to test this. This coding had been less of a problem as long as it lived in gc land and was only called for heap areas. Now it is a general purpose function.
>> 
>> (why is this function static btw?)
>> 
>> As an example for larger alignment which have nothing to do with page size: Metaspace reserves 4M-aligned ReservedSpaces. Currently only uses small pages, but that may change.
>> 
>> Furthermore I think the notion of asking about the page size of a range is itself shaky. A range can and does consist of multiple page sizes (see e.g. what Linux reserves with UseHugeTLBFS).
>> 
>> The patch also assumes os::large_page_size() to be the one large page size used, and currently there are attempts by Intel to more or less fall back to a "small large page" if the large page is too large for a given area: see https://github.com/openjdk/jdk/pull/1153. This would increase the number of used page sizes, and could mean space reserved with reserve_memory_special() would silently return memory with small-large-pages instead of os::large_page_size().
>> 
>> I am sorry to make such a fuss.
>> 
>> -----
>> 
>> In an ideal world we would have something like
>> 
>> pagesizes_t os::query_page_sizes(address range);
>> 
>> which would return information about all page sizes in the range (page_sizes_t could be a bitmask btw since all page sizes are pow2). We have something like this on AIX, see `os::Aix::query_pagesize`. On Linux, we have e.g. int getpagesize(void); 
>> 
>> That'd be awesome since it would take the burden from us to keep information of page sizes or second-guessing reservation logic. That information could even be cached in a ReservedSpace if its difficult to get.
>> 
>> Cheers, Thomas
>
> I wrote
> 
>>  On Linux, we have e.g. int getpagesize(void);
>> 
> 
> which is of course wrong since it just returns the global small page size. Querying the page size of an arbitrary range seems to require walking smaps, and that is way more cumbersome.

Hi Thomas, 

Thanks for sharing your concerns. I agree that this becomes more of a problem now when it is exposed outside the GC, that's why I wasn't sure where we should put this:
> Not entirely sure where the best location for such helper is, but a static function ReservedSpace::actual_page_size(ReservedSpace) could work.

This is also why it was added as a static helper rather than a member. An alternative is to move this helper to a shared GC utility, and maybe document it a bit more. Adding multiple large page sizes will require this function to be updated, that's one of the reasons I wanted it shared not duplicated. 

As you say the best thing would be if we had a way to really query the page size rather than doing a good estimation given the looks of the reserved space. But for the current use-cases in the GC, this estimation seems to be enough. Going the other route is, as you say, way more work.

A different solution would be to add a `_largest_page_size` member to the reserved space, which would be the larges page size used by that reservation. That might make sense if we are moving towards having multiple large page sizes.

What would be your preferred way forward?

Cheers,
Stefan

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

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