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

[Thomas Stüfe]

On Thu, May 14, 2020 at 2:44 PM Zhengyu Gu <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?

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