Skip to content

Commit

Permalink
Prevent requesting exclusive, if already acquired
Browse files Browse the repository at this point in the history
This is to prevent requestExclusiveVMAccessMetronomeTemp (used from
alarm thread) from blocking if exclusive VM access is already requested
prior to calling a system GC. This prevention
(synchronizeRequestsFromExternalThread) already works for plain
exclusive, but not if safePoint is acquired (since it does not set
exclusiveAccessState to J9_XACCESS_EXCLUSIVE).

While proper solution might be around fixing
synchronizeRequestsFromExternalThread to account for safePointState and
how exclusiveAccessState is changed during safePoint acquire, it would
require more time to get it right.

This solution is a workaround where there is an additional flag that is
to be set by after safePoint is acquired and before GC is invoked. That
flag will be checked by requestExclusiveVMAccessMetronomeTemp before
actually proceeding with the request.

There should be no timing hole where
requestExclusiveVMAccessMetronomeTemp is called after safe point is
acquired and the flag is set, since
requestExclusiveVMAccessMetronomeTemp itself cannot be invoked if GC is
not in progress (and indeed system GC has not been triggered yet).

Signed-off-by: Aleksandar Micic <[email protected]>
  • Loading branch information
Aleksandar Micic authored and Aleksandar Micic committed Dec 14, 2023
1 parent fe20d7a commit 76062ef
Show file tree
Hide file tree
Showing 3 changed files with 33 additions and 5 deletions.
12 changes: 8 additions & 4 deletions runtime/jvmti/jvmtiClass.c
Original file line number Diff line number Diff line change
Expand Up @@ -1233,10 +1233,14 @@ redefineClassesCommon(jvmtiEnv* env,
/* Eliminate dark matter so that none will be encountered in prepareToFixMemberNames(). */
UDATA savedAllowUserHeapWalkFlag = vm->requiredDebugAttributes & J9VM_DEBUG_ATTRIBUTE_ALLOW_USER_HEAP_WALK;
vm->requiredDebugAttributes |= J9VM_DEBUG_ATTRIBUTE_ALLOW_USER_HEAP_WALK;
/* J9MMCONSTANT_EXPLICIT_GC_RASDUMP_COMPACT allows the GC to run while the current thread is holding
* exclusive VM access.
*/
vm->memoryManagerFunctions->j9gc_modron_global_collect_with_overrides(currentThread, J9MMCONSTANT_EXPLICIT_GC_RASDUMP_COMPACT);

/* This is to help with Metronome to avoid requesting Exclusive if we already have SafePoint, which may not look as Exclusive to the requester thread */
vm->alreadyHaveExclusive = TRUE;

vm->memoryManagerFunctions->j9gc_modron_global_collect_with_overrides(currentThread, J9MMCONSTANT_EXPLICIT_GC_EXCLUSIVE_VMACCESS_ALREADY_ACQUIRED);

vm->alreadyHaveExclusive = FALSE;

if (0 == savedAllowUserHeapWalkFlag) {
/* Clear the flag to restore its original value. */
vm->requiredDebugAttributes &= ~J9VM_DEBUG_ATTRIBUTE_ALLOW_USER_HEAP_WALK;
Expand Down
1 change: 1 addition & 0 deletions runtime/oti/j9nonbuilder.h
Original file line number Diff line number Diff line change
Expand Up @@ -5911,6 +5911,7 @@ typedef struct J9JavaVM {
UDATA addModulesCount;
UDATA safePointState;
UDATA safePointResponseCount;
BOOLEAN alreadyHaveExclusive;
struct J9VMRuntimeStateListener vmRuntimeStateListener;
#if defined(J9VM_INTERP_ATOMIC_FREE_JNI_USES_FLUSH)
#if defined(J9UNIX) || defined(AIXPPC)
Expand Down
25 changes: 24 additions & 1 deletion runtime/vm/VMAccess.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -755,6 +755,11 @@ acquireExclusiveVMAccessFromExternalThread(J9JavaVM * vm)
UDATA vmResponsesExpected = 0;
UDATA jniResponsesExpected = 0;

/* If exclusive has already been acquired, nothing need be done here */
if (vm->alreadyHaveExclusive) {
return;
}

synchronizeRequestsFromExternalThread(vm, TRUE);

/* Post the halt request to all threads */
Expand Down Expand Up @@ -836,6 +841,12 @@ void
releaseExclusiveVMAccessFromExternalThread(J9JavaVM * vm)
{
J9VMThread * currentThread;

/* If exclusive has already been acquired, nothing need be done here */
if (vm->alreadyHaveExclusive) {
return;
}

Assert_VM_true(J9_XACCESS_EXCLUSIVE == vm->exclusiveAccessState);

/* Acquire these monitors in the same order as in allocateVMThread to prevent deadlock */
Expand Down Expand Up @@ -913,6 +924,11 @@ requestExclusiveVMAccessMetronomeTemp(J9JavaVM *vm, UDATA block, UDATA *vmRespon
UDATA jniResponsesExpected = 0;
*gcPriority = J9THREAD_PRIORITY_MAX;

/* If exclusive has already been acquired, nothing need be done here */
if (vm->alreadyHaveExclusive) {
return FALSE;
}

/* Check if another party is requesting X access already. */
if (FALSE == synchronizeRequestsFromExternalThread(vm, block)) {
/* Yes, there was another party requesting X access, but because we did not want to block,
Expand Down Expand Up @@ -1010,7 +1026,14 @@ waitForExclusiveVMAccessMetronome(J9VMThread * vmThread, UDATA responsesRequired
void
waitForExclusiveVMAccessMetronomeTemp(J9VMThread * vmThread, UDATA vmResponsesRequired, UDATA jniResponsesRequired)
{
waitForResponseFromExternalThread(vmThread->javaVM, vmResponsesRequired, jniResponsesRequired);
J9JavaVM *vm = vmThread->javaVM;

/* If exclusive has already been acquired, nothing need be done here */
if (vm->alreadyHaveExclusive) {
return;
}

waitForResponseFromExternalThread(vm, vmResponsesRequired, jniResponsesRequired);

VM_VMAccess::backOffFromSafePoint(vmThread);

Expand Down

0 comments on commit 76062ef

Please sign in to comment.