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

[Zhengyu Gu]

Hi,

Sorry for replying late.
> 
> 
> 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?

My memory is a bit rusty. I think it assumes no holes in stack, after 
adjusting guard pages.

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

Okay, I have no idea what mincore reports if memory is swapped out. 
smaps seems to report it in "Swap", not "RSS".

It really depends on what user wants to see. The most cases I dealt 
with, users were comparing NMT outputs agains RSS sizes and smaps info.

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

Yes, I would like to have them too :-)

Thanks,

-Zhengyu

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