On Tue, 1 Apr 2025 15:29:55 GMT, Stefan Karlsson <stef...@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
>> - swapping the order of `NmtVirtualMemoryLocker` and release/uncommit
>> - Fail fatally if release/uncommit does not complete.
>>
>> 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);
>
>> 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).
Thank you @stefank for the feedback. I've applied your suggestions.
@tstuefe, when you have time, can you please have another look at this? Based
on the discussion above, I've reverted the changes to the locking scope in
favor of new comments explaining the asymmetrical locking and warning against
patterns that lead to races. The new tests that are still relevant are kept.
-------------
PR Comment: https://git.openjdk.org/jdk/pull/24084#issuecomment-2790269355
