On Tue, 25 Jun 2019 at 12:36, Peter Zijlstra <peterz@xxxxxxxxxxxxx> wrote: > On Tue, Jun 25, 2019 at 11:19:35AM +0200, Andreas Gruenbacher wrote: > > > diff --git a/fs/gfs2/glops.c b/fs/gfs2/glops.c > > > index cf4c767005b1..666629ea5da7 100644 > > > --- a/fs/gfs2/glops.c > > > +++ b/fs/gfs2/glops.c > > > @@ -227,6 +227,7 @@ static void gfs2_clear_glop_pending(struct gfs2_inode *ip) > > > return; > > > > > > clear_bit_unlock(GIF_GLOP_PENDING, &ip->i_flags); > > > + smp_mb__after_atomic(); > > > wake_up_bit(&ip->i_flags, GIF_GLOP_PENDING); > > > > This should become clear_and_wake_up_bit as well, right? There are > > several more instances of the same pattern. > > Only if we do as David suggested and make clean_and_wake_up_bit() > provide the RELEASE barrier. (It's clear_and_wake_up_bit, not clean_and_wake_up_bit.) > That is, currently clear_and_wake_up_bit() is > > clear_bit() > smp_mb__after_atomic(); > wake_up_bit(); > > But the above is: > > clear_bit_unlock(); > smp_mb__after_atomic(); > wake_up_bit() > > the difference is that _unlock() uses RELEASE semantics, where > clear_bit() does not. > > The difference is illustrated with something like: > > cond = true; > clear_bit() > smp_mb__after_atomic(); > wake_up_bit(); > > In this case, a remote CPU can first observe the clear_bit() and then > the 'cond = true' store. When we use clear_bit_unlock() this is not > possible, because the RELEASE barrier ensures that everything before, > stays before. Now I'm confused because clear_and_wake_up_bit() in mainline does use clear_bit_unlock(), so it's the exact opposite of what you just said. Thanks, Andreas _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
