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

[Thomas Stuefe]

On Tue, 30 Jul 2024 09:40:18 GMT, Johan Sjölen <jsjolen 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 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 tagged with either mtClassSpace or mtMetaspace, and in the course of Lilliput we may see even more tags. In all cases I reuse the same allocator code. Here, why should I be forced to have to specify the tag on commit? I already marked the underlying memory with the correct tag when reserving it.
> > The ability of painting regions with a certain flag and then having subsequent memory operations on that memory preserve the flag is powerful. You can, for example, in Metaspace account memory at the chunk level. Without having to add code everywhere that keeps track of those flags. You only have to paint the chunks you give to a Metaspace arena with the correct flag, and from then on the whole process is automatic.
> 
> I don't disagree with this, I wasn't thinking about committing when I wrote my comments.

Okay

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

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