RFR: 8369622: GlobalChunkPoolMutex needs to be recursive
David Holmes
dholmes at openjdk.org
Mon Oct 13 06:43:05 UTC 2025
On Sun, 12 Oct 2025 12:55:53 GMT, Johan Sjölen <jsjolen at openjdk.org> wrote:
> The GlobalChunkPoolMutex replaced ThreadCritical in [JDK-8356173](https://bugs.openjdk.org/browse/JDK-8356173). In NMT, we can unfortunately reach a recursive locking situation in situations when the VM is crashing and NMT attempts to perform a error report. I suggest that we make this particular mutex recursive, until we can resolve the design issues that causes this.
>
> This PR also replaces the static initialization with `DeferredStatic`, and fixes the indentation of access modifiers to be consistently 0-indented.
>
> Please note that we do not take the lock in NMT in order to allocate memory, but in order to have a "static picture" of the arena counters.
src/hotspot/share/memory/arena.cpp line 43:
> 41: // code and other areas. For many calls, the current thread has not
> 42: // been created so we cannot use Mutex.
> 43: class RecursivePlatformMutex : public PlatformMutex {
It is a shame that we needed to (re)invent yet-another lock class.
src/hotspot/share/memory/arena.cpp line 49:
> 47:
> 48: ChunkPoolLocker::ChunkPoolLocker() {
> 49: assert(GlobalChunkPoolMutex != nullptr, "must be initialized");
Do we have an equivalent assertion to replace this with?
src/hotspot/share/memory/arena.cpp line 55:
> 53: PlatformMutex::lock();
> 54: _owner = t;
> 55: }
As per the comment:
// For many calls, the current thread has not
// been created so we cannot use Mutex.
this ownership test won't allow for recursive use in those circumstances. So we have slightly odd semantics here that the mutex is recursive for attached threads but not for un-attached. For this specialised usecase maybe we can tolerate that.
src/hotspot/share/memory/arena.cpp line 250:
> 248: static const int cleaning_interval = 5000; // cleaning interval in ms
> 249:
> 250: public:
The style guide doesn't mention this but I thought the unofficial preference was for one character indent so that we don't have lines at the same level as the outermost class definition.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/27759#discussion_r2425331178
PR Review Comment: https://git.openjdk.org/jdk/pull/27759#discussion_r2425309901
PR Review Comment: https://git.openjdk.org/jdk/pull/27759#discussion_r2425336141
PR Review Comment: https://git.openjdk.org/jdk/pull/27759#discussion_r2425338008
More information about the hotspot-runtime-dev
mailing list