[Adding Paul McKenney as he's the expert.] Akira Yokosawa <akiyks@xxxxxxxxx> wrote: > David Howells wrote: > > Use clear_and_wake_up_bit() rather than something like: > > > > clear_bit_unlock(NETFS_RREQ_IN_PROGRESS, &rreq->flags); > > wake_up_bit(&rreq->flags, NETFS_RREQ_IN_PROGRESS); > > > > as there needs to be a barrier inserted between which is present in > > clear_and_wake_up_bit(). > > If I am reading the kernel-doc comment of clear_bit_unlock() [1, 2]: > > This operation is atomic and provides release barrier semantics. > > correctly, there already seems to be a barrier which should be > good enough. > > [1]: https://www.kernel.org/doc/html/latest/core-api/kernel-api.html#c.clear_bit_unlock > [2]: include/asm-generic/bitops/instrumented-lock.h > > > > > Fixes: 288ace2f57c9 ("netfs: New writeback implementation") > > Fixes: ee4cdf7ba857 ("netfs: Speed up buffered reading") > > So I'm not sure this fixes anything. > > What am I missing? We may need two barriers. You have three things to synchronise: (1) The stuff you did before unlocking. (2) The lock bit. (3) The task state. clear_bit_unlock() interposes a release barrier between (1) and (2). Neither clear_bit_unlock() nor wake_up_bit(), however, necessarily interpose a barrier between (2) and (3). I'm not sure it entirely matters, but it seems that since we have a function that combines the two, we should probably use it - though, granted, it might not actually be a fix. David
