Document and fix thread flags memory ordering

Expand mutator_gc_coord.md with a discussion of memory ordering
for state and flags accesses.

Change ReadFlag() to take a non-defaulted memory order argument.
Fix all calls to supply it. The prior memory_order_relaxed default does
not make sense, since it is incorrect in a few places. And even when it
is correct, the arguments behind that are extremely subtle and vary by
call site. We want this to be very explicit and documented.

Document ThreadList::RunCheckpoint memory ordering requirements.
Document the fact that empty checkpoinst should also satisfy those
requirements, but currently do not fully do so.

Temporarily strengthen memory ordering to test kEmptyCheckpointRequest
to partially work around the above issue.

Strengthen some of the loads to test kRunningFlipFunction. We did not
always perform an acquire load before concluding that the flip function
had completed.

Weaken one load in EnsureFlipFunctionStarted for consistency.
In a corner case, reload a flag with acquire ordering in the same
function.

This is the result of a fairly complete pass over the flag setting
and reading logic. The empty checkpoint issue still needs to be more
completley addressed, most probably by switching empty checkpoints to
use the general checkpoint logic. It currently uses an "optimization"
that is very difficult, and probably expensive, to fully correct.

It is not clear whether we have observed bugs caused by this in the
field.

Bug: 382722942
Bug: 380159893
Test: Build and boot aosp
Test: Checked .md file with lowdown
Change-Id: I793eac7ddf80fada36bb4f16ddfaaf317829caa3

1 files changed, 6 insertions, 3 deletions

diff --git a/runtime/thread.h b/runtime/thread.h
index 8ce9854212..3ccf7c3c97 100644
--- a/runtime/thread.h
+++ b/runtime/thread.h
@@ -127,6 +127,7 @@ enum class ThreadFlag : uint32_t {
   kSuspendRequest = 1u << 0,
 
   // Request that the thread do some checkpoint work and then continue.
+  // Only modified while holding thread_suspend_count_lock_ .
   kCheckpointRequest = 1u << 1,
 
   // Request that the thread do empty checkpoint and then continue.
@@ -158,7 +159,7 @@ enum class ThreadFlag : uint32_t {
   // in any case not check for such requests; other clients of SuspendAll might.
   // Prevents a situation in which we are asked to suspend just before we suspend all
   // other threads, and then notice the suspension request and suspend ourselves,
-  // leading to deadlock. Guarded by suspend_count_lock_ .
+  // leading to deadlock. Guarded by thread_suspend_count_lock_ .
   // Should not ever be set when we try to transition to kRunnable.
   // TODO(b/296639267): Generalize use to prevent SuspendAll from blocking
   // in-progress GC.
@@ -1451,8 +1452,10 @@ class EXPORT Thread {
   // Undo the effect of the previous call. Again only invoked by the thread itself.
   void AllowPreMonitorMutexes();
 
-  bool ReadFlag(ThreadFlag flag) const {
-    return GetStateAndFlags(std::memory_order_relaxed).IsFlagSet(flag);
+  // Read a flag with the given memory order. See mutator_gc_coord.md for memory ordering
+  // considerations.
+  bool ReadFlag(ThreadFlag flag, std::memory_order order) const {
+    return GetStateAndFlags(order).IsFlagSet(flag);
   }
 
   void AtomicSetFlag(ThreadFlag flag, std::memory_order order = std::memory_order_seq_cst) {