RFR: 8335091: NMT: VMATree reserve_mapping and commit_mapping APIs need MEMFLAGS while un/-committing API has no MEMFLAGS arg [v3]

[Johan Sjölen]

On Tue, 24 Sep 2024 17:11:50 GMT, Gerard Ziemski <gziemski at openjdk.org> wrote:

>>> > I disagree :)
>>> > The point of this default mechanism is to take cognitive load from the programmer. I don't want to have to think about the flag when I rely on this mechanism. Having this mechanism, but then still forcing the caller to specify a flag "just in case you commit into unreserved territory" is not a big help.
>>> > By using copy_flag=true, I declare that I expect there to be a prior mapping in that range. That is part of the API contract. If that expectation is broken, I did something wrong. This is not something that can happen due to random runtime conditions. Therefore, like any other invalid state it should be handled with an assert. I don't think it makes a lot of sense to think about what to do in release builds: this is an error like many others, and we have many asserts in the JVM and we only rarely encode a valid non-assert path.
>>> 
>>> I don't understand what you disagree with specifically. I'm saying that asserting is fine, but we shouldn't give up and kill the VM in release builds with a `ShouldNotReachHere()` in case the programmer messed up when reporting diagnostics to NMT. It's not necessarily a bug in the actual 'business logic' of the code and we can recover from it by providing worse diagnostics.
>> 
>> oh, I misunderstood you then. Sure, that is fine.
>> 
>>> 
>>> > ```
>>> > The nicest behavior is an O(n) algorithm where we go through all regions in [a, b) and convert each to its reserved version with the flag, then uncommit with holes in the range would leave those holes unreserved.
>>> > ```
>>> > 
>>> > 
>>> >     
>>> >       
>>> >     
>>> > 
>>> >       
>>> >     
>>> > 
>>> >     
>>> >   
>>> > You lost me here. This is what we do right now, isn't it? VMATree just replaces all existing mappings between [a, b) with the new mapping?
>>> 
>>> I was thinking in the sense of `uncommit`. Example:
>>> 
>>> ```
>>> reserve(0, 100, mtTest);
>>> reserve(600, 1000, mtTest);
>>> //  [0, 100) and [600, 1000) reserved with flag mtTest
>>> commit(0, 100); commit(600, 1000);
>>> //  [0, 100) and [600, 1000) committed with flag mtTest
>>> // Now we can do
>>> uncommit(0, 1000);
>>> // And we end up with [0, 100) and [600, 1000) being reserved with flag mtTest
>>> ```
>> 
>> In VMAtree sure, but in reality not: since NMT has no complete knowledge about the address space. Uncommitting a range containing what NMT sees as address holes will wipe any mappings that happen to be in these holes that NMT does not know. Which are quite a lot.
>> 
>>> 
>>> That is not equi...
>
> `// If we specify copy_flag then the new region takes over the current flag instead of the flag in metadata.
> `
> The argument name `copy_flag` suggests to me that we will be using the provided `flag` from `metadata`, but the implementation does the reverse.
> 
> I think we might need a better name, ex: `use_implicit_flag` in which case we copy the flag out of the region itself, or we could use `use_explicit_flag` and do the opposite.

I see what you're saying, maybe `use_found_flag` or `use_flag_inplace`? At least the assertions give a strong hint that that assumption is wrong. Better names are (almost) always welcome.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/20330#discussion_r1774610654