On Tue, 2024-07-16 at 12:06 -0400, 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? So let's just replace rpc_clear_queued() with a call to clear_bit_unlocked(). That still ends up being less expensive than the full memory barrier, doesn't it? -- Trond Myklebust Linux NFS client maintainer, Hammerspace trond.myklebust@xxxxxxxxxxxxxxx
