Fix implicit suspend trigger memory ordering

Bug: 318779054
Test: TreeHugger
Change-Id: If77e8bdcf067fe8e0b0d12b4548752142ecf1284

2 files changed, 12 insertions, 14 deletions

diff --git a/runtime/thread.h b/runtime/thread.h
index a59b10ae13..5dcdb8b1ed 100644
--- a/runtime/thread.h
+++ b/runtime/thread.h
@@ -1429,21 +1429,18 @@ class EXPORT Thread {
   }
   // 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 @@ class EXPORT Thread {
     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 5e63b27b20..b841eaaccb 100644
--- a/runtime/thread_list.cc
+++ b/runtime/thread_list.cc
@@ -1127,9 +1127,6 @@ bool ThreadList::SuspendThread(Thread* self,
   // 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;