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

[Thomas Stuefe]

On Wed, 25 Nov 2020 09:34:56 GMT, Joakim Nordström <github.com+779991+jaokim at openjdk.org> wrote:

>> @jaokim I've run some additional testing and haven't seen any problems so I'm ready to sponsor whenever you want to integrate.
>
> Thank you @kstefanj and @tschatzl for reviewing and comments!
> 
> Tested tier1, tier2, tier3 without problems.

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

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

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