Fix implicit suspend trigger memory ordering am: 0fb2079920 am: 2e553f12ac
Original change: https://android-review.googlesource.com/c/platform/art/+/2982020
Change-Id: If67c151f351824c6279714a4b25784fe332c6767
Signed-off-by: Automerger Merge Worker <android-build-automerger-merge-worker@system.gserviceaccount.com>
diff --git a/runtime/thread.h b/runtime/thread.h
index a59b10a..5dcdb8b 100644
--- a/runtime/thread.h
+++ b/runtime/thread.h
@@ -1429,21 +1429,18 @@
}
// Remove the suspend trigger for this thread by making the suspend_trigger_ TLS value
// equal to a valid pointer.
- // TODO: does this need to atomic? I don't think so.
void RemoveSuspendTrigger() {
- tlsPtr_.suspend_trigger = reinterpret_cast<uintptr_t*>(&tlsPtr_.suspend_trigger);
+ tlsPtr_.suspend_trigger.store(reinterpret_cast<uintptr_t*>(&tlsPtr_.suspend_trigger),
+ std::memory_order_relaxed);
}
// Trigger a suspend check by making the suspend_trigger_ TLS value an invalid pointer.
// The next time a suspend check is done, it will load from the value at this address
// and trigger a SIGSEGV.
- // Only needed if Runtime::implicit_suspend_checks_ is true and fully implemented. It currently
- // is always false. Client code currently just looks at the thread flags directly to determine
- // whether we should suspend, so this call is currently unnecessary.
- void TriggerSuspend() {
- tlsPtr_.suspend_trigger = nullptr;
- }
-
+ // Only needed if Runtime::implicit_suspend_checks_ is true. On some platforms, and in the
+ // interpreter, client code currently just looks at the thread flags directly to determine
+ // whether we should suspend, so this call is not always necessary.
+ void TriggerSuspend() { tlsPtr_.suspend_trigger.store(nullptr, std::memory_order_release); }
// Push an object onto the allocation stack.
bool PushOnThreadLocalAllocationStack(mirror::Object* obj)
@@ -2150,8 +2147,12 @@
ManagedStack managed_stack;
// In certain modes, setting this to 0 will trigger a SEGV and thus a suspend check. It is
- // normally set to the address of itself.
- uintptr_t* suspend_trigger;
+ // normally set to the address of itself. It should be cleared with release semantics to ensure
+ // that prior state changes etc. are visible to any thread that faults as a result.
+ // We assume that the kernel ensures that such changes are then visible to the faulting
+ // thread, even if it is not an acquire load that faults. (Indeed, it seems unlikely that the
+ // ordering semantics associated with the faulting load has any impact.)
+ std::atomic<uintptr_t*> suspend_trigger;
// Every thread may have an associated JNI environment
JNIEnvExt* jni_env;
diff --git a/runtime/thread_list.cc b/runtime/thread_list.cc
index 5e63b27..b841eaa 100644
--- a/runtime/thread_list.cc
+++ b/runtime/thread_list.cc
@@ -1127,9 +1127,6 @@
// Now wait for target to decrement suspend barrier.
std::optional<std::string> failure_info;
if (!is_suspended) {
- // As an experiment, redundantly trigger suspension. TODO: Remove this.
- std::atomic_thread_fence(std::memory_order_seq_cst);
- thread->TriggerSuspend();
failure_info = WaitForSuspendBarrier(&wrapped_barrier.barrier_, tid, attempt_of_4);
if (!failure_info.has_value()) {
is_suspended = true;