RFR: 8341491: Reserve and commit memory operations should be protected by NMT lock

[Stefan Karlsson]

On Wed, 26 Mar 2025 14:16:06 GMT, Robert Toyonaga <duke at openjdk.org> wrote:

> > Are there any code that we know of that doesn't fit into a synchronization pattern similar to the above?
> > I can think of some contrived example where Thread B asks the OS for memory mappings and uses that to ascertain that a pre-determined address has been reserved, and how that could lead to an incorrect booking as you described, but do we really have code like that?
> 
> From what I can tell, it doesn't look like that's happening anywhere, someone else may know better though. Similarly, for uncommit, the base address must be passed over from somewhere else in the JVM so relying on some external synchonization seems reasonable here too. If this problem scenario is not present in the current code and it's not expected it to become a possiblity in the future, then I suppose there's no reason to guard against it. Maybe just a comment explaining the reasoning is good enough (and a warning not to use such patterns).
> 
> > When does a release/uncommit fail? Would that be a JVM bug?
> 
> On Windows, VirtualFree also looks like it only fails if an invalid set of arguments are passed. So if os::pd_release fails it's probably a JVM bug. Uncommit uses mmap, which could fail for a larger variety of reasons. Some reasons are out of control of the JVM. For example: "The number of mapped regions would exceed an implementation-defined limit (per process or per system)." See [here](https://github.com/openjdk/jdk/blob/jdk-25%2B15/src/hotspot/share/memory/metaspace/virtualSpaceNode.cpp#L191)

Right. And that failure is fatal, so there should be no need to fix any NMT bookkeeping for that.

> 
> > What state is the memory in when such a failure happens? Do we even know if the memory is still committed if an uncommit fails?
> 
> If release/uncommit fails, then it would be hard to know what state the target memory is in. If the arguments are invalid (bad base address), the target region may not even be allocated. Or, in the case of uncommit, if the base address is not aligned, maybe the target committed region does indeed exist but the uncommit still fails. So it would be hard to determine how to readjust the NMT accounting afterward.

Agreed. And this would be a pre-existing problem already. If a release/uncommit fails, then we have the similar issues for that as well.

> 
> > I don't understand why we don't treat that as a fatal error OR make sure that all call-sites handles that error, which they don't do today.
> 
> I think release/uncommit failures should be handled by the callers. Currently, uncommit failure is handled in most places by the caller, release failure seems mostly not. Since, at least for uncommit, we could sometimes fail for valid reasons, I think we shouldn't fail fatally in the os:: functions.

I would like to drill a bit deeper into this. Do you have any concrete examples of an uncommit failure that should not be handled as a fatal error?

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

PR Comment: https://git.openjdk.org/jdk/pull/24084#issuecomment-2754953604