8225702: Safepoint counter can't be used for safepoint detection
authorrehn
Wed, 26 Jun 2019 18:22:28 +0200
changeset 55514 03468b206457
parent 55505 0bf9477626ef
child 55515 4c52949a3487
8225702: Safepoint counter can't be used for safepoint detection Reviewed-by: dcubed, dholmes
src/hotspot/share/ci/ciMethodData.cpp
src/hotspot/share/code/dependencyContext.hpp
src/hotspot/share/runtime/biasedLocking.cpp
src/hotspot/share/runtime/safepoint.cpp
src/hotspot/share/runtime/safepoint.hpp
src/hotspot/share/runtime/vmThread.cpp
--- a/src/hotspot/share/ci/ciMethodData.cpp	Wed Jun 26 11:25:51 2019 -0400
+++ b/src/hotspot/share/ci/ciMethodData.cpp	Wed Jun 26 18:22:28 2019 +0200
@@ -81,13 +81,13 @@
 // Check for entries that reference an unloaded method
 class PrepareExtraDataClosure : public CleanExtraDataClosure {
   MethodData*            _mdo;
-  uint64_t               _safepoint_counter;
+  SafepointStateTracker  _safepoint_tracker;
   GrowableArray<Method*> _uncached_methods;
 
 public:
   PrepareExtraDataClosure(MethodData* mdo)
     : _mdo(mdo),
-      _safepoint_counter(SafepointSynchronize::safepoint_counter()),
+      _safepoint_tracker(SafepointSynchronize::safepoint_state_tracker()),
       _uncached_methods()
   { }
 
@@ -103,7 +103,7 @@
   }
 
   bool has_safepointed() {
-    return SafepointSynchronize::safepoint_counter() != _safepoint_counter;
+    return _safepoint_tracker.safepoint_state_changed();
   }
 
   bool finish() {
--- a/src/hotspot/share/code/dependencyContext.hpp	Wed Jun 26 11:25:51 2019 -0400
+++ b/src/hotspot/share/code/dependencyContext.hpp	Wed Jun 26 18:22:28 2019 +0200
@@ -99,15 +99,15 @@
   // Safepoints are forbidden during DC lifetime. GC can invalidate
   // _dependency_context_addr if it relocates the holder
   // (e.g. CallSiteContext Java object).
-  uint64_t _safepoint_counter;
+  SafepointStateTracker _safepoint_tracker;
 
   DependencyContext(nmethodBucket* volatile* bucket_addr, volatile uint64_t* last_cleanup_addr)
     : _dependency_context_addr(bucket_addr),
       _last_cleanup_addr(last_cleanup_addr),
-      _safepoint_counter(SafepointSynchronize::safepoint_counter()) {}
+      _safepoint_tracker(SafepointSynchronize::safepoint_state_tracker()) {}
 
   ~DependencyContext() {
-    assert(SafepointSynchronize::is_same_safepoint(_safepoint_counter), "must be the same safepoint");
+    assert(!_safepoint_tracker.safepoint_state_changed(), "must be the same safepoint");
   }
 #else
   DependencyContext(nmethodBucket* volatile* bucket_addr, volatile uint64_t* last_cleanup_addr)
--- a/src/hotspot/share/runtime/biasedLocking.cpp	Wed Jun 26 11:25:51 2019 -0400
+++ b/src/hotspot/share/runtime/biasedLocking.cpp	Wed Jun 26 18:22:28 2019 +0200
@@ -548,7 +548,7 @@
       if (biased_locker != NULL) {
         _biased_locker_id = JFR_THREAD_ID(biased_locker);
       }
-      _safepoint_id = SafepointSynchronize::safepoint_counter();
+      _safepoint_id = SafepointSynchronize::safepoint_id();
       clean_up_cached_monitor_info();
       return;
     } else {
@@ -589,7 +589,7 @@
 
   virtual void doit() {
     _status_code = bulk_revoke_or_rebias_at_safepoint((*_obj)(), _bulk_rebias, _attempt_rebias_of_object, _requesting_thread);
-    _safepoint_id = SafepointSynchronize::safepoint_counter();
+    _safepoint_id = SafepointSynchronize::safepoint_id();
     clean_up_cached_monitor_info();
   }
 
--- a/src/hotspot/share/runtime/safepoint.cpp	Wed Jun 26 11:25:51 2019 -0400
+++ b/src/hotspot/share/runtime/safepoint.cpp	Wed Jun 26 18:22:28 2019 +0200
@@ -118,12 +118,22 @@
   }
 }
 
+// SafepointCheck
+SafepointStateTracker::SafepointStateTracker(uint64_t safepoint_id, bool at_safepoint)
+  : _safepoint_id(safepoint_id), _at_safepoint(at_safepoint) {}
+
+bool SafepointStateTracker::safepoint_state_changed() {
+  return _safepoint_id != SafepointSynchronize::safepoint_id() ||
+    _at_safepoint != SafepointSynchronize::is_at_safepoint();
+}
+
 // --------------------------------------------------------------------------------------------------
 // Implementation of Safepoint begin/end
 
 SafepointSynchronize::SynchronizeState volatile SafepointSynchronize::_state = SafepointSynchronize::_not_synchronized;
 int SafepointSynchronize::_waiting_to_block = 0;
 volatile uint64_t SafepointSynchronize::_safepoint_counter = 0;
+uint64_t SafepointSynchronize::_safepoint_id = 0;
 const uint64_t SafepointSynchronize::InactiveSafepointCounter = 0;
 int SafepointSynchronize::_current_jni_active_count = 0;
 
@@ -154,7 +164,7 @@
   --_waiting_to_block;
 }
 
-static bool thread_not_running(ThreadSafepointState *cur_state) {
+bool SafepointSynchronize::thread_not_running(ThreadSafepointState *cur_state) {
   if (!cur_state->is_running()) {
     return true;
   }
@@ -408,6 +418,9 @@
 
   OrderAccess::fence();
 
+  // Set the new id
+  ++_safepoint_id;
+
 #ifdef ASSERT
   // Make sure all the threads were visited.
   for (JavaThreadIteratorWithHandle jtiwh; JavaThread *cur = jtiwh.next(); ) {
@@ -419,7 +432,7 @@
   GCLocker::set_jni_lock_count(_current_jni_active_count);
 
   post_safepoint_synchronize_event(sync_event,
-                                   _safepoint_counter,
+                                   _safepoint_id,
                                    initial_running,
                                    _waiting_to_block, iterations);
 
@@ -429,14 +442,14 @@
   // needs cleanup to be completed before running the GC op.
   EventSafepointCleanup cleanup_event;
   do_cleanup_tasks();
-  post_safepoint_cleanup_event(cleanup_event, _safepoint_counter);
+  post_safepoint_cleanup_event(cleanup_event, _safepoint_id);
 
-  post_safepoint_begin_event(begin_event, _safepoint_counter, nof_threads, _current_jni_active_count);
+  post_safepoint_begin_event(begin_event, _safepoint_id, nof_threads, _current_jni_active_count);
   SafepointTracing::cleanup();
 }
 
 void SafepointSynchronize::disarm_safepoint() {
-  uint64_t safepoint_id = _safepoint_counter;
+  uint64_t active_safepoint_counter = _safepoint_counter;
   {
     JavaThreadIteratorWithHandle jtiwh;
 #ifdef ASSERT
@@ -475,7 +488,7 @@
     jtiwh.rewind();
     for (; JavaThread *current = jtiwh.next(); ) {
       // Clear the visited flag to ensure that the critical counts are collected properly.
-      DEBUG_ONLY(current->reset_visited_for_critical_count(safepoint_id);)
+      DEBUG_ONLY(current->reset_visited_for_critical_count(active_safepoint_counter);)
       ThreadSafepointState* cur_state = current->safepoint_state();
       assert(!cur_state->is_running(), "Thread not suspended at safepoint");
       cur_state->restart(); // TSS _running
@@ -497,7 +510,6 @@
 void SafepointSynchronize::end() {
   assert(Threads_lock->owned_by_self(), "must hold Threads_lock");
   EventSafepointEnd event;
-  uint64_t safepoint_id = _safepoint_counter;
   assert(Thread::current()->is_VM_thread(), "Only VM thread can execute a safepoint");
 
   disarm_safepoint();
@@ -506,7 +518,7 @@
 
   SafepointTracing::end();
 
-  post_safepoint_end_event(event, safepoint_id);
+  post_safepoint_end_event(event, safepoint_id());
 }
 
 bool SafepointSynchronize::is_cleanup_needed() {
@@ -554,7 +566,7 @@
     _counters(counters) {}
 
   void work(uint worker_id) {
-    uint64_t safepoint_id = SafepointSynchronize::safepoint_counter();
+    uint64_t safepoint_id = SafepointSynchronize::safepoint_id();
     // All threads deflate monitors and mark nmethods (if necessary).
     Threads::possibly_parallel_threads_do(true, &_cleanup_threads_cl);
 
--- a/src/hotspot/share/runtime/safepoint.hpp	Wed Jun 26 11:25:51 2019 -0400
+++ b/src/hotspot/share/runtime/safepoint.hpp	Wed Jun 26 18:22:28 2019 +0200
@@ -48,6 +48,14 @@
 
 class ThreadSafepointState;
 
+class SafepointStateTracker {
+  uint64_t _safepoint_id;
+  bool     _at_safepoint;
+public:
+  SafepointStateTracker(uint64_t safepoint_id, bool at_safepoint);
+  bool safepoint_state_changed();
+};
+
 //
 // Implements roll-forward to safepoint (safepoint synchronization)
 //
@@ -77,6 +85,7 @@
   friend class SafepointMechanism;
   friend class ThreadSafepointState;
   friend class HandshakeState;
+  friend class SafepointStateTracker;
 
   // Threads might read this flag directly, without acquiring the Threads_lock:
   static volatile SynchronizeState _state;
@@ -91,6 +100,11 @@
   // safepoint.
   static volatile uint64_t _safepoint_counter;
 
+  // A change in this counter or a change in the result of
+  // is_at_safepoint() are used by SafepointStateTracker::
+  // safepoint_state_changed() to determine its answer.
+  static uint64_t _safepoint_id;
+
   // JavaThreads that need to block for the safepoint will stop on the
   // _wait_barrier, where they can quickly be started again.
   static WaitBarrier* _wait_barrier;
@@ -114,6 +128,7 @@
   static void disarm_safepoint();
   static void increment_jni_active_count();
   static void decrement_waiting_to_block();
+  static bool thread_not_running(ThreadSafepointState *cur_state);
 
   // Used in safepoint_safe to do a stable load of the thread state.
   static bool try_stable_load_state(JavaThreadState *state,
@@ -127,6 +142,8 @@
   // If true the VMThread may safely process the handshake operation for the JavaThread.
   static bool handshake_safe(JavaThread *thread);
 
+  static uint64_t safepoint_counter()             { return _safepoint_counter; }
+
 public:
 
   static void init(Thread* vmthread);
@@ -141,8 +158,15 @@
   // Query
   static bool is_at_safepoint()                   { return _state == _synchronized; }
   static bool is_synchronizing()                  { return _state == _synchronizing; }
-  static uint64_t safepoint_counter()             { return _safepoint_counter; }
-  static bool is_same_safepoint(uint64_t counter) { return (SafepointSynchronize::safepoint_counter() - counter) < 2; }
+
+  static uint64_t safepoint_id() {
+    return _safepoint_id;
+  }
+
+  static SafepointStateTracker safepoint_state_tracker() {
+    return SafepointStateTracker(safepoint_id(), is_at_safepoint());
+  }
+
   // Exception handling for page polling
   static void handle_polling_page_exception(JavaThread *thread);
 
--- a/src/hotspot/share/runtime/vmThread.cpp	Wed Jun 26 11:25:51 2019 -0400
+++ b/src/hotspot/share/runtime/vmThread.cpp	Wed Jun 26 18:22:28 2019 +0200
@@ -388,7 +388,7 @@
   // For concurrent vm operations, the thread id is set to 0 indicating thread is unknown.
   // This is because the caller thread could have exited already.
   event->set_caller(is_concurrent ? 0 : JFR_THREAD_ID(op->calling_thread()));
-  event->set_safepointId(evaluate_at_safepoint ? SafepointSynchronize::safepoint_counter() : 0);
+  event->set_safepointId(evaluate_at_safepoint ? SafepointSynchronize::safepoint_id() : 0);
   event->commit();
 }