On 16 Jul 2024, at 12:06, Benjamin Coddington wrote: > On 16 Jul 2024, at 10:37, Trond Myklebust wrote: > >> On Tue, 2024-07-16 at 09:44 -0400, Benjamin Coddington wrote: >>> On 16 Jul 2024, at 9:23, Trond Myklebust wrote: >>>> Hmm... Why isn't it sufficient to use smp_mb__after_atomic() here? >>>> That's what clear_and_wake_up_bit() uses in this case. >>> >>> Great question, one that I can't answer with confidence (which is why >>> I >>> solicited advice under the RFC posting). >>> >>> I ended up following the guidance in the comment above wake_up_bit(), >>> since >>> we clear RPC_TASK_RUNNING non-atomically within the queue's >>> spinlock. It >>> states that we'd probably need smp_mb(). >>> >>> However - this race isn't to tk_runstate, its actually to >>> waitqueue_active() >>> being true/false. So - I believe unless we're checking the bit in >>> the >>> waiter's wait_bit_action function, we really only want to look at the >>> race >>> in the terms of making sure the waiter's setting of the >>> wait_queue_head is >>> visible after the waker tests_and_sets RPC_TASK_RUNNING. >>> >> >> My point is that if we were to call >> >> clear_and_wake_up_bit(RPC_TASK_QUEUED, &task->tk_runstate); >> >> then that should be sufficient to resolve the race with >> >> status = out_of_line_wait_on_bit(&task->tk_runstate, >> RPC_TASK_QUEUED, rpc_wait_bit_killable, >> TASK_KILLABLE|TASK_FREEZABLE); >> >> So really, all we should need to do is to add in the >> smp_mb__after_atomic(). This is consistent with the guidance in >> Documentation/atomic_t.txt and Documentation/atomic_bitops.txt. > > Point taken, but isn't clear_bit_unlock() (via clear_and_wake_up_bit()) > providing a release barrier that clear_bit() (via rpc_clear_running(), > spin_unlock()) is not providing? Another few minutes of looking at it.. and now I think you're right.. the smp_mb__after_atomic() is for ordering after rpc_clear_queued(), which I was overlooking in rpc_make_runnable(). I will still kick off my testing, and send along the results. Thanks for the look. Ben
