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 11:42:45 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:

>> 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

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.

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

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