8271217: Fix race between G1PeriodicGCTask checks and GC request

Reviewed-by: iwalulya, tschatzl, lkorinth

src/hotspot/share/gc/g1/g1CollectedHeap.cpp

@@ -42,6 +42,7 @@
 #include "gc/g1/g1DirtyCardQueue.hpp"
 #include "gc/g1/g1EvacStats.inline.hpp"
 #include "gc/g1/g1FullCollector.hpp"
+#include "gc/g1/g1GCCounters.hpp"
 #include "gc/g1/g1GCParPhaseTimesTracker.hpp"
 #include "gc/g1/g1GCPhaseTimes.hpp"
 #include "gc/g1/g1GCPauseType.hpp"
@@ -2015,8 +2016,14 @@ void G1CollectedHeap::increment_old_marking_cycles_completed(bool concurrent,
   ml.notify_all();
 }
 
+// Helper for collect().
+static G1GCCounters collection_counters(G1CollectedHeap* g1h) {
+  MutexLocker ml(Heap_lock);
+  return G1GCCounters(g1h);
+}
+
 void G1CollectedHeap::collect(GCCause::Cause cause) {
-  try_collect(cause);
+  try_collect(cause, collection_counters(this));
 }
 
 // Return true if (x < y) with allowance for wraparound.
@@ -2211,25 +2218,13 @@ bool G1CollectedHeap::try_collect_concurrently(GCCause::Cause cause,
   }
 }
 
-bool G1CollectedHeap::try_collect(GCCause::Cause cause) {
-  assert_heap_not_locked();
-
-  // Lock to get consistent set of values.
-  uint gc_count_before;
-  uint full_gc_count_before;
-  uint old_marking_started_before;
-  {
-    MutexLocker ml(Heap_lock);
-    gc_count_before = total_collections();
-    full_gc_count_before = total_full_collections();
-    old_marking_started_before = _old_marking_cycles_started;
-  }
-
+bool G1CollectedHeap::try_collect(GCCause::Cause cause,
+                                  const G1GCCounters& counters_before) {
   if (should_do_concurrent_full_gc(cause)) {
     return try_collect_concurrently(cause,
-                                    gc_count_before,
-                                    old_marking_started_before);
-  } else if (GCLocker::should_discard(cause, gc_count_before)) {
+                                    counters_before.total_collections(),
+                                    counters_before.old_marking_cycles_started());
+  } else if (GCLocker::should_discard(cause, counters_before.total_collections())) {
     // Indicate failure to be consistent with VMOp failure due to
     // another collection slipping in after our gc_count but before
     // our request is processed.
@@ -2240,14 +2235,16 @@ bool G1CollectedHeap::try_collect(GCCause::Cause cause) {
     // Schedule a standard evacuation pause. We're setting word_size
     // to 0 which means that we are not requesting a post-GC allocation.
     VM_G1CollectForAllocation op(0,     /* word_size */
-                                 gc_count_before,
+                                 counters_before.total_collections(),
                                  cause,
                                  policy()->max_pause_time_ms());
     VMThread::execute(&op);
     return op.gc_succeeded();
   } else {
     // Schedule a Full GC.
-    VM_G1CollectFull op(gc_count_before, full_gc_count_before, cause);
+    VM_G1CollectFull op(counters_before.total_collections(),
+                        counters_before.total_full_collections(),
+                        cause);
     VMThread::execute(&op);
     return op.gc_succeeded();
   }

src/hotspot/share/gc/g1/g1CollectedHeap.hpp

@@ -81,6 +81,7 @@ class Space;
 class G1BatchedGangTask;
 class G1CardTableEntryClosure;
 class G1CollectionSet;
+class G1GCCounters;
 class G1Policy;
 class G1HotCardCache;
 class G1RemSet;
@@ -670,7 +671,11 @@ class G1CollectedHeap : public CollectedHeap {
   // to only parts, or aborted before completion).
   void increment_old_marking_cycles_completed(bool concurrent, bool whole_heap_examined);
 
-  uint old_marking_cycles_completed() {
+  uint old_marking_cycles_started() const {
+    return _old_marking_cycles_started;
+  }
+
+  uint old_marking_cycles_completed() const {
     return _old_marking_cycles_completed;
   }
 
@@ -1136,7 +1141,7 @@ class G1CollectedHeap : public CollectedHeap {
 
   // Perform a collection of the heap with the given cause.
   // Returns whether this collection actually executed.
-  bool try_collect(GCCause::Cause cause);
+  bool try_collect(GCCause::Cause cause, const G1GCCounters& counters_before);
 
   // True iff an evacuation has failed in the most-recent collection.
   inline bool evacuation_failed() const;

src/hotspot/share/gc/g1/g1GCCounters.cpp

@@ -0,0 +1,34 @@
+/*
+ * Copyright (c) 2021, 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.
+ *
+ */
+
+#include "precompiled.hpp"
+#include "gc/g1/g1CollectedHeap.hpp"
+#include "gc/g1/g1GCCounters.hpp"
+
+G1GCCounters::G1GCCounters(G1CollectedHeap* g1h) :
+  _total_collections(g1h->total_collections()),
+  _total_full_collections(g1h->total_full_collections()),
+  _old_marking_cycles_started(g1h->old_marking_cycles_started())
+{}
+

src/hotspot/share/gc/g1/g1GCCounters.hpp

@@ -0,0 +1,51 @@
+/*
+ * Copyright (c) 2021, 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.
+ *
+ */
+
+#ifndef SHARE_GC_G1_G1GCCOUNTERS_HPP
+#define SHARE_GC_G1_G1GCCOUNTERS_HPP
+
+#include "utilities/globalDefinitions.hpp"
+
+class G1CollectedHeap;
+
+// Record collection counters for later use when deciding whether a GC has
+// been run since the counter state was recorded.
+class G1GCCounters {
+  uint _total_collections;
+  uint _total_full_collections;
+  uint _old_marking_cycles_started;
+
+public:
+  G1GCCounters() {}             // Uninitialized
+
+  // Capture the current counters from the heap.  The caller must ensure no
+  // collections will occur while this constructor is executing.
+  explicit G1GCCounters(G1CollectedHeap* g1h);
+
+  uint total_collections() const { return _total_collections; }
+  uint total_full_collections() const { return _total_full_collections; }
+  uint old_marking_cycles_started() const { return _old_marking_cycles_started; }
+};
+
+#endif // SHARE_GC_G1_G1GCCOUNTERS_HPP

src/hotspot/share/gc/g1/g1PeriodicGCTask.cpp

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2020, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2020, 2021, 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
@@ -26,18 +26,19 @@
 #include "gc/g1/g1CollectedHeap.inline.hpp"
 #include "gc/g1/g1ConcurrentMark.inline.hpp"
 #include "gc/g1/g1ConcurrentMarkThread.inline.hpp"
+#include "gc/g1/g1GCCounters.hpp"
 #include "gc/g1/g1PeriodicGCTask.hpp"
 #include "gc/shared/suspendibleThreadSet.hpp"
 #include "logging/log.hpp"
 #include "runtime/globals.hpp"
 #include "runtime/os.hpp"
 #include "utilities/globalDefinitions.hpp"
 
-bool G1PeriodicGCTask::should_start_periodic_gc() {
+bool G1PeriodicGCTask::should_start_periodic_gc(G1CollectedHeap* g1h,
+                                                G1GCCounters* counters) {
   // Ensure no GC safepoints while we're doing the checks, to avoid data races.
   SuspendibleThreadSetJoiner sts;
 
-  G1CollectedHeap* g1h = G1CollectedHeap::heap();
   // If we are currently in a concurrent mark we are going to uncommit memory soon.
   if (g1h->concurrent_mark()->cm_thread()->in_progress()) {
     log_debug(gc, periodic)("Concurrent cycle in progress. Skipping.");
@@ -60,6 +61,11 @@ bool G1PeriodicGCTask::should_start_periodic_gc() {
                             recent_load, G1PeriodicGCSystemLoadThreshold);
     return false;
   }
+
+  // Record counters with GC safepoints blocked, to get a consistent snapshot.
+  // These are passed to try_collect so a GC between our release of the
+  // STS-joiner and the GC VMOp can be detected and cancel the request.
+  *counters = G1GCCounters(g1h);
   return true;
 }
 
@@ -70,8 +76,10 @@ void G1PeriodicGCTask::check_for_periodic_gc() {
   }
 
   log_debug(gc, periodic)("Checking for periodic GC.");
-  if (should_start_periodic_gc()) {
-    if (!G1CollectedHeap::heap()->try_collect(GCCause::_g1_periodic_collection)) {
+  G1CollectedHeap* g1h = G1CollectedHeap::heap();
+  G1GCCounters counters;
+  if (should_start_periodic_gc(g1h, &counters)) {
+    if (!g1h->try_collect(GCCause::_g1_periodic_collection, counters)) {
       log_debug(gc, periodic)("GC request denied. Skipping.");
     }
   }

src/hotspot/share/gc/g1/g1PeriodicGCTask.hpp

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2020, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2020, 2021, 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
@@ -27,14 +27,18 @@
 
 #include "gc/g1/g1ServiceThread.hpp"
 
+class G1CollectedHeap;
+class G1GCCounters;
+
 // Task handling periodic GCs
 class G1PeriodicGCTask : public G1ServiceTask {
-  bool should_start_periodic_gc();
+  bool should_start_periodic_gc(G1CollectedHeap* g1h,
+                                G1GCCounters* counters);
   void check_for_periodic_gc();
 
 public:
   G1PeriodicGCTask(const char* name);
   virtual void execute();
 };
 
-#endif // SHARE_GC_G1_G1PERIODICGCTASK_HPP
+#endif // SHARE_GC_G1_G1PERIODICGCTASK_HPP

src/hotspot/share/gc/g1/g1VMOperations.cpp

@@ -36,6 +36,17 @@
 #include "memory/universe.hpp"
 #include "runtime/interfaceSupport.inline.hpp"
 
+bool VM_G1CollectFull::skip_operation() const {
+  // There is a race between the periodic collection task's checks for
+  // wanting a collection and processing its request.  A collection in that
+  // gap should cancel the request.
+  if ((_gc_cause == GCCause::_g1_periodic_collection) &&
+      (G1CollectedHeap::heap()->total_collections() != _gc_count_before)) {
+    return true;
+  }
+  return VM_GC_Operation::skip_operation();
+}
+
 void VM_G1CollectFull::doit() {
   G1CollectedHeap* g1h = G1CollectedHeap::heap();
   GCCauseSetter x(g1h, _gc_cause);

src/hotspot/share/gc/g1/g1VMOperations.hpp

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2001, 2020, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2001, 2021, 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
@@ -33,14 +33,17 @@
 class VM_G1CollectFull : public VM_GC_Operation {
   bool _gc_succeeded;
 
+protected:
+  bool skip_operation() const override;
+
 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),
     _gc_succeeded(false) { }
-  virtual VMOp_Type type() const { return VMOp_G1CollectFull; }
-  virtual void doit();
+  VMOp_Type type() const override { return VMOp_G1CollectFull; }
+  void doit() override;
   bool gc_succeeded() const { return _gc_succeeded; }
 };