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 Mon, 29 Jul 2024 06:23:09 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:

>> Afshin Zafari has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   reserve always use the flag in metadata
>
> src/hotspot/share/nmt/vmatree.cpp line 37:
> 
>> 35: 
>> 36: VMATree::SummaryDiff VMATree::register_mapping(position A, position B, StateType state,
>> 37:                                                const RegionData& metadata, bool copy_flag) {
> 
> We need to think about what the behavior should be if the caller passes `copy_flag=true` but vmatree disagrees and thinks the caller should specify a flag. At the moment we quietly take over the flag from the `metadata` argument, but I think we should assert. Something like "reserving without pre-existing range requires a valid NMT flag".
> 
> Tied into that is the question of what the flag in the `metadata` argument should be for `copy_flag=true`. I would argue that the flag must be `mtNone` - any other flag would contradict the "copy_flag" argument.
> 
> In fact, we could argue that `mtNone` means `copy_flag==true` and then we don't need the new `copy_flag` argument. In that case, reserving with `mtNone` means "no clue about the flag, you figure it out" and then it requires a pre-existing mapping. Are there valid use cases for reserving ranges with mtNone as a valid NMT flag?

Asserting may make sense on debug builds, as you might be making a mistake, but we should have pre-defined behavior for production builds anyhow, no `ShouldNotReachHere()`. I think that valid behavior is to set the flag to whichever was provided in the call.


Another option is to have a more elaborate heuristic. If you uncommit `[a, b)` then find the first range which starts inside of `[a, b)` and take its memory flag, if none is found then give up and take `mtNone`.

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. 

>Are there valid use cases for reserving ranges with mtNone as a valid NMT flag?

Only in the context of then later setting the memory flag, which I think is a pattern which should be avoided if possible.

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

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