On Mon, 31 Mar 2025 14:09:22 GMT, Robert Toyonaga <d...@openjdk.org> wrote:
> OK should I update this PR to do the following things:
>
> * Add comments explaining the asymmetrical locking and warning against
> patterns that lead to races
Sounds like a good idea.
>
> * swapping the order of `NmtVirtualMemoryLocker` and release/uncommit
I wonder if this should be done as new RFE after the change below. It might
need a bit of investigation to make sure that the reasoning around this is
correct.
>
> * Fail fatally if release/uncommit does not complete.
I think this would be a good, separate RFE to be done before we try to swap the
order.
>
>
> Or does it make more sense to do that in a different issue/PR?
>
> Also, do we want to keep the new tests and the refactorings (see below)?
>
> ```
> if (MemTracker::enabled()) {
> MemTracker::NmtVirtualMemoryLocker nvml;
> result = pd_some_operation(addr, bytes);
> if (result != nullptr) {
> MemTracker::record_some_operation(addr, bytes);
> }
> } else {
> result = pd_unmap_memory(addr, bytes);
> }
> ```
>
> To:
>
> ```
> MemTracker::NmtVirtualMemoryLocker nvml;
> result = pd_unmap_memory(addr, bytes);
> MemTracker::record_some_operation(addr, bytes);
> ```
My thinking is that after you done (2) above, then you will not need to expose
the NMT lock to this level. The code would be:
MemTracker::record_some_operation(addr, bytes); // Lock confined inside this
pd_unmap_memory(addr, bytes);
So, I would wait with this cleanup until we know more about (2).
-------------
PR Comment: https://git.openjdk.org/jdk/pull/24084#issuecomment-2769766908
