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

[Gerard Ziemski]

On Tue, 30 Jul 2024 10:14:58 GMT, Thomas Stuefe <stuefe 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.
>> 
>>> 
>>> 
>>>     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
>> 
>> 
>> That is not equivalent to `register_mapping` right now, but `register_mapping` already does everything to support this kind of operation.
>> 
>> 
>> 
>> 
>>> 
>>>     I'm only considering the usage of this flag as something which should be used by an uncommit_region API, if that wasn't clear.
>>> 
>>> Restricting this to uncommit would not use its full potential.
>>> 
>>> Consider this example: In the Metaspace allocator I would like to commit memory, but that memory can be t...
>
>> > 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 equivalent to `register_mapping` right now, but `reg...

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.

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

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