8322957: Generational ZGC: Relocation selection must join the STS

Co-authored-by: Axel Boldt-Christmas <aboldtch@openjdk.org>
Reviewed-by: eosterlund, aboldtch

src/hotspot/share/gc/shared/workerThread.cpp

@@ -31,6 +31,7 @@
 #include "runtime/init.hpp"
 #include "runtime/java.hpp"
 #include "runtime/os.hpp"
+#include "runtime/safepoint.hpp"
 
 WorkerTaskDispatcher::WorkerTaskDispatcher() :
     _task(nullptr),
@@ -141,40 +142,44 @@ void WorkerThreads::threads_do(ThreadClosure* tc) const {
   }
 }
 
-void WorkerThreads::set_indirectly_suspendible_threads() {
+template <typename Function>
+void WorkerThreads::threads_do_f(Function function) const {
+  for (uint i = 0; i < _created_workers; i++) {
+    function(_workers[i]);
+  }
+}
+
+void WorkerThreads::set_indirect_states() {
 #ifdef ASSERT
-  class SetIndirectlySuspendibleThreadClosure : public ThreadClosure {
-    virtual void do_thread(Thread* thread) {
+  const bool is_suspendible = Thread::current()->is_suspendible_thread();
+  const bool is_safepointed = Thread::current()->is_VM_thread() && SafepointSynchronize::is_at_safepoint();
+
+  threads_do_f([&](Thread* thread) {
+    assert(!thread->is_indirectly_suspendible_thread(), "Unexpected");
+    assert(!thread->is_indirectly_safepoint_thread(), "Unexpected");
+    if (is_suspendible) {
       thread->set_indirectly_suspendible_thread();
     }
-  };
-
-  if (Thread::current()->is_suspendible_thread()) {
-    SetIndirectlySuspendibleThreadClosure cl;
-    threads_do(&cl);
-  }
+    if (is_safepointed) {
+      thread->set_indirectly_safepoint_thread();
+    }
+  });
 #endif
 }
 
-void WorkerThreads::clear_indirectly_suspendible_threads() {
+void WorkerThreads::clear_indirect_states() {
 #ifdef ASSERT
-  class ClearIndirectlySuspendibleThreadClosure : public ThreadClosure {
-    virtual void do_thread(Thread* thread) {
-      thread->clear_indirectly_suspendible_thread();
-    }
-  };
-
-  if (Thread::current()->is_suspendible_thread()) {
-    ClearIndirectlySuspendibleThreadClosure cl;
-    threads_do(&cl);
-  }
+  threads_do_f([&](Thread* thread) {
+    thread->clear_indirectly_suspendible_thread();
+    thread->clear_indirectly_safepoint_thread();
+  });
 #endif
 }
 
 void WorkerThreads::run_task(WorkerTask* task) {
-  set_indirectly_suspendible_threads();
+  set_indirect_states();
   _dispatcher.coordinator_distribute_task(task, _active_workers);
-  clear_indirectly_suspendible_threads();
+  clear_indirect_states();
 }
 
 void WorkerThreads::run_task(WorkerTask* task, uint num_workers) {

src/hotspot/share/gc/shared/workerThread.hpp

@@ -93,8 +93,8 @@ private:
 
   WorkerThread* create_worker(uint name_suffix);
 
-  void set_indirectly_suspendible_threads();
-  void clear_indirectly_suspendible_threads();
+  void set_indirect_states();
+  void clear_indirect_states();
 
 protected:
   virtual void on_create_worker(WorkerThread* worker) {}
@@ -111,6 +111,8 @@ public:
   uint set_active_workers(uint num_workers);
 
   void threads_do(ThreadClosure* tc) const;
+  template <typename Function>
+  void threads_do_f(Function function) const;
 
   const char* name() const { return _name; }
 

src/hotspot/share/gc/z/zBarrier.inline.hpp

@@ -26,14 +26,13 @@
 
 #include "gc/z/zBarrier.hpp"
 
-#include "code/codeCache.hpp"
 #include "gc/z/zAddress.inline.hpp"
 #include "gc/z/zGeneration.inline.hpp"
 #include "gc/z/zHeap.inline.hpp"
 #include "gc/z/zResurrection.inline.hpp"
+#include "gc/z/zVerify.hpp"
 #include "oops/oop.hpp"
 #include "runtime/atomic.hpp"
-#include "runtime/continuation.hpp"
 
 // A self heal must always "upgrade" the address metadata bits in
 // accordance with the metadata bits state machine. The following
@@ -320,17 +319,9 @@ inline zaddress ZBarrier::make_load_good_no_relocate(zpointer o) {
   return remap(ZPointer::uncolor_unsafe(o), remap_generation(o));
 }
 
-inline void z_assert_is_barrier_safe() {
-  assert(!Thread::current()->is_ConcurrentGC_thread() ||          /* Need extra checks for ConcurrentGCThreads */
-         Thread::current()->is_suspendible_thread() ||            /* Thread prevents safepoints */
-         Thread::current()->is_indirectly_suspendible_thread() || /* Coordinator thread prevents safepoints */
-         SafepointSynchronize::is_at_safepoint(),                 /* Is at safepoint */
-         "Shouldn't perform load barrier");
-}
-
 template <typename ZBarrierSlowPath>
 inline zaddress ZBarrier::barrier(ZBarrierFastPath fast_path, ZBarrierSlowPath slow_path, ZBarrierColor color, volatile zpointer* p, zpointer o, bool allow_null) {
-  z_assert_is_barrier_safe();
+  z_verify_safepoints_are_blocked();
 
   // Fast path
   if (fast_path(o)) {

src/hotspot/share/gc/z/zGeneration.cpp

@@ -286,6 +286,10 @@ void ZGeneration::desynchronize_relocation() {
   _relocate.desynchronize();
 }
 
+bool ZGeneration::is_relocate_queue_active() const {
+  return _relocate.is_queue_active();
+}
+
 void ZGeneration::reset_statistics() {
   assert(SafepointSynchronize::is_at_safepoint(), "Should be at safepoint");
   _freed = 0;
@@ -1497,7 +1501,7 @@ void ZGenerationOld::remap_young_roots() {
   uint remap_nworkers = clamp(ZGeneration::young()->workers()->active_workers() + prev_nworkers, 1u, ZOldGCThreads);
   _workers.set_active_workers(remap_nworkers);
 
-  // TODO: The STS joiner is only needed to satisfy z_assert_is_barrier_safe that doesn't
+  // TODO: The STS joiner is only needed to satisfy ZBarrier::assert_is_state_barrier_safe that doesn't
   // understand the driver locker. Consider making the assert aware of the driver locker.
   SuspendibleThreadSetJoiner sts_joiner;
 

src/hotspot/share/gc/z/zGeneration.hpp

@@ -166,6 +166,7 @@ public:
   // Relocation
   void synchronize_relocation();
   void desynchronize_relocation();
+  bool is_relocate_queue_active() const;
   zaddress relocate_or_remap_object(zaddress_unsafe addr);
   zaddress remap_object(zaddress_unsafe addr);
 

src/hotspot/share/gc/z/zIterator.inline.hpp

@@ -26,11 +26,21 @@
 
 #include "gc/z/zIterator.hpp"
 
+#include "gc/z/zVerify.hpp"
 #include "memory/iterator.inline.hpp"
 #include "oops/objArrayOop.hpp"
 #include "oops/oop.inline.hpp"
 
 inline bool ZIterator::is_invisible_object(oop obj) {
+  // This is a good place to make sure that we can't concurrently iterate over
+  // objects while VMThread operations think they have exclusive access to the
+  // object graph.
+  //
+  // One example that have caused problems is the JFR Leak Profiler, which
+  // sets the mark word to a value that makes the object arrays look like
+  // invisible objects.
+  z_verify_safepoints_are_blocked();
+
   return obj->mark_acquire().is_marked();
 }
 

src/hotspot/share/gc/z/zRelocate.cpp

@@ -87,6 +87,7 @@ ZRelocateQueue::ZRelocateQueue()
     _nworkers(0),
     _nsynchronized(0),
     _synchronize(false),
+    _is_active(false),
     _needs_attention(0) {}
 
 bool ZRelocateQueue::needs_attention() const {
@@ -103,6 +104,20 @@ void ZRelocateQueue::dec_needs_attention() {
   assert(needs_attention == 0 || needs_attention == 1, "Invalid state");
 }
 
+void ZRelocateQueue::activate(uint nworkers) {
+  _is_active = true;
+  join(nworkers);
+}
+
+void ZRelocateQueue::deactivate() {
+  Atomic::store(&_is_active, false);
+  clear();
+}
+
+bool ZRelocateQueue::is_active() const {
+  return Atomic::load(&_is_active);
+}
+
 void ZRelocateQueue::join(uint nworkers) {
   assert(nworkers != 0, "Must request at least one worker");
   assert(_nworkers == 0, "Invalid state");
@@ -327,7 +342,7 @@ ZWorkers* ZRelocate::workers() const {
 }
 
 void ZRelocate::start() {
-  _queue.join(workers()->active_workers());
+  _queue.activate(workers()->active_workers());
 }
 
 void ZRelocate::add_remset(volatile zpointer* p) {
@@ -1088,6 +1103,9 @@ public:
 
   ~ZRelocateTask() {
     _generation->stat_relocation()->at_relocate_end(_small_allocator.in_place_count(), _medium_allocator.in_place_count());
+
+    // Signal that we're not using the queue anymore. Used mostly for asserts.
+    _queue->deactivate();
   }
 
   virtual void work() {
@@ -1232,8 +1250,6 @@ void ZRelocate::relocate(ZRelocationSet* relocation_set) {
     ZRelocateAddRemsetForFlipPromoted task(relocation_set->flip_promoted_pages());
     workers()->run(&task);
   }
-
-  _queue.clear();
 }
 
 ZPageAge ZRelocate::compute_to_age(ZPageAge from_age) {
@@ -1316,3 +1332,7 @@ void ZRelocate::desynchronize() {
 ZRelocateQueue* ZRelocate::queue() {
   return &_queue;
 }
+
+bool ZRelocate::is_queue_active() const {
+  return _queue.is_active();
+}

src/hotspot/share/gc/z/zRelocate.hpp

@@ -41,6 +41,7 @@ private:
   uint                 _nworkers;
   uint                 _nsynchronized;
   bool                 _synchronize;
+  volatile bool        _is_active;
   volatile int         _needs_attention;
 
   bool needs_attention() const;
@@ -53,6 +54,10 @@ private:
 public:
   ZRelocateQueue();
 
+  void activate(uint nworkers);
+  void deactivate();
+  bool is_active() const;
+
   void join(uint nworkers);
   void resize_workers(uint nworkers);
   void leave();
@@ -99,6 +104,8 @@ public:
   void desynchronize();
 
   ZRelocateQueue* queue();
+
+  bool is_queue_active() const;
 };
 
 #endif // SHARE_GC_Z_ZRELOCATE_HPP

src/hotspot/share/gc/z/zRelocationSet.cpp

@@ -106,11 +106,16 @@ public:
   }
 
   virtual void work() {
+    // Join the STS to block out VMThreads while running promote_barrier_on_young_oop_field
+    SuspendibleThreadSetJoiner sts_joiner;
+
     // Allocate and install forwardings for small pages
     for (size_t page_index; _small_iter.next_index(&page_index);) {
       ZPage* page = _small->at(int(page_index));
       ZForwarding* const forwarding = ZForwarding::alloc(_allocator, page, to_age(page));
       install_small(forwarding, _medium->length() + page_index);
+
+      SuspendibleThreadSet::yield();
     }
 
     // Allocate and install forwardings for medium pages
@@ -118,6 +123,8 @@ public:
       ZPage* page = _medium->at(int(page_index));
       ZForwarding* const forwarding = ZForwarding::alloc(_allocator, page, to_age(page));
       install_medium(forwarding, page_index);
+
+      SuspendibleThreadSet::yield();
     }
   }
 

src/hotspot/share/gc/z/zUncoloredRoot.inline.hpp

@@ -29,11 +29,12 @@
 #include "gc/z/zAddress.inline.hpp"
 #include "gc/z/zBarrier.inline.hpp"
 #include "gc/z/zHeap.inline.hpp"
+#include "gc/z/zBarrier.hpp"
 #include "oops/oop.hpp"
 
 template <typename ObjectFunctionT>
 inline void ZUncoloredRoot::barrier(ObjectFunctionT function, zaddress_unsafe* p, uintptr_t color) {
-  z_assert_is_barrier_safe();
+  z_verify_safepoints_are_blocked();
 
   const zaddress_unsafe addr = Atomic::load(p);
   assert_is_valid(addr);

src/hotspot/share/gc/z/zVerify.cpp

@@ -43,16 +43,67 @@
 #include "runtime/frame.inline.hpp"
 #include "runtime/globals.hpp"
 #include "runtime/handles.hpp"
-#include "runtime/javaThread.hpp"
+#include "runtime/javaThread.inline.hpp"
+#include "runtime/mutexLocker.hpp"
 #include "runtime/safepoint.hpp"
 #include "runtime/stackFrameStream.inline.hpp"
 #include "runtime/stackWatermark.inline.hpp"
 #include "runtime/stackWatermarkSet.inline.hpp"
+#include "runtime/thread.hpp"
 #include "utilities/debug.hpp"
 #include "utilities/globalDefinitions.hpp"
 #include "utilities/preserveException.hpp"
 #include "utilities/resourceHash.hpp"
 
+#ifdef ASSERT
+
+// Used to verify that safepoints operations can't be scheduled concurrently
+// with callers to this function. Typically used to verify that object oops
+// and headers are safe to access.
+void z_verify_safepoints_are_blocked() {
+  Thread* current = Thread::current();
+
+  if (current->is_ConcurrentGC_thread()) {
+    assert(current->is_suspendible_thread(), // Thread prevents safepoints
+        "Safepoints are not blocked by current thread");
+
+  } else if (current->is_Worker_thread()) {
+    assert(// Check if ...
+        // the thread prevents safepoints
+        current->is_suspendible_thread() ||
+        // the coordinator thread is the safepointing VMThread
+        current->is_indirectly_safepoint_thread() ||
+        // the coordinator thread prevents safepoints
+        current->is_indirectly_suspendible_thread() ||
+        // the RelocateQueue prevents safepoints
+        //
+        // RelocateQueue acts as a pseudo STS leaver/joiner and blocks
+        // safepoints. There's currently no infrastructure  to check if the
+        // current thread is active or not, so check the global states instead.
+        ZGeneration::young()->is_relocate_queue_active() ||
+        ZGeneration::old()->is_relocate_queue_active(),
+        "Safepoints are not blocked by current thread");
+
+  } else if (current->is_Java_thread()) {
+    JavaThreadState state = JavaThread::cast(current)->thread_state();
+    assert(state == _thread_in_Java || state == _thread_in_vm || state == _thread_new,
+        "Safepoints are not blocked by current thread from state: %d", state);
+
+  } else if (current->is_JfrSampler_thread()) {
+    // The JFR sampler thread blocks out safepoints with this lock.
+    assert_lock_strong(Threads_lock);
+
+  } else if (current->is_VM_thread()) {
+    // The VM Thread doesn't schedule new safepoints while executing
+    // other safepoint or handshake operations.
+
+  } else {
+    fatal("Unexpected thread type");
+  }
+}
+
+#endif
+
 #define BAD_OOP_ARG(o, p)   "Bad oop " PTR_FORMAT " found at " PTR_FORMAT, untype(o), p2i(p)
 
 static bool z_is_null_relaxed(zpointer o) {

src/hotspot/share/gc/z/zVerify.hpp

@@ -30,6 +30,8 @@ class frame;
 class ZForwarding;
 class ZPageAllocator;
 
+NOT_DEBUG(inline) void z_verify_safepoints_are_blocked() NOT_DEBUG_RETURN;
+
 class ZVerify : public AllStatic {
 private:
   static void roots_strong(bool verify_after_old_mark);

src/hotspot/share/runtime/thread.cpp

@@ -73,6 +73,7 @@ Thread::Thread() {
   set_lgrp_id(-1);
   DEBUG_ONLY(clear_suspendible_thread();)
   DEBUG_ONLY(clear_indirectly_suspendible_thread();)
+  DEBUG_ONLY(clear_indirectly_safepoint_thread();)
 
   // allocated data structures
   set_osthread(nullptr);

src/hotspot/share/runtime/thread.hpp

@@ -207,6 +207,7 @@ class Thread: public ThreadShadow {
  private:
   DEBUG_ONLY(bool _suspendible_thread;)
   DEBUG_ONLY(bool _indirectly_suspendible_thread;)
+  DEBUG_ONLY(bool _indirectly_safepoint_thread;)
 
  public:
   // Determines if a heap allocation failure will be retried
@@ -225,6 +226,10 @@ class Thread: public ThreadShadow {
   void set_indirectly_suspendible_thread()   { _indirectly_suspendible_thread = true; }
   void clear_indirectly_suspendible_thread() { _indirectly_suspendible_thread = false; }
   bool is_indirectly_suspendible_thread()    { return _indirectly_suspendible_thread; }
+
+  void set_indirectly_safepoint_thread()   { _indirectly_safepoint_thread = true; }
+  void clear_indirectly_safepoint_thread() { _indirectly_safepoint_thread = false; }
+  bool is_indirectly_safepoint_thread()    { return _indirectly_safepoint_thread; }
 #endif
 
  private: