RFR: 8351661: NMT: VMATree should support separate call-stacks for reserve and commit operations [v2]

[Johan Sjölen]

On Thu, 13 Mar 2025 13:13:15 GMT, Afshin Zafari <azafari at openjdk.org> wrote:

>> In NMT detail mode, we need to have separate call-stacks for Reserve and Commit operations.
>> This PR adds a second stack to every node that will be used when committing (and uncommitting) the start node of a reserved region.
>
> Afshin Zafari has updated the pull request incrementally with one additional commit since the last revision:
> 
>   fixed a bug.

Hi,

I'd like to see this have clearer defined semantics written out into words in the PR description.

What are the expected semantics of this snippet in terms of the resulting nodes and their stacks?


// (address, size, callstack) arguments
# Case 1
tree.reserve(0, 100, s1);
tree.commit(25, 25, s2);
# Case 2
tree.reserve(0, 100, s1);
tree.reserve(10, 10, s2);
# Case 3
tree.commit(0, 100, s1);
# Case 4
tree.commit(0, 100, s1);
tree.reserve(0, 100, s2);


I'd expect them to be:


# Case 1
[] 0 R[s1] - [s1]R 25 RC[s1, s2] - [s1, s2]RC 50 - R[s1]  - [s1]R 100 []
# Case 2
[] 0 R[s2] - [s2]R 100 []
# Case 3
[] 0 RC[s1, s1] - [s1, s1]RC 100 []
# Case 4
[] 0 R[s2] - [s2]R 100 []


Is my intuition correct?

Thanks,
Johan

src/hotspot/share/nmt/nmtNativeCallStackStorage.hpp line 53:

> 51: 
> 52:   static bool is_invalid(StackIndex a) {
> 53:     return a == invalid || a < 0;

If this change is necessary, then there's a bug in the code.

src/hotspot/share/nmt/vmatree.cpp line 96:

> 94:           && !(state == StateType::Reserved && !use_tag_inplace)            // we are not reserving a new region
> 95:           && !NativeCallStackStorage::is_invalid(leqA_n->val().out.stack()) // the primary stack is already filled
> 96:          ) {

These conditions can be refactored out and given useful names:

```c++
bool baz = ...;
bool foo = ...;
boo bar = ...;
if (baz || (foo && bar)) { /* ... */ }

src/hotspot/share/nmt/vmatree.hpp line 95:

> 93:     uint8_t type_tag[2];
> 94:     NativeCallStackStorage::StackIndex sidx;       // call-stack of all operations
> 95:     NativeCallStackStorage::StackIndex second_idx; // call-stack when committing/uncommitting the start-node of a reserved region

These can be called `primary_stack` and `secondary_stack`. Does an uncommitted region really have a stack?

src/hotspot/share/nmt/vmatree.hpp line 128:

> 126: 
> 127:     NativeCallStackStorage::StackIndex second_stack() const {
> 128:       return second_idx;

Rename accessors into `reserved_stack` and `committed_stack` and add `bool has_committed_stack()`?

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

PR Review: https://git.openjdk.org/jdk/pull/24028#pullrequestreview-2689829076
PR Review Comment: https://git.openjdk.org/jdk/pull/24028#discussion_r1998370903
PR Review Comment: https://git.openjdk.org/jdk/pull/24028#discussion_r1998386428
PR Review Comment: https://git.openjdk.org/jdk/pull/24028#discussion_r1998377261
PR Review Comment: https://git.openjdk.org/jdk/pull/24028#discussion_r1998379132