Synchronization between continuation mounting and concurrent scanning

There is a race condition between continuation mounting(swapping java
 stacks with carrier thread) and concurrent continuation scanning the
 related java stacks).

 use atomic operations with continuation->state to synchronizer
 to avoid to introduce a new Mutex.
 1, GC concurrent scanning use low bit of continuation->state for enter
  and exit ConcurrentGCScan(J9_GC_CONTINUATION_STATE_CONCURRENT_SCAN,
  tryWinningConcurrentGCScan(), exitConcurrentGCScan()).
 2, mounting use atomic "or" set continuation->state
  (synchronizeWithConcurrentGCScan()).
 3, if mounting enter earlier than scanning, then drop scanning
 4, if scanning enter earlier than mounting, then mounting is going to
  wait scanning complete
 5, after scanning complete, need to clear the low bit
  (J9_GC_CONTINUATION_STATE_CONCURRENT_SCAN) and if mounting is in
  waiting, notify the mounting.

Signed-off-by: Lin Hu <linhu@ca.ibm.com>

runtime/gc_glue_java/CompactSchemeFixupObject.cpp

@@ -76,7 +76,7 @@ MM_CompactSchemeFixupObject::fixupContinuationNativeSlots(MM_EnvironmentStandard
 	 * mounted Virtual threads later during root fixup, we will skip it during this heap fixup pass
 	 * (hence passing true for scanOnlyUnmounted parameter).
 	 */
-	if (VM_VMHelpers::needScanStacksForContinuation(currentThread, objectPtr, true)) {
+	if (VM_VMHelpers::needScanStacksForContinuation(currentThread, objectPtr)) {
 		StackIteratorData4CompactSchemeFixupObject localData;
 		localData.compactSchemeFixupObject = this;
 		localData.env = env;

runtime/gc_glue_java/HeapWalkerDelegate.cpp

@@ -58,14 +58,15 @@ void
 MM_HeapWalkerDelegate::doContinuationNativeSlots(MM_EnvironmentBase *env, omrobjectptr_t objectPtr, MM_HeapWalkerSlotFunc function, void *userData)
 {
 	J9VMThread *currentThread = (J9VMThread *)env->getLanguageVMThread();
+
 	if (VM_VMHelpers::needScanStacksForContinuation(currentThread, objectPtr)) {
 		StackIteratorData4HeapWalker localData;
 		localData.heapWalker = _heapWalker;
 		localData.env = env;
 		localData.fromObject = objectPtr;
 		localData.function = function;
 		localData.userData = userData;
-		/* so far there is no case we need ClassWalk for heapwalker, so we set bStackFrameClassWalkNeeded = false */
+		/* so far there is no case we need ClassWalk for heapwalker, so we set stackFrameClassWalkNeeded = false */
 		GC_VMThreadStackSlotIterator::scanSlots(currentThread, objectPtr, (void *)&localData, stackSlotIteratorForHeapWalker, false, false);
 	}
 }

runtime/gc_glue_java/MarkingDelegate.cpp

@@ -267,12 +267,14 @@ MM_MarkingDelegate::scanContinuationNativeSlots(MM_EnvironmentBase *env, omrobje
 		localData.env = env;
 		localData.fromObject = objectPtr;
 
-		bool bStackFrameClassWalkNeeded = false;
+		bool stackFrameClassWalkNeeded = false;
 #if defined(J9VM_GC_DYNAMIC_CLASS_UNLOADING)
-		bStackFrameClassWalkNeeded = isDynamicClassUnloadingEnabled();
+		stackFrameClassWalkNeeded = isDynamicClassUnloadingEnabled();
 #endif /* J9VM_GC_DYNAMIC_CLASS_UNLOADING */
 
-		GC_VMThreadStackSlotIterator::scanSlots(currentThread, objectPtr, (void *)&localData, stackSlotIteratorForMarkingDelegate, bStackFrameClassWalkNeeded, false);
+		/* In STW GC there are no racing carrier threads doing mount and no need for the synchronization. */
+		bool syncWithContinuationMounting = J9_ARE_ANY_BITS_SET(currentThread->privateFlags, J9_PRIVATE_FLAGS_CONCURRENT_MARK_ACTIVE);
+		GC_VMThreadStackSlotIterator::scanSlots(currentThread, objectPtr, (void *)&localData, stackSlotIteratorForMarkingDelegate, stackFrameClassWalkNeeded, false, syncWithContinuationMounting);
 	}
 }
 

runtime/gc_glue_java/MetronomeDelegate.cpp

@@ -199,7 +199,7 @@ MM_MetronomeDelegate::initialize(MM_EnvironmentBase *env)
 	if (NULL == accessBarrier) {
 		return false;
 	}
-	MM_GCExtensions::getExtensions(_javaVM)->accessBarrier = (MM_ObjectAccessBarrier *)accessBarrier;
+	_extensions->accessBarrier = (MM_ObjectAccessBarrier *)accessBarrier;
 
 	_javaVM->realtimeHeapMapBasePageRounded = _markingScheme->_markMap->getHeapMapBaseRegionRounded();
 	_javaVM->realtimeHeapMapBits = _markingScheme->_markMap->getHeapMapBits();
@@ -1653,12 +1653,15 @@ MM_MetronomeDelegate::scanContinuationNativeSlots(MM_EnvironmentRealtime *env, J
 		localData.env = env;
 		localData.fromObject = objectPtr;
 
-		bool bStackFrameClassWalkNeeded = false;
+		bool stackFrameClassWalkNeeded = false;
 #if defined(J9VM_GC_DYNAMIC_CLASS_UNLOADING)
-		bStackFrameClassWalkNeeded = isDynamicClassUnloadingEnabled();
+		stackFrameClassWalkNeeded = isDynamicClassUnloadingEnabled();
 #endif /* J9VM_GC_DYNAMIC_CLASS_UNLOADING */
 
-		GC_VMThreadStackSlotIterator::scanSlots(currentThread, objectPtr, (void *)&localData, stackSlotIteratorForRealtimeGC, bStackFrameClassWalkNeeded, false);
+		/* In STW GC there are no racing carrier threads doing mount and no need for the synchronization. */
+		bool syncWithContinuationMounting = _realtimeGC->isCollectorConcurrentTracing();
+
+		GC_VMThreadStackSlotIterator::scanSlots(currentThread, objectPtr, (void *)&localData, stackSlotIteratorForRealtimeGC, stackFrameClassWalkNeeded, false, syncWithContinuationMounting);
 	}
 }
 

runtime/gc_glue_java/ScavengerDelegate.cpp

@@ -351,8 +351,10 @@ MM_ScavengerDelegate::scanContinuationNativeSlots(MM_EnvironmentStandard *env, o
 		localData.env = env;
 		localData.reason = reason;
 		localData.shouldRemember = &shouldRemember;
+		/* In STW GC there are no racing carrier threads doing mount and no need for the synchronization. */
+		bool  syncWithContinuationMounting = _extensions->isConcurrentScavengerInProgress();
 
-		GC_VMThreadStackSlotIterator::scanSlots(currentThread, objectPtr, (void *)&localData, stackSlotIteratorForScavenge, false, false);
+		GC_VMThreadStackSlotIterator::scanSlots(currentThread, objectPtr, (void *)&localData, stackSlotIteratorForScavenge, false, false, syncWithContinuationMounting);
 	}
 	return 	shouldRemember;
 }

runtime/gc_structs/VMThreadStackSlotIterator.cpp

@@ -136,13 +136,14 @@ GC_VMThreadStackSlotIterator::scanSlots(
 			void *userData,
 			J9MODRON_OSLOTITERATOR *oSlotIterator,
 			bool includeStackFrameClassReferences,
-			bool trackVisibleFrameDepth
+			bool trackVisibleFrameDepth,
+			bool syncWithContinuationMounting
 		)
 {
 	J9StackWalkState stackWalkState;
-	initializeStackWalkState(&stackWalkState, vmThread, userData, oSlotIterator, includeStackFrameClassReferences, trackVisibleFrameDepth);
 
-	VM_VMHelpers::walkContinuationStackFramesWrapper(vmThread, continuationObjectPtr, &stackWalkState);
+	initializeStackWalkState(&stackWalkState, vmThread, userData, oSlotIterator, includeStackFrameClassReferences, trackVisibleFrameDepth);
+	VM_VMHelpers::walkContinuationStackFramesWrapper(vmThread, continuationObjectPtr, &stackWalkState, syncWithContinuationMounting);
 }
 
 #if JAVA_SPEC_VERSION >= 19

runtime/gc_structs/VMThreadStackSlotIterator.hpp

@@ -66,7 +66,8 @@ class GC_VMThreadStackSlotIterator
 			void *userData,
 			J9MODRON_OSLOTITERATOR *oSlotIterator,
 			bool includeStackFrameClassReferences,
-			bool trackVisibleFrameDepth);
+			bool trackVisibleFrameDepth,
+			bool syncWithContinuationMounting = false);
 
 #if JAVA_SPEC_VERSION >= 19
 	static void scanSlots(

runtime/gc_vlhgc/CopyForwardScheme.cpp

@@ -2328,11 +2328,11 @@ MM_CopyForwardScheme::scanContinuationNativeSlots(MM_EnvironmentVLHGC *env, MM_A
 		localData.env = env;
 		localData.fromObject = objectPtr;
 		/* check _includeStackFrameClassReferences, _trackVisibleStackFrameDepth  */
-		bool bStackFrameClassWalkNeeded = false;
+		bool stackFrameClassWalkNeeded = false;
 #if defined(J9VM_GC_DYNAMIC_CLASS_UNLOADING)
-		bStackFrameClassWalkNeeded = isDynamicClassUnloadingEnabled();
+		stackFrameClassWalkNeeded = isDynamicClassUnloadingEnabled();
 #endif /* J9VM_GC_DYNAMIC_CLASS_UNLOADING */
-		GC_VMThreadStackSlotIterator::scanSlots(currentThread, objectPtr, (void *)&localData, stackSlotIteratorForCopyForwardScheme, bStackFrameClassWalkNeeded, false);
+		GC_VMThreadStackSlotIterator::scanSlots(currentThread, objectPtr, (void *)&localData, stackSlotIteratorForCopyForwardScheme, stackFrameClassWalkNeeded, false);
 	}
 }
 

runtime/gc_vlhgc/GlobalMarkingScheme.cpp

@@ -797,12 +797,15 @@ MM_GlobalMarkingScheme::scanContinuationNativeSlots(MM_EnvironmentVLHGC *env, J9
 		localData.globalMarkingScheme = this;
 		localData.env = env;
 		localData.fromObject = objectPtr;
-		bool bStackFrameClassWalkNeeded = false;
+		bool stackFrameClassWalkNeeded = false;
 #if defined(J9VM_GC_DYNAMIC_CLASS_UNLOADING)
-		bStackFrameClassWalkNeeded = isDynamicClassUnloadingEnabled();
+		stackFrameClassWalkNeeded = isDynamicClassUnloadingEnabled();
 #endif /* J9VM_GC_DYNAMIC_CLASS_UNLOADING */
 
-		GC_VMThreadStackSlotIterator::scanSlots(currentThread, objectPtr, (void *)&localData, stackSlotIteratorForGlobalMarkingScheme, bStackFrameClassWalkNeeded, false);
+		/* In STW GC there are no racing carrier threads doing mount and no need for the synchronization. */
+		bool syncWithContinuationMounting = (MM_VLHGCIncrementStats::mark_concurrent == static_cast<MM_CycleStateVLHGC*>(env->_cycleState)->_vlhgcIncrementStats._globalMarkIncrementType);
+
+		GC_VMThreadStackSlotIterator::scanSlots(currentThread, objectPtr, (void *)&localData, stackSlotIteratorForGlobalMarkingScheme, stackFrameClassWalkNeeded, syncWithContinuationMounting);
 	}
 }
 

runtime/gc_vlhgc/WriteOnceCompactor.cpp

@@ -1239,7 +1239,7 @@ MM_WriteOnceCompactor::fixupContinuationNativeSlots(MM_EnvironmentVLHGC* env, J9
 	 * mounted Virtual threads later during root fixup, we will skip it during this heap fixup pass
 	 * (hence passing true for scanOnlyUnmounted parameter).
 	 */
-	if (VM_VMHelpers::needScanStacksForContinuation(currentThread, objectPtr, true)) {
+	if (VM_VMHelpers::needScanStacksForContinuation(currentThread, objectPtr)) {
 		StackIteratorData4WriteOnceCompactor localData;
 		localData.writeOnceCompactor = this;
 		localData.env = env;

runtime/oti/VMHelpers.hpp

@@ -41,6 +41,7 @@
 #include "j9modifiers_api.h"
 #include "j9cp.h"
 #include "ute.h"
+#include "AtomicSupport.hpp"
 #include "ObjectAllocationAPI.hpp"
 
 typedef enum {
@@ -2048,18 +2049,19 @@ class VM_VMHelpers
 #endif /* JAVA_SPEC_VERSION > 11 */
 	}
 
-	static VMINLINE UDATA
-	walkContinuationStackFramesWrapper(J9VMThread *vmThread, j9object_t continuationObject, J9StackWalkState *walkState)
-	{
-		UDATA rc = J9_STACKWALK_RC_NONE;
 #if JAVA_SPEC_VERSION >= 19
-		J9VMContinuation *continuation = J9VMJDKINTERNALVMCONTINUATION_VMREF(vmThread, continuationObject);
-		rc = vmThread->javaVM->internalVMFunctions->walkContinuationStackFrames(vmThread, continuation, walkState);
-#endif /* JAVA_SPEC_VERSION >= 19 */
-		return rc;
+	static VMINLINE J9VMThread *
+	getCarrierThreadFromContinuationState(uintptr_t continuationState)
+	{
+		return (J9VMThread *)(continuationState & (~(uintptr_t)J9_GC_CONTINUATION_STATE_CONCURRENT_SCAN));
+	}
+
+	static VMINLINE bool
+	isConcurrentlyScannedFromContinuationState(uintptr_t continuationState)
+	{
+		return J9_ARE_ANY_BITS_SET(continuationState, J9_GC_CONTINUATION_STATE_CONCURRENT_SCAN);
 	}
 
-#if JAVA_SPEC_VERSION >= 19
 	/**
 	 * Check if the related J9VMContinuation is mounted to carrier thread
 	 * @param[in] continuation the related J9VMContinuation
@@ -2068,25 +2070,89 @@ class VM_VMHelpers
 	static VMINLINE bool
 	isContinuationMounted(J9VMContinuation *continuation)
 	{
-		return (NULL != continuation->carrierThread);
+		return J9_ARE_ANY_BITS_SET(continuation->state, ~(uintptr_t)J9_GC_CONTINUATION_STATE_CONCURRENT_SCAN);
+	}
+
+	static VMINLINE bool
+	isContinuationMountedOrConcurrentlyScanned(J9VMContinuation *continuation)
+	{
+		return isContinuationMounted(continuation) || isConcurrentlyScannedFromContinuationState(continuation->state);
 	}
+
+/*
+ *	If low tagging failed due to either
+ *
+ *   a carrier thread winning to mount, we don't need to do anything, since it will be compensated by pre/post mount actions
+ *   another GC thread winning to scan, again don't do anything, and let the winning thread do the work, instead
+ */
+	static VMINLINE bool
+	tryWinningConcurrentGCScan(J9VMContinuation *continuation)
+	{
+		return J9_GC_CONTINUATION_STATE_INITIAL == VM_AtomicSupport::lockCompareExchange(&continuation->state, J9_GC_CONTINUATION_STATE_INITIAL, J9_GC_CONTINUATION_STATE_CONCURRENT_SCAN);
+	}
+
+	static VMINLINE void
+	exitConcurrentGCScan(J9VMContinuation *continuation)
+	{
+		/* clear CONCURRENTSCANNING flag */
+		uintptr_t oldContinuationState = VM_AtomicSupport::bitAnd(&continuation->state, ~(uintptr_t)J9_GC_CONTINUATION_STATE_CONCURRENT_SCAN);
+		J9VMThread *carrierThread = getCarrierThreadFromContinuationState(oldContinuationState);
+		if (NULL != carrierThread) {
+			omrthread_monitor_enter(carrierThread->publicFlagsMutex);
+			/*  notify the waiting carrierThread that we just finished scanning, and it can proceed with mounting. */
+			omrthread_monitor_notify_all(carrierThread->publicFlagsMutex);
+			omrthread_monitor_exit(carrierThread->publicFlagsMutex);
+		}
+	}
+#endif /* JAVA_SPEC_VERSION >= 19 */
+
+	static VMINLINE UDATA
+	walkContinuationStackFramesWrapper(J9VMThread *vmThread, j9object_t continuationObject, J9StackWalkState *walkState, bool syncWithContinuationMounting)
+	{
+		UDATA rc = J9_STACKWALK_RC_NONE;
+#if JAVA_SPEC_VERSION >= 19
+		J9VMContinuation *continuation = J9VMJDKINTERNALVMCONTINUATION_VMREF(vmThread, continuationObject);
+		if (syncWithContinuationMounting && (NULL != continuation)) {
+			if (!tryWinningConcurrentGCScan(continuation)) {
+				/* If continuation is mounted or already being scanned by another GC thread, we do nothing */
+				return rc;
+			}
+		}
+		rc = vmThread->javaVM->internalVMFunctions->walkContinuationStackFrames(vmThread, continuation, walkState);
+		if (syncWithContinuationMounting && (NULL != continuation)) {
+			exitConcurrentGCScan(continuation);
+		}
 #endif /* JAVA_SPEC_VERSION >= 19 */
+		return rc;
+	}
 
 	/**
 	 * Check if we need to scan the java stack for the Continuation Object
+	 * Used during main scan phase of GC (object graph traversal) or heap object iteration (in sliding compact).
+	 * Not meant to be used during root scanning (neither strong roots nor weak roots)!
 	 * @param[in] vmThread the current J9VMThread
 	 * @param[in] continuationObject the continuation object
 	 * @param[in] scanOnlyUnmounted if it is true, only scan unmounted continuation object, default is false
 	 * @return true if we need to scan the java stack
 	 */
 	static VMINLINE bool
-	needScanStacksForContinuation(J9VMThread *vmThread, j9object_t continuationObject, bool scanOnlyUnmounted = false)
+	needScanStacksForContinuation(J9VMThread *vmThread, j9object_t continuationObject)
 	{
 		bool needScan = false;
 #if JAVA_SPEC_VERSION >= 19
 		jboolean started = J9VMJDKINTERNALVMCONTINUATION_STARTED(vmThread, continuationObject);
 		J9VMContinuation *continuation = J9VMJDKINTERNALVMCONTINUATION_VMREF(vmThread, continuationObject);
-		needScan = started && (NULL != continuation) && (!scanOnlyUnmounted || !isContinuationMounted(continuation));
+		/**
+		 * We don't scan mounted continuations:
+		 *
+		 * for concurrent GCs, since stack is actively changing. Instead, we scan them during preMount or during root scanning if already mounted at cycle start or during postUnmount (might be indirectly via card cleaning) or during final STW (via root re-scan) if still mounted at cycle end
+		 * for sliding compacts to avoid double slot fixups
+		 *
+		 * For fully STW GCs, there is no harm to scan them, but it's a waste of time since they are scanned during root scanning already.
+		 *
+		 * We don't scan currently scanned either - one scan is enough.
+		 */
+		needScan = started && (NULL != continuation) && (!isContinuationMountedOrConcurrentlyScanned(continuation));
 #endif /* JAVA_SPEC_VERSION >= 19 */
 		return needScan;
 	}

runtime/oti/j9consts.h

@@ -491,6 +491,8 @@ extern "C" {
 #define J9_GC_MARK_MAP_LOG_SIZEOF_UDATA 0x5
 #define J9_GC_MARK_MAP_UDATA_MASK 0x1F
 #endif /* J9VM_ENV_DATA64 */
+#define J9_GC_CONTINUATION_STATE_INITIAL 0
+#define J9_GC_CONTINUATION_STATE_CONCURRENT_SCAN 0x1
 
 #define J9VMGC_SIZECLASSES_MIN 0x1
 #define J9VMGC_SIZECLASSES_MIN_SMALL 0x1

runtime/oti/j9nonbuilder.h

@@ -5003,7 +5003,7 @@ typedef struct J9VMContinuation {
 	struct J9JITGPRSpillArea jitGPRs;
 	struct J9I2JState i2jState;
 	struct J9VMEntryLocalStorage* oldEntryLocalStorage;
-	struct J9VMThread* carrierThread;
+	volatile UDATA state; /* it's a bit-wise struct of CarrierThread ID and ConcurrentlyScanned flag */
 } J9VMContinuation;
 #endif /* JAVA_SPEC_VERSION >= 19 */
 

runtime/vm/ContinuationHelpers.cpp

@@ -88,19 +88,44 @@ createContinuation(J9VMThread *currentThread, j9object_t continuationObject)
 	return result;
 }
 
+void
+synchronizeWithConcurrentGCScan(J9VMThread *currentThread, J9VMContinuation *continuation)
+{
+	volatile uintptr_t *localAddr = &continuation->state;
+	/* atomically 'or' (not 'set') continuation->state  with currentThread */
+	uintptr_t oldContinuationState = VM_AtomicSupport::bitOr(localAddr, (uintptr_t)currentThread);
+
+	Assert_VM_Null(VM_VMHelpers::getCarrierThreadFromContinuationState(oldContinuationState));
+
+	if (VM_VMHelpers::isConcurrentlyScannedFromContinuationState(oldContinuationState)) {
+		/* currentThread was low tagged (GC was already in progress), but by 'or'-ing our ID, we let GC know there is a pending mount */
+		internalReleaseVMAccess(currentThread);
+
+		omrthread_monitor_enter(currentThread->publicFlagsMutex);
+		while (VM_VMHelpers::isConcurrentlyScannedFromContinuationState(*localAddr)) {
+			/* GC is still concurrently scanning the continuation(currentThread was still low tagged), wait for GC thread to notify us when it's done. */
+			omrthread_monitor_wait(currentThread->publicFlagsMutex);
+		}
+		omrthread_monitor_exit(currentThread->publicFlagsMutex);
+
+		internalAcquireVMAccess(currentThread);
+	}
+}
+
 BOOLEAN
 enterContinuation(J9VMThread *currentThread, j9object_t continuationObject)
 {
 	BOOLEAN result = TRUE;
 	jboolean started = J9VMJDKINTERNALVMCONTINUATION_STARTED(currentThread, continuationObject);
 	J9VMContinuation *continuation = J9VMJDKINTERNALVMCONTINUATION_VMREF(currentThread, continuationObject);
-
 	Assert_VM_Null(currentThread->currentContinuation);
+	Assert_VM_notNull(continuation);
 
+	/* let GC know we are mounting, so they don't need to scan us, or if there is already ongoing scan wait till it's complete. */
+	synchronizeWithConcurrentGCScan(currentThread, continuation);
 
 	VM_ContinuationHelpers::swapFieldsWithContinuation(currentThread, continuation, started);
 
-	continuation->carrierThread = currentThread;
 	currentThread->currentContinuation = continuation;
 
 	/* Reset counters which determine if the current continuation is pinned. */
@@ -141,19 +166,29 @@ yieldContinuation(J9VMThread *currentThread)
 {
 	BOOLEAN result = TRUE;
 	J9VMContinuation *continuation = currentThread->currentContinuation;
-
 	Assert_VM_notNull(currentThread->currentContinuation);
 
-	VM_ContinuationHelpers::swapFieldsWithContinuation(currentThread, continuation);
-	continuation->carrierThread = NULL;
 	currentThread->currentContinuation = NULL;
+	VM_ContinuationHelpers::swapFieldsWithContinuation(currentThread, continuation);
 
 	/* We need a full fence here to preserve happens-before relationship on PPC and other weakly
 	 * ordered architectures since learning/reservation is turned on by default. Since we have the
 	 * global pin lock counters we only need to need to address yield points, as thats the
 	 * only time a different virtualThread can run on the underlying j9vmthread.
 	 */
 	VM_AtomicSupport::readWriteBarrier();
+	/* we don't need atomic here, since no GC thread should be able to start scanning while continuation is mounted,
+	 * nor should another carrier thread be able to mount before we complete the unmount (hence no risk to overwrite anything in a race).
+	 * Order
+	 *
+	 *	swap-stacks
+	 *	writeBarrier
+	 *	state initial
+	 *
+	 * must be maintained for weakly ordered CPUs, to unsure that once the continuation is again available for GC scan (on potentially remote CPUs), all CPUs see up-to-date stack .
+	 */
+	Assert_VM_true((uintptr_t)currentThread == continuation->state);
+	continuation->state = J9_GC_CONTINUATION_STATE_INITIAL;
 
 	return result;
 }