Fix deadlock and race condition in AbstractCollectionPolicy.updateSizeParameters().

Author: christianhaeubl

substratevm/mx.substratevm/suite.py

@@ -400,6 +400,7 @@
             ],
             "requiresConcealed" : {
                 "java.base": [
+                    "jdk.internal.misc",
                     "sun.nio.ch",
                 ],
                 "java.management": [

substratevm/src/com.oracle.svm.core.genscavenge/src/com/oracle/svm/core/genscavenge/AbstractCollectionPolicy.java

@@ -24,9 +24,6 @@
  */
 package com.oracle.svm.core.genscavenge;
 
-import java.util.concurrent.atomic.AtomicBoolean;
-
-import jdk.graal.compiler.word.Word;
 import org.graalvm.nativeimage.Platform;
 import org.graalvm.nativeimage.Platforms;
 import org.graalvm.word.UnsignedWord;
@@ -36,12 +33,14 @@
 import com.oracle.svm.core.heap.PhysicalMemory;
 import com.oracle.svm.core.heap.ReferenceAccess;
 import com.oracle.svm.core.jdk.UninterruptibleUtils;
+import com.oracle.svm.core.thread.JavaSpinLockUtils;
 import com.oracle.svm.core.thread.VMOperation;
 import com.oracle.svm.core.util.UnsignedUtils;
 import com.oracle.svm.core.util.VMError;
 
 import jdk.graal.compiler.api.replacements.Fold;
-import jdk.graal.compiler.nodes.PauseNode;
+import jdk.graal.compiler.word.Word;
+import jdk.internal.misc.Unsafe;
 
 abstract class AbstractCollectionPolicy implements CollectionPolicy {
 
@@ -54,6 +53,9 @@ static int getMaxSurvivorSpaces(Integer userValue) {
         return (userValue != null) ? userValue : AbstractCollectionPolicy.MAX_TENURING_THRESHOLD;
     }
 
+    private static final Unsafe U = Unsafe.getUnsafe();
+    private static final long SIZES_UPDATE_LOCK_OFFSET = U.objectFieldOffset(AbstractCollectionPolicy.class, "sizesUpdateLock");
+
     /*
      * Constants that can be made options if desirable. These are -XX options in HotSpot, refer to
      * their descriptions for details. The values are HotSpot defaults unless labeled otherwise.
@@ -78,8 +80,8 @@ static int getMaxSurvivorSpaces(Integer userValue) {
     protected UnsignedWord oldSize;
     protected int tenuringThreshold;
 
-    protected volatile SizeParameters sizes;
-    private final AtomicBoolean sizesUpdateSpinLock = new AtomicBoolean();
+    protected volatile SizeParameters sizes = null;
+    @SuppressWarnings("unused") private volatile int sizesUpdateLock;
 
     protected AbstractCollectionPolicy(int initialNewRatio, int initialTenuringThreshold) {
         this.initialNewRatio = initialNewRatio;
@@ -145,40 +147,49 @@ protected void guaranteeSizeParametersInitialized() {
 
     @Override
     public void updateSizeParameters() {
-        SizeParameters params = computeSizeParameters(sizes);
-        SizeParameters previous = sizes;
-        if (previous != null && params.equal(previous)) {
+        /*
+         * Read the old object before computing the new values. Otherwise, we risk reusing an
+         * outdated SizeParameters object.
+         */
+        SizeParameters prevParams = sizes;
+        SizeParameters newParams = computeSizeParameters(prevParams);
+        if (prevParams != null && newParams.equal(prevParams)) {
             return; // nothing to do
         }
-        while (!sizesUpdateSpinLock.compareAndSet(false, true)) {
-            /*
-             * We use a primitive spin lock because at this point, the current thread might be
-             * unable to use a Java lock (e.g. no Thread object yet), and the critical section is
-             * short, so we do not want to suspend and wake up threads for it.
-             */
-            PauseNode.pause();
-        }
+        updateSizeParameters0(newParams, prevParams);
+        guaranteeSizeParametersInitialized(); // sanity
+    }
+
+    @Uninterruptible(reason = "Holding the spin lock at a safepoint can result in deadlocks.")
+    private void updateSizeParameters0(SizeParameters newParams, SizeParameters prevParams) {
+        /*
+         * We use a primitive spin lock because at this point, the current thread might be unable to
+         * use a Java lock (e.g. no Thread object yet), and the critical section is short, so we do
+         * not want to suspend and wake up threads for it.
+         */
+        JavaSpinLockUtils.lockNoTransition(this, SIZES_UPDATE_LOCK_OFFSET);
         try {
-            updateSizeParametersLocked(params, previous);
+            if (sizes != prevParams) {
+                /*
+                 * Some other thread beat us and we cannot tell if our values or their values are
+                 * newer, so back off - any newer values will be applied eventually.
+                 */
+                return;
+            }
+            updateSizeParametersLocked(newParams, prevParams);
         } finally {
-            sizesUpdateSpinLock.set(false);
+            JavaSpinLockUtils.unlock(this, SIZES_UPDATE_LOCK_OFFSET);
         }
-        guaranteeSizeParametersInitialized(); // sanity
     }
 
-    @Uninterruptible(reason = "Must be atomic with regard to garbage collection.")
-    private void updateSizeParametersLocked(SizeParameters params, SizeParameters previous) {
-        if (sizes != previous) {
-            // Some other thread beat us and we cannot tell if our values or their values are newer,
-            // so back off -- any newer values will be applied eventually.
-            return;
-        }
-        sizes = params;
+    @Uninterruptible(reason = "Holding the spin lock at a safepoint can result in deadlocks. Updating the size parameters must be atomic with regard to garbage collection.")
+    private void updateSizeParametersLocked(SizeParameters newParams, SizeParameters prevParams) {
+        sizes = newParams;
 
-        if (previous == null || gcCount() == 0) {
-            survivorSize = params.initialSurvivorSize;
-            edenSize = params.initialEdenSize;
-            oldSize = params.initialOldSize();
+        if (prevParams == null || gcCount() == 0) {
+            survivorSize = newParams.initialSurvivorSize;
+            edenSize = newParams.initialEdenSize;
+            oldSize = newParams.initialOldSize();
             promoSize = UnsignedUtils.min(edenSize, oldSize);
         }
 
@@ -191,10 +202,10 @@ private void updateSizeParametersLocked(SizeParameters params, SizeParameters pr
          * We assume that such changes happen very early on and values then adapt reasonably quick,
          * but we must still ensure that computations can handle it (for example, no overflows).
          */
-        survivorSize = UnsignedUtils.min(survivorSize, params.maxSurvivorSize());
+        survivorSize = UnsignedUtils.min(survivorSize, newParams.maxSurvivorSize());
         edenSize = UnsignedUtils.min(edenSize, getMaximumEdenSize());
-        oldSize = UnsignedUtils.min(oldSize, params.maxOldSize());
-        promoSize = UnsignedUtils.min(promoSize, params.maxOldSize());
+        oldSize = UnsignedUtils.min(oldSize, newParams.maxOldSize());
+        promoSize = UnsignedUtils.min(promoSize, newParams.maxOldSize());
     }
 
     @Override

substratevm/src/com.oracle.svm.core.genscavenge/src/com/oracle/svm/core/genscavenge/BasicCollectionPolicies.java

@@ -26,7 +26,6 @@
 
 import static com.oracle.svm.core.genscavenge.CollectionPolicy.shouldCollectYoungGenSeparately;
 
-import jdk.graal.compiler.word.Word;
 import org.graalvm.nativeimage.Platform;
 import org.graalvm.nativeimage.Platforms;
 import org.graalvm.word.UnsignedWord;
@@ -40,6 +39,8 @@
 import com.oracle.svm.core.util.UnsignedUtils;
 import com.oracle.svm.core.util.VMError;
 
+import jdk.graal.compiler.word.Word;
+
 /** Basic/legacy garbage collection policies. */
 final class BasicCollectionPolicies {
     @Platforms(Platform.HOSTED_ONLY.class)