RFR: 8304824: NMT should not use ThreadCritical [v3]

[Robert Toyonaga]

On Wed, 18 Sep 2024 05:53:05 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:

> > Hi @tstuefe,
> > > Ah thank you. I think I added that printing ages ago. I should not have used tty. The immediate solution to get you going may be to just change this from tty to fileStream fs(stderr);
> > 
> > 
> > Thanks! I adopted your suggestion and replaced tty. The lock conflict is gone now.
> > > Ensuring that we feed NMT the tracking information in the order they happened. This is only relevant for mmap accounting, since only there do we need to track address ranges. For malloc accounting, we just increase/decrease counters, and there, the order of operations does not matter, just that we do it atomically.
> > > ...
> > > A pragmatic solution would be to move the locking down the stack. Currently, we lock in the generic layer around { pd_release_memory() + MemTracker::record }. But we only need to lock around the OS allocation function (mmap/munmap/VirtualAlloc etc). So we could move the locks downward to the OS specific layer where the actual allocations happen in the OS code, e.g. just lock around { munmap() + MemTracker::record }.
> > 
> > 
> > Ok I see. So the locking should indeed not happen inside of `MemTracker`. Yes, maybe moving the locking down the stack into platform specific code would be a good task for the future. However, if the intention is to feed NMT the tracking info in the order it happened, why are the operations on memory in `os::commit_memory` and `os::reserve_memory` not protected by the lock? In those, we delay and lock inside `MemTracker` instead.
> 
> That looks like an error that should be fixed.

Ok this is probably a task to do alongside ensuring all code paths are protected by the lock. So afterward, we can maybe use plain counters instead of atomic ones for virtual memory tracking.

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

PR Comment: https://git.openjdk.org/jdk/pull/20852#issuecomment-2364273437