On Wed, 18 Sep 2024 05:53:05 GMT, Thomas Stuefe <[email protected]> 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
