On Thu, 22 Aug 2024 at 06:12, NeilBrown <neilb@xxxxxxx> wrote:
>
> So this barrier isn't about the bit. This barrier is about the
> wait_queue. In particular it is about waitqueue_active() call
Correct. The waitqueue_active() call is basically very racy, but it's
also often a huge deal for performance.
In *most* cases, wakeups happen with nobody waiting for them at all,
and taking the waitqueue spinlock just to see that there are no
waiters is hugely expensive and easily causes lots of pointless cache
ping-pong.
So the waitqueue_active() is there to basically say "if there are no
waiters, don't bother doing anything".
But *because* it is done - by definition - without holding the
waitqueue lock, it means that a new waiter might have just checked for
the condition, and is about to add itself to the wait queue.
Now, waiters will then re-check *after* they have added themselves to
the waitqueue too (that's what all the wait_for_event etc stuff does),
and the waiter side will have barriers thanks to the spinlocks on the
waitqueue.
But the waker still needs to make sure it's ordered wrt "I changed the
condition that the waiter is waiting for" and the lockless
waitqueue_active() test.
If waiters and wakers are mutually serialized by some external lock,
there are no races.
But commonly, the waker will just do an atomic op, or maybe it holds a
lock (as a writer) that the waiting side does not hold, and the
waiting side just does a "READ_ONCE()" or a "smp_load_acquire()" or
whatever.
So the waker needs to make sure that its state setting is serialized
with the waiter. If it uses our atomics, then typically a
smp_mb__after_atomic() is appropriate (which often ends up being a
no-op).
But commonly you end up needing a smp_mb(), which serializes whatever
action the waker did with the waker then doing that optimistic
lockless waitqueue_active().
> /* no barrier needs as both waker and waiter are in spin-locked regions */
If all the inode state waiters do indeed hold the lock, then that is fine.
Linus
