From ba23025cd8a9c1af37afea6444ce5ea2ff41e5af Mon Sep 17 00:00:00 2001 From: Stefan Karlsson Date: Fri, 12 Jan 2024 09:32:50 +0000 Subject: [PATCH] 8322957: Generational ZGC: Relocation selection must join the STS Co-authored-by: Axel Boldt-Christmas Reviewed-by: eosterlund, aboldtch --- src/hotspot/share/gc/shared/workerThread.cpp | 49 +++++++++-------- src/hotspot/share/gc/shared/workerThread.hpp | 6 ++- src/hotspot/share/gc/z/zBarrier.inline.hpp | 13 +---- src/hotspot/share/gc/z/zGeneration.cpp | 6 ++- src/hotspot/share/gc/z/zGeneration.hpp | 1 + src/hotspot/share/gc/z/zIterator.inline.hpp | 10 ++++ src/hotspot/share/gc/z/zRelocate.cpp | 26 +++++++-- src/hotspot/share/gc/z/zRelocate.hpp | 7 +++ src/hotspot/share/gc/z/zRelocationSet.cpp | 7 +++ .../share/gc/z/zUncoloredRoot.inline.hpp | 3 +- src/hotspot/share/gc/z/zVerify.cpp | 53 ++++++++++++++++++- src/hotspot/share/gc/z/zVerify.hpp | 2 + src/hotspot/share/runtime/thread.cpp | 1 + src/hotspot/share/runtime/thread.hpp | 5 ++ 14 files changed, 148 insertions(+), 41 deletions(-) diff --git a/src/hotspot/share/gc/shared/workerThread.cpp b/src/hotspot/share/gc/shared/workerThread.cpp index b64c5050a22..49e43c284fa 100644 --- a/src/hotspot/share/gc/shared/workerThread.cpp +++ b/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 +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) { diff --git a/src/hotspot/share/gc/shared/workerThread.hpp b/src/hotspot/share/gc/shared/workerThread.hpp index 39687e2581c..642c8d93f59 100644 --- a/src/hotspot/share/gc/shared/workerThread.hpp +++ b/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 + void threads_do_f(Function function) const; const char* name() const { return _name; } diff --git a/src/hotspot/share/gc/z/zBarrier.inline.hpp b/src/hotspot/share/gc/z/zBarrier.inline.hpp index e0d83619934..2c81c14865b 100644 --- a/src/hotspot/share/gc/z/zBarrier.inline.hpp +++ b/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 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)) { diff --git a/src/hotspot/share/gc/z/zGeneration.cpp b/src/hotspot/share/gc/z/zGeneration.cpp index 5e42fbd7ef8..0396a3f7516 100644 --- a/src/hotspot/share/gc/z/zGeneration.cpp +++ b/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; diff --git a/src/hotspot/share/gc/z/zGeneration.hpp b/src/hotspot/share/gc/z/zGeneration.hpp index 23736f45b7b..32762a50b62 100644 --- a/src/hotspot/share/gc/z/zGeneration.hpp +++ b/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); diff --git a/src/hotspot/share/gc/z/zIterator.inline.hpp b/src/hotspot/share/gc/z/zIterator.inline.hpp index 9ccacdc9a3c..af97a549b0d 100644 --- a/src/hotspot/share/gc/z/zIterator.inline.hpp +++ b/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(); } diff --git a/src/hotspot/share/gc/z/zRelocate.cpp b/src/hotspot/share/gc/z/zRelocate.cpp index 60a6616d0a7..78efa7cdb12 100644 --- a/src/hotspot/share/gc/z/zRelocate.cpp +++ b/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(); +} diff --git a/src/hotspot/share/gc/z/zRelocate.hpp b/src/hotspot/share/gc/z/zRelocate.hpp index ed54103d53c..1b35abdf521 100644 --- a/src/hotspot/share/gc/z/zRelocate.hpp +++ b/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 diff --git a/src/hotspot/share/gc/z/zRelocationSet.cpp b/src/hotspot/share/gc/z/zRelocationSet.cpp index 83bdf13b2bb..92f245777b4 100644 --- a/src/hotspot/share/gc/z/zRelocationSet.cpp +++ b/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(); } } diff --git a/src/hotspot/share/gc/z/zUncoloredRoot.inline.hpp b/src/hotspot/share/gc/z/zUncoloredRoot.inline.hpp index 3eb28d3cafe..a7d39e5ed9c 100644 --- a/src/hotspot/share/gc/z/zUncoloredRoot.inline.hpp +++ b/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 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); diff --git a/src/hotspot/share/gc/z/zVerify.cpp b/src/hotspot/share/gc/z/zVerify.cpp index dafe680939e..c88d7ad7978 100644 --- a/src/hotspot/share/gc/z/zVerify.cpp +++ b/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) { diff --git a/src/hotspot/share/gc/z/zVerify.hpp b/src/hotspot/share/gc/z/zVerify.hpp index e9ada2cefa9..447d38504a2 100644 --- a/src/hotspot/share/gc/z/zVerify.hpp +++ b/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); diff --git a/src/hotspot/share/runtime/thread.cpp b/src/hotspot/share/runtime/thread.cpp index 851e5139f8a..7401629bf94 100644 --- a/src/hotspot/share/runtime/thread.cpp +++ b/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); diff --git a/src/hotspot/share/runtime/thread.hpp b/src/hotspot/share/runtime/thread.hpp index cc431e8c900..7461876f378 100644 --- a/src/hotspot/share/runtime/thread.hpp +++ b/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: