From 1ae2dc011c6c74f25ef49e5cc5bd50d814545fff Mon Sep 17 00:00:00 2001 From: Severin Gehwolf Date: Wed, 30 Aug 2023 16:54:24 +0200 Subject: [PATCH 1/2] 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: #556 --- .../oracle/svm/core/heap/PhysicalMemory.java | 51 ++++++-- .../core/jdk/Target_jdk_internal_misc_VM.java | 118 ++++++++++++++---- 2 files changed, 138 insertions(+), 31 deletions(-) diff --git a/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/heap/PhysicalMemory.java b/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/heap/PhysicalMemory.java index 4fe58e816ae6..fc557390a017 100644 --- a/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/heap/PhysicalMemory.java +++ b/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/heap/PhysicalMemory.java @@ -24,6 +24,10 @@ */ package com.oracle.svm.core.heap; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.locks.ReentrantLock; + import org.graalvm.nativeimage.ImageSingletons; import org.graalvm.word.UnsignedWord; import org.graalvm.word.WordFactory; @@ -55,13 +59,23 @@ 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 ReentrantLock LOCK = new ReentrantLock(); private static final UnsignedWord UNSET_SENTINEL = UnsignedUtils.MAX_VALUE; private static UnsignedWord cachedSize = UNSET_SENTINEL; @Uninterruptible(reason = "Called from uninterruptible code.", mayBeInlined = true) 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; } /** @@ -80,13 +94,26 @@ 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 { + LOCK.lock(); + try { + if (!isInitialized()) { + 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(); long memoryLimit = SubstrateOptions.MaxRAM.getValue(); if (memoryLimit > 0) { cachedSize = WordFactory.unsigned(memoryLimit); @@ -96,9 +123,13 @@ public static UnsignedWord size() { ? WordFactory.unsigned(memoryLimit) : ImageSingletons.lookup(PhysicalMemorySupport.class).size(); } - } finally { - INITIALIZING.decrementAndGet(); + // Now that we have set the cachedSize let other threads know it's + // available to use. + INITIALIZING.incrementAndGet(); + CACHED_SIZE_AVAIL_LATCH.countDown(); } + } finally { + LOCK.unlock(); } return cachedSize; diff --git a/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/jdk/Target_jdk_internal_misc_VM.java b/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/jdk/Target_jdk_internal_misc_VM.java index a86d95a6755f..c14c745afed4 100644 --- a/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/jdk/Target_jdk_internal_misc_VM.java +++ b/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 // + public static native void initLevel(int newVal); + + @Alias // + public static native int initLevel(); } 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 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(); - } - return pageAlignDirectMemory; - } - private static void initialize() { + if (INIT_COUNT.get() == 2) { + /* + * Shouldn't really happen, but safeguard for recursive init anyway + */ + return; + } /* * 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,82 @@ 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. + */ + 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, "Runtime.maxMemory() invariant"); + /* + * Once we know PhysicalMemory has been properly initialized we can use + * Runtime.maxMemory(). Note that we might end up in this branch for code explicitly + * using the JDK cgroups code. At that point PhysicalMemory has likely been + * initialized. + */ + INIT_COUNT.incrementAndGet(); + newDirectMemory = Runtime.getRuntime().maxMemory(); + } + } else { + /* + * For explicitly set direct memory we are done */ - newDirectMemory = Runtime.getRuntime().maxMemory(); + Unsafe.getUnsafe().storeFence(); + directMemory = newDirectMemory; + isInitialized = true; + if (Target_jdk_internal_misc_VM.initLevel() < 1) { + // only the first accessor needs to set this + Target_jdk_internal_misc_VM.initLevel(1); + } + return; } + 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; + if (Target_jdk_internal_misc_VM.initLevel() < 1) { + // only the first accessor needs to set this + 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. */ From 318f1083a75735a19f162f586ff2f815a771b1b1 Mon Sep 17 00:00:00 2001 From: Severin Gehwolf Date: Wed, 4 Oct 2023 18:37:11 +0200 Subject: [PATCH 2/2] Fix race with double init --- .../com/oracle/svm/core/heap/PhysicalMemory.java | 2 +- .../core/jdk/Target_jdk_internal_misc_VM.java | 16 +++++++++------- 2 files changed, 10 insertions(+), 8 deletions(-) diff --git a/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/heap/PhysicalMemory.java b/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/heap/PhysicalMemory.java index fc557390a017..8b4663ab0ef1 100644 --- a/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/heap/PhysicalMemory.java +++ b/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/heap/PhysicalMemory.java @@ -107,7 +107,7 @@ public static UnsignedWord size() { if (expired) { throw new InternalError("Expired latch!"); } - VMError.guarantee(cachedSize != UNSET_SENTINEL, "Expected chached size to be set"); + VMError.guarantee(cachedSize != UNSET_SENTINEL, "Expected cached size to be set"); return cachedSize; } catch (InterruptedException e) { throw VMError.shouldNotReachHere("Interrupt on countdown latch!"); diff --git a/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/jdk/Target_jdk_internal_misc_VM.java b/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/jdk/Target_jdk_internal_misc_VM.java index c14c745afed4..058880077c5c 100644 --- a/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/jdk/Target_jdk_internal_misc_VM.java +++ b/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/jdk/Target_jdk_internal_misc_VM.java @@ -90,6 +90,8 @@ final class DirectMemoryAccessors { * otherwise. */ private static boolean isInitialized; + private static final int INITIALIZING = 1; + private static final int INITIALIZED = 2; private static final AtomicInteger INIT_COUNT = new AtomicInteger(); private static final long STATIC_DIRECT_MEMORY_AMOUNT = 25 * 1024 * 1024; private static long directMemory; @@ -102,9 +104,9 @@ static long getDirectMemory() { } private static void initialize() { - if (INIT_COUNT.get() == 2) { + if (INIT_COUNT.get() == INITIALIZED) { /* - * Shouldn't really happen, but safeguard for recursive init anyway + * Safeguard for recursive init */ return; } @@ -130,18 +132,18 @@ private static void initialize() { * 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"); + VMError.guarantee(INIT_COUNT.get() <= INITIALIZING, "Initial run needs to have init count 0 or 1"); newDirectMemory = STATIC_DIRECT_MEMORY_AMOUNT; // Static value during initialization - INIT_COUNT.incrementAndGet(); + INIT_COUNT.setRelease(INITIALIZING); } else { - VMError.guarantee(INIT_COUNT.get() <= 1, "Runtime.maxMemory() invariant"); + VMError.guarantee(INIT_COUNT.get() <= INITIALIZING, "Runtime.maxMemory() invariant"); /* * Once we know PhysicalMemory has been properly initialized we can use * Runtime.maxMemory(). Note that we might end up in this branch for code explicitly * using the JDK cgroups code. At that point PhysicalMemory has likely been * initialized. */ - INIT_COUNT.incrementAndGet(); + INIT_COUNT.setRelease(INITIALIZED); newDirectMemory = Runtime.getRuntime().maxMemory(); } } else { @@ -161,7 +163,7 @@ private static void initialize() { Unsafe.getUnsafe().storeFence(); directMemory = newDirectMemory; - if (PhysicalMemory.isInitialized()) { + if (PhysicalMemory.isInitialized() && INITIALIZED == INIT_COUNT.get()) { /* * 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