Auto merge of #105590 - solid-rs:patch/kmc-solid/thread-lifecycle-ord…

…ering, r=m-ou-se

kmc-solid: Fix memory ordering in thread operations

Fixes two memory ordering issues in the thread state machine (`ThreadInner::lifecycle`) of the [`*-kmc-solid_*`](https://doc.rust-lang.org/nightly/rustc/platform-support/kmc-solid.html) Tier 3 targets.

1. When detaching a thread that is still running (i.e., the owner updates `lifecycle` first, and the child updates it next), the first update did not synchronize-with the second update, resulting in a data race between the first update and the deallocation of `ThreadInner` by the child thread.
2. When joining on a thread, the joiner has to pass its own task ID to the joinee in order to be woken up later, but in doing so, it did not synchronize-with the read operation, creating possible sequences of execution where the joinee wakes up an incorrect or non-existent task.

Both issue are theoretical and most likely have never manifested in practice because of the stronger guarantees provided by the Arm memory model (particularly due to its barrier-based definition). Compiler optimizations could have subverted this, but the inspection of compiled code did not reveal such optimizations taking place.

std/src/sys/itron/thread.rs

@@ -119,7 +119,7 @@ impl Thread {
 
             let old_lifecycle = inner
                 .lifecycle
-                .swap(LIFECYCLE_EXITED_OR_FINISHED_OR_JOIN_FINALIZE, Ordering::Release);
+                .swap(LIFECYCLE_EXITED_OR_FINISHED_OR_JOIN_FINALIZE, Ordering::AcqRel);
 
             match old_lifecycle {
                 LIFECYCLE_DETACHED => {
@@ -129,9 +129,9 @@ impl Thread {
 
                     // In this case, `*p_inner`'s ownership has been moved to
                     // us, and we are responsible for dropping it. The acquire
-                    // ordering is not necessary because the parent thread made
-                    // no memory access needing synchronization since the call
-                    // to `acre_tsk`.
+                    // ordering ensures that the swap operation that wrote
+                    // `LIFECYCLE_DETACHED` happens-before `Box::from_raw(
+                    // p_inner)`.
                     // Safety: See above.
                     let _ = unsafe { Box::from_raw(p_inner) };
 
@@ -151,6 +151,9 @@ impl Thread {
                     // Since the parent might drop `*inner` and terminate us as
                     // soon as it sees `JOIN_FINALIZE`, the release ordering
                     // must be used in the above `swap` call.
+                    //
+                    // To make the task referred to by `parent_tid` visible, we
+                    // must use the acquire ordering in the above `swap` call.
 
                     // [JOINING → JOIN_FINALIZE]
                     // Wake up the parent task.
@@ -218,11 +221,15 @@ impl Thread {
 
         let current_task = current_task as usize;
 
-        match inner.lifecycle.swap(current_task, Ordering::Acquire) {
+        match inner.lifecycle.swap(current_task, Ordering::AcqRel) {
             LIFECYCLE_INIT => {
                 // [INIT → JOINING]
                 // The child task will transition the state to `JOIN_FINALIZE`
                 // and wake us up.
+                //
+                // To make the task referred to by `current_task` visible from
+                // the child task's point of view, we must use the release
+                // ordering in the above `swap` call.
                 loop {
                     expect_success_aborting(unsafe { abi::slp_tsk() }, &"slp_tsk");
                     // To synchronize with the child task's memory accesses to
@@ -267,15 +274,15 @@ impl Drop for Thread {
         let inner = unsafe { self.p_inner.as_ref() };
 
         // Detach the thread.
-        match inner.lifecycle.swap(LIFECYCLE_DETACHED_OR_JOINED, Ordering::Acquire) {
+        match inner.lifecycle.swap(LIFECYCLE_DETACHED_OR_JOINED, Ordering::AcqRel) {
             LIFECYCLE_INIT => {
                 // [INIT → DETACHED]
                 // When the time comes, the child will figure out that no
                 // one will ever join it.
                 // The ownership of `*p_inner` is moved to the child thread.
-                // However, the release ordering is not necessary because we
-                // made no memory access needing synchronization since the call
-                // to `acre_tsk`.
+                // The release ordering ensures that the above swap operation on
+                // `lifecycle` happens-before the child thread's
+                // `Box::from_raw(p_inner)`.
             }
             LIFECYCLE_FINISHED => {
                 // [FINISHED → JOINED]