8218880: G1 crashes when issuing a periodic GC while the GCLocker is held
authortschatzl
Mon, 04 Mar 2019 11:49:16 +0100
changeset 53983 7935353a466a
parent 53982 05d35241e1e9
child 53984 31884d19e945
8218880: G1 crashes when issuing a periodic GC while the GCLocker is held Summary: Do not wait for the GC locker in the periodic GC thread as the GC locker being held is an indication for being busy anyway. Reviewed-by: kbarrett, shade
src/hotspot/share/gc/g1/g1CollectedHeap.cpp
src/hotspot/share/gc/g1/g1CollectedHeap.hpp
src/hotspot/share/gc/g1/g1VMOperations.cpp
src/hotspot/share/gc/g1/g1VMOperations.hpp
src/hotspot/share/gc/g1/g1YoungRemSetSamplingThread.cpp
test/hotspot/jtreg/gc/g1/TestPeriodicCollectionJNI.java
test/hotspot/jtreg/gc/g1/libTestPeriodicCollectionJNI.c
--- a/src/hotspot/share/gc/g1/g1CollectedHeap.cpp	Mon Mar 04 10:19:35 2019 +0100
+++ b/src/hotspot/share/gc/g1/g1CollectedHeap.cpp	Mon Mar 04 11:49:16 2019 +0100
@@ -2113,15 +2113,21 @@
 }
 
 void G1CollectedHeap::collect(GCCause::Cause cause) {
+  try_collect(cause, true);
+}
+
+bool G1CollectedHeap::try_collect(GCCause::Cause cause, bool retry_on_gc_failure) {
   assert_heap_not_locked();
 
-  uint gc_count_before;
-  uint old_marking_count_before;
-  uint full_gc_count_before;
-  bool retry_gc;
+  bool gc_succeeded;
+  bool should_retry_gc;
 
   do {
-    retry_gc = false;
+    should_retry_gc = false;
+
+    uint gc_count_before;
+    uint old_marking_count_before;
+    uint full_gc_count_before;
 
     {
       MutexLocker ml(Heap_lock);
@@ -2142,19 +2148,18 @@
                                    true,  /* should_initiate_conc_mark */
                                    g1_policy()->max_pause_time_ms());
       VMThread::execute(&op);
-      if (!op.pause_succeeded()) {
+      gc_succeeded = op.gc_succeeded();
+      if (!gc_succeeded && retry_on_gc_failure) {
         if (old_marking_count_before == _old_marking_cycles_started) {
-          retry_gc = op.should_retry_gc();
+          should_retry_gc = op.should_retry_gc();
         } else {
           // A Full GC happened while we were trying to schedule the
-          // initial-mark GC. No point in starting a new cycle given
+          // concurrent cycle. No point in starting a new cycle given
           // that the whole heap was collected anyway.
         }
 
-        if (retry_gc) {
-          if (GCLocker::is_active_and_needs_gc()) {
-            GCLocker::stall_until_clear();
-          }
+        if (should_retry_gc && GCLocker::is_active_and_needs_gc()) {
+          GCLocker::stall_until_clear();
         }
       }
     } else {
@@ -2169,13 +2174,16 @@
                                      false, /* should_initiate_conc_mark */
                                      g1_policy()->max_pause_time_ms());
         VMThread::execute(&op);
+        gc_succeeded = op.gc_succeeded();
       } else {
         // Schedule a Full GC.
         VM_G1CollectFull op(gc_count_before, full_gc_count_before, cause);
         VMThread::execute(&op);
+        gc_succeeded = op.gc_succeeded();
       }
     }
-  } while (retry_gc);
+  } while (should_retry_gc);
+  return gc_succeeded;
 }
 
 bool G1CollectedHeap::is_in(const void* p) const {
@@ -2586,7 +2594,7 @@
   VMThread::execute(&op);
 
   HeapWord* result = op.result();
-  bool ret_succeeded = op.prologue_succeeded() && op.pause_succeeded();
+  bool ret_succeeded = op.prologue_succeeded() && op.gc_succeeded();
   assert(result == NULL || ret_succeeded,
          "the result should be NULL if the VM did not succeed");
   *succeeded = ret_succeeded;
--- a/src/hotspot/share/gc/g1/g1CollectedHeap.hpp	Mon Mar 04 10:19:35 2019 +0100
+++ b/src/hotspot/share/gc/g1/g1CollectedHeap.hpp	Mon Mar 04 11:49:16 2019 +0100
@@ -1061,6 +1061,11 @@
   // "CollectedHeap" supports.
   virtual void collect(GCCause::Cause cause);
 
+  // Perform a collection of the heap with the given cause; if the VM operation
+  // fails to execute for any reason, retry only if retry_on_gc_failure is set.
+  // Returns whether this collection actually executed.
+  bool try_collect(GCCause::Cause cause, bool retry_on_gc_failure);
+
   // True iff an evacuation has failed in the most-recent collection.
   bool evacuation_failed() { return _evacuation_failed; }
 
--- a/src/hotspot/share/gc/g1/g1VMOperations.cpp	Mon Mar 04 10:19:35 2019 +0100
+++ b/src/hotspot/share/gc/g1/g1VMOperations.cpp	Mon Mar 04 11:49:16 2019 +0100
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2001, 2018, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2001, 2019, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -36,7 +36,7 @@
 void VM_G1CollectFull::doit() {
   G1CollectedHeap* g1h = G1CollectedHeap::heap();
   GCCauseSetter x(g1h, _gc_cause);
-  g1h->do_full_collection(false /* clear_all_soft_refs */);
+  _gc_succeeded = g1h->do_full_collection(true /* explicit_gc */, false /* clear_all_soft_refs */);
 }
 
 VM_G1CollectForAllocation::VM_G1CollectForAllocation(size_t         word_size,
@@ -45,7 +45,7 @@
                                                      bool           should_initiate_conc_mark,
                                                      double         target_pause_time_ms) :
   VM_CollectForAllocation(word_size, gc_count_before, gc_cause),
-  _pause_succeeded(false),
+  _gc_succeeded(false),
   _should_initiate_conc_mark(should_initiate_conc_mark),
   _should_retry_gc(false),
   _target_pause_time_ms(target_pause_time_ms),
@@ -85,7 +85,7 @@
     if (_result != NULL) {
       // If we can successfully allocate before we actually do the
       // pause then we will consider this pause successful.
-      _pause_succeeded = true;
+      _gc_succeeded = true;
       return;
     }
   }
@@ -130,13 +130,13 @@
   }
 
   // Try a partial collection of some kind.
-  _pause_succeeded = g1h->do_collection_pause_at_safepoint(_target_pause_time_ms);
+  _gc_succeeded = g1h->do_collection_pause_at_safepoint(_target_pause_time_ms);
 
-  if (_pause_succeeded) {
+  if (_gc_succeeded) {
     if (_word_size > 0) {
       // An allocation had been requested. Do it, eventually trying a stronger
       // kind of GC.
-      _result = g1h->satisfy_failed_allocation(_word_size, &_pause_succeeded);
+      _result = g1h->satisfy_failed_allocation(_word_size, &_gc_succeeded);
     } else {
       bool should_upgrade_to_full = g1h->should_upgrade_to_full_gc(_gc_cause);
 
@@ -145,11 +145,11 @@
         // information on how much memory has been asked for. In case there are
         // absolutely no regions left to allocate into, do a maximally compacting full GC.
         log_info(gc, ergo)("Attempting maximally compacting collection");
-        _pause_succeeded = g1h->do_full_collection(false, /* explicit gc */
+        _gc_succeeded = g1h->do_full_collection(false, /* explicit gc */
                                                    true   /* clear_all_soft_refs */);
       }
     }
-    guarantee(_pause_succeeded, "Elevated collections during the safepoint must always succeed.");
+    guarantee(_gc_succeeded, "Elevated collections during the safepoint must always succeed.");
   } else {
     assert(_result == NULL, "invariant");
     // The only reason for the pause to not be successful is that, the GC locker is
--- a/src/hotspot/share/gc/g1/g1VMOperations.hpp	Mon Mar 04 10:19:35 2019 +0100
+++ b/src/hotspot/share/gc/g1/g1VMOperations.hpp	Mon Mar 04 11:49:16 2019 +0100
@@ -35,22 +35,26 @@
 //   - VM_G1CollectFull
 
 class VM_G1CollectFull : public VM_GC_Operation {
+  bool _gc_succeeded;
+
 public:
   VM_G1CollectFull(uint gc_count_before,
                    uint full_gc_count_before,
                    GCCause::Cause cause) :
-    VM_GC_Operation(gc_count_before, cause, full_gc_count_before, true) { }
+    VM_GC_Operation(gc_count_before, cause, full_gc_count_before, true),
+    _gc_succeeded(false) { }
   virtual VMOp_Type type() const { return VMOp_G1CollectFull; }
   virtual void doit();
+  bool gc_succeeded() { return _gc_succeeded; }
 };
 
 class VM_G1CollectForAllocation : public VM_CollectForAllocation {
-  bool         _pause_succeeded;
+  bool _gc_succeeded;
 
-  bool         _should_initiate_conc_mark;
-  bool         _should_retry_gc;
-  double       _target_pause_time_ms;
-  uint         _old_marking_cycles_completed_before;
+  bool _should_initiate_conc_mark;
+  bool _should_retry_gc;
+  double _target_pause_time_ms;
+  uint  _old_marking_cycles_completed_before;
 
 public:
   VM_G1CollectForAllocation(size_t         word_size,
@@ -63,7 +67,7 @@
   virtual void doit();
   virtual void doit_epilogue();
   bool should_retry_gc() const { return _should_retry_gc; }
-  bool pause_succeeded() { return _pause_succeeded; }
+  bool gc_succeeded() { return _gc_succeeded; }
 };
 
 // Concurrent G1 stop-the-world operations such as remark and cleanup.
--- a/src/hotspot/share/gc/g1/g1YoungRemSetSamplingThread.cpp	Mon Mar 04 10:19:35 2019 +0100
+++ b/src/hotspot/share/gc/g1/g1YoungRemSetSamplingThread.cpp	Mon Mar 04 11:49:16 2019 +0100
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2015, 2018, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2015, 2019, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -89,7 +89,10 @@
   if ((os::elapsedTime() - _last_periodic_gc_attempt_s) > (G1PeriodicGCInterval / 1000.0)) {
     log_debug(gc, periodic)("Checking for periodic GC.");
     if (should_start_periodic_gc()) {
-      Universe::heap()->collect(GCCause::_g1_periodic_collection);
+      if (!G1CollectedHeap::heap()->try_collect(GCCause::_g1_periodic_collection,
+                                                    false /* retry_on_vmop_failure */)) {
+        log_debug(gc, periodic)("GC request denied. Skipping.");
+      }
     }
     _last_periodic_gc_attempt_s = os::elapsedTime();
   }
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/test/hotspot/jtreg/gc/g1/TestPeriodicCollectionJNI.java	Mon Mar 04 11:49:16 2019 +0100
@@ -0,0 +1,88 @@
+/*
+ * Copyright (c) 2019, Oracle and/or its affiliates. All rights reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+
+package gc.g1;
+
+/* @test
+ * @bug 8218880
+ * @summary Test that issuing a periodic collection while the GC locker is
+ * held does not crash the VM.
+ * @key gc
+ * @requires vm.gc.G1
+ * @modules java.base
+ * @run main/othervm/native
+ *    -Xbootclasspath/a:.
+ *    -XX:+UnlockDiagnosticVMOptions -XX:+WhiteBoxAPI
+ *    -XX:+UseG1GC -XX:G1PeriodicGCInterval=100
+ *    -XX:+G1PeriodicGCInvokesConcurrent
+ *    -Xlog:gc,gc+periodic=debug
+ *    gc.g1.TestPeriodicCollectionJNI
+ * @run main/othervm/native
+ *    -Xbootclasspath/a:.
+ *    -XX:+UnlockDiagnosticVMOptions -XX:+WhiteBoxAPI
+ *    -XX:+UseG1GC -XX:G1PeriodicGCInterval=100
+ *    -XX:-G1PeriodicGCInvokesConcurrent
+ *    -Xlog:gc,gc+periodic=debug
+ *    gc.g1.TestPeriodicCollectionJNI
+ */
+
+public class TestPeriodicCollectionJNI {
+    static { System.loadLibrary("TestPeriodicCollectionJNI"); }
+
+    private static native boolean blockInNative(byte[] array);
+    private static native void unblock();
+
+    public static void block() {
+        if (!blockInNative(new byte[0])) {
+            throw new RuntimeException("failed to acquire lock to dummy object");
+        }
+    }
+
+    public static void main(String[] args) throws InterruptedException {
+        long timeout = 2000;
+        long startTime = System.currentTimeMillis();
+
+        // Start thread doing JNI call
+        BlockInNative blocker = new BlockInNative();
+        blocker.start();
+
+        try {
+            // Wait for periodic GC timeout to trigger
+            while (System.currentTimeMillis() < startTime + timeout) {
+                System.out.println("Sleeping to let periodic GC trigger...");
+                Thread.sleep(200);
+            }
+        } finally {
+            unblock();
+        }
+    }
+}
+
+class BlockInNative extends Thread {
+
+    public void run() {
+        TestPeriodicCollectionJNI.block();
+    }
+
+    native void unlock();
+}
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/test/hotspot/jtreg/gc/g1/libTestPeriodicCollectionJNI.c	Mon Mar 04 11:49:16 2019 +0100
@@ -0,0 +1,70 @@
+/*
+ * Copyright (c) 2019, Oracle and/or its affiliates. All rights reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+
+/*
+ * Native support for TestPeriodicCollectionJNI test.
+ */
+
+#include "jni.h"
+
+#ifdef WINDOWS
+#include <windows.h>
+#else
+#include <unistd.h>
+#endif
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+static volatile int release_critical = 0;
+
+JNIEXPORT jboolean JNICALL
+Java_gc_g1_TestPeriodicCollectionJNI_blockInNative(JNIEnv* env, jobject obj, jintArray dummy) {
+    void* native_array = (*env)->GetPrimitiveArrayCritical(env, dummy, 0);
+
+    if (native_array == NULL) {
+        return JNI_FALSE;
+    }
+
+    while (!release_critical) {
+#ifdef WINDOWS
+        Sleep(1);
+#else
+        usleep(1000);
+#endif
+    }
+
+    (*env)->ReleasePrimitiveArrayCritical(env, dummy, native_array, 0);
+
+    return JNI_TRUE;
+}
+
+JNIEXPORT void JNICALL Java_gc_g1_TestPeriodicCollectionJNI_unblock(JNIEnv *env, jobject obj)
+{
+    release_critical = 1;
+}
+
+#ifdef __cplusplus
+}
+#endif