Break the initialization cycle in NIO/cgroups code

First, we use a separate accessor for page-alignedness as it doesn't
need the more sophisticated initialization of the directMemory field.

Next, ensure PhysicalMemory initialization is serialized and when it is,
set directMemory to a static value so that the container code can finish
initialization without introducing a cyle. The final directMemory value
based on the heap size is then published to JDK code by setting the VM
init level to 1. Therefore, application code would use the non-static
value as the upper bound.

Closes: oracle#556

substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/heap/PhysicalMemory.java

@@ -24,6 +24,9 @@
  */
 package com.oracle.svm.core.heap;
 
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.TimeUnit;
+
 import org.graalvm.nativeimage.ImageSingletons;
 import org.graalvm.word.UnsignedWord;
 import org.graalvm.word.WordFactory;
@@ -54,12 +57,21 @@ default boolean hasSize() {
         UnsignedWord size();
     }
 
+    private static final CountDownLatch CACHED_SIZE_AVAIL_LATCH = new CountDownLatch(1);
     private static final AtomicInteger INITIALIZING = new AtomicInteger(0);
     private static final UnsignedWord UNSET_SENTINEL = UnsignedUtils.MAX_VALUE;
     private static UnsignedWord cachedSize = UNSET_SENTINEL;
 
     public static boolean isInitialized() {
-        return cachedSize != UNSET_SENTINEL;
+        return INITIALIZING.get() > 1;
+    }
+
+    /**
+     *
+     * @return {@code true} when PhycialMemory.size() is still initializing
+     */
+    private static boolean isInitializing() {
+        return INITIALIZING.get() == 1;
     }
 
     /**
@@ -78,24 +90,42 @@ public static UnsignedWord size() {
             throw VMError.shouldNotReachHere("Accessing the physical memory size may require allocation and synchronization");
         }
 
-        if (!isInitialized()) {
-            /*
-             * Multiple threads can race to initialize the cache. This is OK because all of them
-             * will (most-likely) compute the same value.
-             */
-            INITIALIZING.incrementAndGet();
-            try {
-                long memoryLimit = SubstrateOptions.MaxRAM.getValue();
-                if (memoryLimit > 0) {
-                    cachedSize = WordFactory.unsigned(memoryLimit);
-                } else {
-                    memoryLimit = Containers.memoryLimitInBytes();
-                    cachedSize = memoryLimit > 0
-                                    ? WordFactory.unsigned(memoryLimit)
-                                    : ImageSingletons.lookup(PhysicalMemorySupport.class).size();
+        synchronized (INITIALIZING) {
+            if (!isInitialized()) {
+                VMError.guarantee(INITIALIZING.get() <= 1, "Must not initialize twice");
+                if (isInitializing()) {
+                    /*
+                     * Recursive initializations need to wait for the one initializing thread to
+                     * finish so as to get correct reads of the cachedSize value.
+                     */
+                    try {
+                        boolean expired = !CACHED_SIZE_AVAIL_LATCH.await(1L, TimeUnit.SECONDS);
+                        if (expired) {
+                            throw new InternalError("expired latch!");
+                        }
+                        VMError.guarantee(cachedSize != UNSET_SENTINEL, "Expected chached size to be set");
+                        return cachedSize;
+                    } catch (InterruptedException e) {
+                        throw VMError.shouldNotReachHere("Interrupt on countdown latch!");
+                    }
+                }
+                INITIALIZING.incrementAndGet();
+                try {
+                    long memoryLimit = SubstrateOptions.MaxRAM.getValue();
+                    if (memoryLimit > 0) {
+                        cachedSize = WordFactory.unsigned(memoryLimit);
+                    } else {
+                        memoryLimit = Containers.memoryLimitInBytes();
+                        cachedSize = memoryLimit > 0
+                                        ? WordFactory.unsigned(memoryLimit)
+                                        : ImageSingletons.lookup(PhysicalMemorySupport.class).size();
+                    }
+                    // Now that we have set the cachedSize let other threads know it's
+                    // available to use.
+                    CACHED_SIZE_AVAIL_LATCH.countDown();
+                } finally {
+                    INITIALIZING.incrementAndGet();
                 }
-            } finally {
-                INITIALIZING.decrementAndGet();
             }
         }
 

substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/jdk/Target_jdk_internal_misc_VM.java

@@ -25,6 +25,7 @@
 package com.oracle.svm.core.jdk;
 
 import java.util.Map;
+import java.util.concurrent.atomic.AtomicInteger;
 
 import com.oracle.svm.core.NeverInline;
 import com.oracle.svm.core.SubstrateOptions;
@@ -35,7 +36,9 @@
 import com.oracle.svm.core.annotate.RecomputeFieldValue.Kind;
 import com.oracle.svm.core.annotate.Substitute;
 import com.oracle.svm.core.annotate.TargetClass;
+import com.oracle.svm.core.heap.PhysicalMemory;
 import com.oracle.svm.core.snippets.KnownIntrinsics;
+import com.oracle.svm.core.util.VMError;
 
 import jdk.internal.misc.Unsafe;
 
@@ -67,36 +70,44 @@ public static ClassLoader latestUserDefinedLoader0() {
 
     @Alias @InjectAccessors(DirectMemoryAccessors.class) //
     private static long directMemory;
-    @Alias @InjectAccessors(DirectMemoryAccessors.class) //
+    @Alias @InjectAccessors(PageAlignDirectMemoryAccessors.class) //
     private static boolean pageAlignDirectMemory;
+
+    @Alias //
+    @SuppressWarnings("unused")
+    public static void initLevel(int newVal) {
+        throw VMError.shouldNotReachHere("This is an alias to the method in the target class, so this code is unreachable");
+    }
 }
 
 final class DirectMemoryAccessors {
 
     /*
-     * Not volatile to avoid a memory barrier when reading the values. Instead, an explicit barrier
-     * is inserted when writing the values.
+     * Full initialization is two-staged. First, we init directMemory to a static value (25MB) so
+     * that initialization of PhysicalMemory has a chance to finish. At that point isInintialized
+     * will be false, since we need to (potentially) set the value to the actual configured heap
+     * size. That can only be done once PhysicalMemory init completed. We'd introduce a cycle
+     * otherwise.
      */
-    private static boolean initialized;
-
+    private static volatile boolean isInitialized;
+    private static final AtomicInteger INIT_COUNT = new AtomicInteger();
+    private static final long STATIC_DIRECT_MEMORY_AMOUNT = 25 * 1024 * 1024;
     private static long directMemory;
-    private static boolean pageAlignDirectMemory;
 
     static long getDirectMemory() {
-        if (!initialized) {
+        if (!isInitialized) {
             initialize();
         }
         return directMemory;
     }
 
-    static boolean getPageAlignDirectMemory() {
-        if (!initialized) {
-            initialize();
+    private static synchronized void initialize() {
+        if (INIT_COUNT.get() == 2) {
+            /*
+             * Shouldn't really happen, but safeguard for recursive init anyway
+             */
+            return;
         }
-        return pageAlignDirectMemory;
-    }
-
-    private static void initialize() {
         /*
          * The JDK method VM.saveAndRemoveProperties looks at the system property
          * "sun.nio.MaxDirectMemorySize". However, that property is always set by the Java HotSpot
@@ -107,17 +118,65 @@ private static void initialize() {
         if (newDirectMemory == 0) {
             /*
              * No value explicitly specified. The default in the JDK in this case is the maximum
-             * heap size.
+             * heap size. However, we cannot rely on Runtime.maxMemory() until PhysicalMemory has
+             * fully initialized. Runtime.maxMemory() has a dependency on PhysicalMemory.size()
+             * which in turn depends on container support which might use NIO. To avoid this cycle,
+             * we first initialize the 'directMemory' field to an arbitrary value (25MB), and only
+             * use the Runtime.maxMemory() API once PhysicalMemory has fully initialized.
              */
-            newDirectMemory = Runtime.getRuntime().maxMemory();
+            if (!PhysicalMemory.isInitialized()) {
+                /*
+                 * While initializing physical memory we might end up back here with an INIT_COUNT
+                 * of 1, since we read the directMemory field during container support code
+                 * execution which runs when PhysicalMemory is still initializing.
+                 */
+                VMError.guarantee(INIT_COUNT.get() <= 1, "Initial run needs to have init count 0 or 1");
+                newDirectMemory = STATIC_DIRECT_MEMORY_AMOUNT; // Static value during initialization
+                INIT_COUNT.incrementAndGet();
+            } else {
+                VMError.guarantee(INIT_COUNT.get() == 1, "Second cycle needs to have init count 1");
+                /*
+                 * Once we know PhysicalMemory has been properly initialized we can use
+                 * Runtime.maxMemory()
+                 */
+                INIT_COUNT.incrementAndGet();
+                newDirectMemory = Runtime.getRuntime().maxMemory();
+            }
         }
+        VMError.guarantee(newDirectMemory > 0, "New direct memory should be initialized");
 
-        /*
-         * The initialization is not synchronized, so multiple threads can race. Usually this will
-         * lead to the same value, unless the runtime options are modified concurrently - which is
-         * possible but not a case we care about.
-         */
+        Unsafe.getUnsafe().storeFence();
         directMemory = newDirectMemory;
+        if (PhysicalMemory.isInitialized()) {
+            /*
+             * Complete initialization hand-shake once PhysicalMemory is properly initialized. Also
+             * set the VM init level to 1 so as to provoke the NIO code to re-set the internal
+             * MAX_MEMORY field.
+             */
+            isInitialized = true;
+            Target_jdk_internal_misc_VM.initLevel(1);
+        }
+    }
+}
+
+final class PageAlignDirectMemoryAccessors {
+
+    /*
+     * Not volatile to avoid a memory barrier when reading the values. Instead, an explicit barrier
+     * is inserted when writing the values.
+     */
+    private static boolean initialized;
+
+    private static boolean pageAlignDirectMemory;
+
+    static boolean getPageAlignDirectMemory() {
+        if (!initialized) {
+            initialize();
+        }
+        return pageAlignDirectMemory;
+    }
+
+    private static void initialize() {
         pageAlignDirectMemory = Boolean.getBoolean("sun.nio.PageAlignDirectMemory");
 
         /* Ensure values are published to other threads before marking fields as initialized. */