8149834: gc/shared/gcTimer.cpp:88 assert(_is_concurrent_phase_active) failed: A concurrent phase is not active

Compare-and-exchange for concurrent gc timer related flag at G1CollectedHeap

Reviewed-by: jmasa, drwhite

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

@@ -2329,9 +2329,13 @@ void G1CollectedHeap::register_concurrent_cycle_end() {
     GCIdMarkAndRestore conc_gc_id_mark(_cmThread->gc_id());
     if (_cm->has_aborted()) {
       _gc_tracer_cm->report_concurrent_mode_failure();
+
+      // ConcurrentGCTimer will be ended as well.
+      _cm->register_concurrent_gc_end_and_stop_timer();
+    } else {
+      _gc_timer_cm->register_gc_end();
     }
 
-    _gc_timer_cm->register_gc_end();
     _gc_tracer_cm->report_gc_end(_gc_timer_cm->gc_end(), _gc_timer_cm->time_partitions());
 
     // Clear state variables to prepare for the next concurrent cycle.

hotspot/src/share/vm/gc/g1/g1ConcurrentMark.cpp

@@ -441,7 +441,7 @@ G1ConcurrentMark::G1ConcurrentMark(G1CollectedHeap* g1h, G1RegionToSpaceMapper*
   _has_aborted(false),
   _restart_for_overflow(false),
   _concurrent_marking_in_progress(false),
-  _concurrent_phase_started(false),
+  _concurrent_phase_status(ConcPhaseNotStarted),
 
   // _verbose_level set below
 
@@ -1008,16 +1008,43 @@ void G1ConcurrentMark::scanRootRegions() {
 }
 
 void G1ConcurrentMark::register_concurrent_phase_start(const char* title) {
-  assert(!_concurrent_phase_started, "Sanity");
+  uint old_val = 0;
-  _concurrent_phase_started = true;
+  do {
+    old_val = Atomic::cmpxchg(ConcPhaseStarted, &_concurrent_phase_status, ConcPhaseNotStarted);
+  } while (old_val != ConcPhaseNotStarted);
   _g1h->gc_timer_cm()->register_gc_concurrent_start(title);
 }
 
-void G1ConcurrentMark::register_concurrent_phase_end() {
+void G1ConcurrentMark::register_concurrent_phase_end_common(bool end_timer) {
-  if (_concurrent_phase_started) {
+  if (_concurrent_phase_status == ConcPhaseNotStarted) {
-    _concurrent_phase_started = false;
+    return;
-    _g1h->gc_timer_cm()->register_gc_concurrent_end();
   }
+
+  uint old_val = Atomic::cmpxchg(ConcPhaseStopping, &_concurrent_phase_status, ConcPhaseStarted);
+  if (old_val == ConcPhaseStarted) {
+    _g1h->gc_timer_cm()->register_gc_concurrent_end();
+    // If 'end_timer' is true, we came here to end timer which needs concurrent phase ended.
+    // We need to end it before changing the status to 'ConcPhaseNotStarted' to prevent
+    // starting a new concurrent phase by 'ConcurrentMarkThread'.
+    if (end_timer) {
+      _g1h->gc_timer_cm()->register_gc_end();
+    }
+    old_val = Atomic::cmpxchg(ConcPhaseNotStarted, &_concurrent_phase_status, ConcPhaseStopping);
+    assert(old_val == ConcPhaseStopping, "Should not have changed since we entered this scope.");
+  } else {
+    do {
+      // Let other thread finish changing '_concurrent_phase_status' to 'ConcPhaseNotStarted'.
+      os::naked_short_sleep(1);
+    } while (_concurrent_phase_status != ConcPhaseNotStarted);
+  }
+}
+
+void G1ConcurrentMark::register_concurrent_phase_end() {
+  register_concurrent_phase_end_common(false);
+}
+
+void G1ConcurrentMark::register_concurrent_gc_end_and_stop_timer() {
+  register_concurrent_phase_end_common(true);
 }
 
 void G1ConcurrentMark::markFromRoots() {
@@ -2605,9 +2632,6 @@ void G1ConcurrentMark::abort() {
 
   _g1h->trace_heap_after_concurrent_cycle();
 
-  // Close any open concurrent phase timing
-  register_concurrent_phase_end();
-
   _g1h->register_concurrent_cycle_end();
 }
 

hotspot/src/share/vm/gc/g1/g1ConcurrentMark.hpp

@@ -352,8 +352,17 @@ protected:
   // time of remark.
   volatile bool           _concurrent_marking_in_progress;
 
-  // Keep track of whether we have started concurrent phase or not.
+  // There would be a race between ConcurrentMarkThread and VMThread(ConcurrentMark::abort())
-  bool                    _concurrent_phase_started;
+  // to call ConcurrentGCTimer::register_gc_concurrent_end().
+  // And this variable is used to keep track of concurrent phase.
+  volatile uint           _concurrent_phase_status;
+  // Concurrent phase is not yet started.
+  static const uint       ConcPhaseNotStarted = 0;
+  // Concurrent phase is started.
+  static const uint       ConcPhaseStarted = 1;
+  // Caller thread of ConcurrentGCTimer::register_gc_concurrent_end() is ending concurrent phase.
+  // So other thread should wait until the status to be changed to ConcPhaseNotStarted.
+  static const uint       ConcPhaseStopping = 2;
 
   // All of these times are in ms
   NumberSeq _init_times;
@@ -485,6 +494,9 @@ protected:
   // Set to true when initialization is complete
   bool _completed_initialization;
 
+  // end_timer, true to end gc timer after ending concurrent phase.
+  void register_concurrent_phase_end_common(bool end_timer);
+
 public:
   // Manipulation of the global mark stack.
   // The push and pop operations are used by tasks for transfers
@@ -520,6 +532,8 @@ public:
 
   void register_concurrent_phase_start(const char* title);
   void register_concurrent_phase_end();
+  // Ends both concurrent phase and timer.
+  void register_concurrent_gc_end_and_stop_timer();
 
   void update_accum_task_vtime(int i, double vtime) {
     _accum_task_vtime[i] += vtime;