[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-hotspot-runtime-dev
Subject:    Re: RFR: 8297718: Make NMT free:ing protocol more granular
From:       Thomas Stuefe <stuefe () openjdk ! org>
Date:       2022-11-28 13:54:07
Message-ID: 6cSrZVXrckQSOy1TGsU2cbchTbNMgG7nSSzaZ21fhfM=.a19d68c4-3d2d-41d4-81e4-7449ef148b22 () github ! com
[Download RAW message or body]

On Mon, 28 Nov 2022 12:38:38 GMT, Johan Sjölen <jsjolen@openjdk.org> wrote:

> This PR avoids the double registration of a failed `os::realloc` in NMT by doing \
> the NMT free protocol steps inline (asserting block integrity, marking of \
> MallocHeader, and registrering results to statistics). This is done by creating a \
> new struct `FreePackage` which includes all of the necessary data for registrering \
> results. 
> I also did some light refactoring which I think makes the protocol clearer, these \
> are done piece wise as separate commits for the reviewers' convenience.

Mostly good. Mostly style nits.

We now handle correctly a very rare condition that would lead to misaccounting if a \
realloc failure occurred and the old block was accounted under a different MEMFLAGs. \
The price is a very slight premium we pay now because we dive down into NMT twice, \
once to check block integrity, and once to de-account. But since reallocs are not \
that hot, this should not be noticeable.

Can you please add a small gtest to test that MallocHeader::mark_block_as_dead is \
fully reversible with mark_block_as_alive, and that the block passes integrity checks \
afterwards?

src/hotspot/share/runtime/os.cpp line 721:

> 719:     header->assert_block_integrity(); // Assert block hasn't been tampered \
>                 with.
> 720:     const FreePackage free_package = header->free_package();
> 721:     header->mark_block_as_dead();

Please revive the old comment, suitably adapted. It should still mostly apply, but \
please mention that de-accounting happens delayed since realloc may fail.

src/hotspot/share/services/mallocHeader.hpp line 95:

> 93:   const MEMFLAGS flags;
> 94:   const uint32_t mst_marker;
> 95: };

Could this be nested into MallocHeader, and renamed to something like "FreeInfo"?

src/hotspot/share/services/mallocHeader.hpp line 125:

> 123: 
> 124:  public:
> 125:   MallocHeader(const MallocHeader&) = default;

This makes me a bit nervous, where do we copy malloc headers?

src/hotspot/share/services/mallocHeader.hpp line 133:

> 131:   bool get_stack(NativeCallStack& stack) const;
> 132: 
> 133:   // Return the necessary data to record the block this belongs to as freed

Proposal: ".. to deaccount block with NMT" (since "record as freed" is ambivalent and \
could refer to different things, e.g. header marks.

src/hotspot/share/services/mallocHeader.hpp line 138:

> 136:   }
> 137:   inline void mark_block_as_dead();
> 138:   inline void mark_block_as_alive();

Proposal: name this "revive" and assert for dead markers. So far this is a one-trick \
pony used in realloc(), we don't really want a general way to mark dead headers as \
life.

src/hotspot/share/services/mallocHeader.inline.hpp line 54:

> 52: }
> 53: 
> 54: inline void MallocHeader::mark_block_as_dead() {

Can you please add a comment to mark_block_as_dead() that its effects should be fully \
reversible with mark_block_as_alive (or "revive()", if you take my suggestion).

src/hotspot/share/services/mallocTracker.cpp line 202:

> 200:   header->assert_block_integrity();
> 201: 
> 202:   record_free(header->free_package());

Proposal: name this "deaccount(x)", since that's what it does. Then you can keep the \
original name for record_free().

src/hotspot/share/services/mallocTracker.hpp line 298:

> 296:   static void* record_free_block(void* memblock);
> 297:   // Record a free.
> 298:   static void record_free(FreePackage free_package);

Can you expand on the comment a bit?

`Given a block returned by os::malloc() or os::realloc(), de-account block from NMT, \
mark its header as dead and return pointer to header.`.

`Given the free info from a block, de-account block from NMT.`.

src/hotspot/share/services/memTracker.hpp line 114:

> 112:   static inline void record_free(FreePackage free_package) {
> 113:     assert(enabled(), "NMT must be enabled");
> 114:     MallocTracker::record_free(free_package);

Please leave the `enabled()` checks down in `MemTracker`. You rob it of its purpose \
in life :) Seriously, should we want to revise that, we should do it for all \
MemTracker methods.

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

PR: https://git.openjdk.org/jdk/pull/11390


[prev in list] [next in list] [prev in thread] [next in thread]