RFR: 8308745: ObjArrayKlass::allocate_objArray_klass may call into java while holding a lock

[David Holmes]

On Tue, 6 Feb 2024 22:59:04 GMT, Coleen Phillimore <coleenp at openjdk.org> wrote:

> This change creates a new sort of native recursive lock that can be held during JNI and Java calls, which can be used for synchronization while creating objArrayKlasses at runtime.
> 
> Passes tier1-4.

src/hotspot/share/classfile/classLoaderData.cpp line 412:

> 410:   // To call this, one must have the MultiArray_lock held, but the _klasses list still has lock free reads.
> 411:   assert_locked_or_safepoint(MultiArray_lock);
> 412: 

Do we no longer hold the lock or are you just missing the API to assert it is held?

src/hotspot/share/oops/arrayKlass.cpp line 141:

> 139:           ObjArrayKlass::allocate_objArray_klass(class_loader_data(), dim + 1, this, CHECK_NULL);
> 140:       // use 'release' to pair with lock-free load
> 141:       release_set_higher_dimension(ak);

Why has this code changed? I only expected to see the lock changed.

src/hotspot/share/prims/jvmtiExport.cpp line 3151:

> 3149:   if (MultiArray_lock->owner() == thread) {
> 3150:     return false;
> 3151:   }

So the recursive nature of the lock now means we don't have to bail out here - right?

src/hotspot/share/prims/jvmtiGetLoadedClasses.cpp line 107:

> 105:     // To get a consistent list of classes we need MultiArray_lock to ensure
> 106:     // array classes aren't created.
> 107:     MutexLocker ma(MultiArray_lock);

Unclear why this is no longer the case.

src/hotspot/share/runtime/recursiveLock.cpp line 45:

> 43:     _recursions++;
> 44:     assert(_recursions == 1, "should be");
> 45:     Atomic::release_store(&_owner, current); // necessary?

No release necessary. The only thing written since the sem.wait is recursions and it is only  updated or needed by the owning thread.

src/hotspot/share/runtime/recursiveLock.cpp line 54:

> 52:   _recursions--;
> 53:   if (_recursions == 0) {
> 54:     Atomic::release_store(&_owner, (Thread*)nullptr); // necessary?

No release necessary.

src/hotspot/share/runtime/recursiveLock.hpp line 33:

> 31: // There are also no checks that the recursive lock is not held when going to Java or to JNI, like
> 32: // other JVM mutexes have.  This should be used only for cases where the alternatives with all the
> 33: // nice safety features don't work.

Mention that it does interact with safepoints properly for JavaThreads

src/hotspot/share/runtime/recursiveLock.hpp line 45:

> 43:   void unlock(Thread* current);
> 44: };
> 45: 

Should expose a `holdsLock` method to allow use in assertions.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/17739#discussion_r1483613181
PR Review Comment: https://git.openjdk.org/jdk/pull/17739#discussion_r1483614823
PR Review Comment: https://git.openjdk.org/jdk/pull/17739#discussion_r1483623614
PR Review Comment: https://git.openjdk.org/jdk/pull/17739#discussion_r1483624460
PR Review Comment: https://git.openjdk.org/jdk/pull/17739#discussion_r1483608746
PR Review Comment: https://git.openjdk.org/jdk/pull/17739#discussion_r1483609712
PR Review Comment: https://git.openjdk.org/jdk/pull/17739#discussion_r1483610648
PR Review Comment: https://git.openjdk.org/jdk/pull/17739#discussion_r1483612492