RFR(S) 8241004: NMT tests fail on unalinged thread size with debug build

[Boris Ulasevich]

Hi Thomas,

On 14.05.2020 16:51, Thomas Stüfe wrote:
>
> On Thu, May 14, 2020 at 2:44 PM Zhengyu Gu <zgu at redhat.com 
> <mailto:zgu at redhat.com>> wrote:
>
>     Hi Boris and Thomas,
>
>     Thanks for fixing it.
>
>     On 5/13/20 2:23 PM, Thomas Stüfe wrote:
>     > Hi Boris,
>     >
>     > I think this looks good. Zhengyu should look at this to make sure.
>     >
>     > When looking at os::committed_in_range() I remembered that this
>     was a
>     > rather specific function. Its name suggests that it counts all
>     committed
>     > pages in a range; but it only counts the first committed
>     contiguous range,
>     > then stops.
>
>     This function was introduced mainly for stack tracking, and it is the
>     only use at this moment.
>
>
> I wondered about that. Since stacks usually grow downward, a function 
> which checks occupancy starting at the first address upward, how does 
> that work? Should we not start at the stack base and go down from 
> there? Otherwise if a page happens to be committed at the end of the 
> stack, with holes inbetween, would we not measure the wrong thing?

As I can see, SnapshotThreadStackWalker::do_allocation_site iterates 
over given ReservedMemoryRegion committed ranges and skips holes between 
them:
http://hg.openjdk.java.net/jdk/jdk/file/a44396903fd8/src/hotspot/share/services/virtualMemoryTracker.cpp#l531

> I may be confused, sorry.
>
>     But I have ideas for other cases, e.g. having NMT no tracking
>     committed
>     regions, but scanning reserved regions for committed regions. This
>     not
>     only eliminates the most of complexity in virtual memory tracking,
>     but
>     also yields more accurate information (because os::commit() does not
>     actually commit memory).
>
>
> In my experience you need both, since these are different information. 
> It helps to know how much space was committed independent from the 
> mincore numbers. E.g. when under memory pressure you start swapping. 
> mincore could be really misleading then, or?
>
> That said, omitting committed sizes certainly would help reducing NMT 
> complexity.
>
>
>     So, I wonder if it is better solution to leave os::committed_in_range
>     alone, but make adjustment in SnapshotThreadStackWalker[1], if it is
>     thread stack only problem?
>
>
> I would prefer having two os:: functions like this:
>
> 1)
> os::memory_incore_contiguously_up(address, size_t maxsize);
> os::memory_incore_contiguously_down(address, size_t maxsize);
> Both would count how much space is in-core starting at address up to 
> maxsize; resp. where the first hole starts. the first variant counts 
> up, the second down. But we only really need down for stacks, so we 
> could omit the _up() version at first.
>
> 2)
> os::memory_incore_in_range(arbitrary range)
> I would prefer this thing to count the whole range, holes or not. You 
> could use this then for counting min-core numbers in NMT.
>
> This would give us two very useful functions, useful outside NMT too.
> The naming seems clunky though, I admit :)
>
> Cheers, Thomas
>
>     -Zhengyu
>
>
>
>     [1]
>     http://hg.openjdk.java.net/jdk/jdk/file/a44396903fd8/src/hotspot/share/services/virtualMemoryTracker.cpp#l531
>
>
>
>
>     >
>     > Thanks, Thomas
>     >
>     >
>     >
>     > On Wed, May 13, 2020 at 6:29 PM Boris Ulasevich
>     <boris.ulasevich at bell-sw.com <mailto:boris.ulasevich at bell-sw.com>>
>     > wrote:
>     >
>     >> Hi Thomas,
>     >>
>     >> Good point! Thanks.
>     >> I reworked the function to ease both restrictions.
>     >> http://cr.openjdk.java.net/~bulasevich/8241004/webrev.01
>     >>
>     >> regards,
>     >> Boris
>     >>
>     >> On 13.05.2020 08:59, Thomas Stüfe wrote:
>     >>
>     >> Hi Boris,
>     >>
>     >> Note that we have unaligned stack boundaries on AIX too, not so
>     strange :)
>     >>
>     >> What I do not like about the solution is that it loosens the
>     restriction
>     >> on the input size while keeping the restriction on the start
>     address. I'd
>     >> rather see this function either in its current form (requiring
>     the input
>     >> ranges to be page aligned) or the input range to be completely
>     >> unrestricted. This function is currently only used by NMT but
>     it is useful
>     >> and may be used in other places too.
>     >>
>     >> Can you make it work with arbitrary input ranges? I do not
>     think this
>     >> would require much changes from your patch, you do most of the
>     work already.
>     >>
>     >> Thanks!
>     >>
>     >> ..Thomas
>     >>
>     >>
>     >>
>     >> On Thu, May 7, 2020 at 2:56 PM boris
>     <boris.ulasevich at bell-sw.com <mailto:boris.ulasevich at bell-sw.com>>
>     wrote:
>     >>
>     >>> Hi,
>     >>>
>     >>> Please review the following change.
>     >>>
>     >>> http://cr.openjdk.java.net/~bulasevich/8241004/webrev.00
>     >>> http://bugs.openjdk.java.net/browse/JDK-8241004
>     >>>
>     >>> The stack size created by the implementation of the pthread
>     library is
>     >>> not necessarily page aligned. It is weird, but it is not
>     forbidden: the
>     >>> pthread library may add some gap on it own. My change removes
>     stack size
>     >>> alignment assert checks and tunes NMT committed range
>     calculation for
>     >>> the case of unaligned stack size.
>     >>>
>     >>> thanks,
>     >>> Boris
>     >>>
>     >>
>     >>
>     >
>