Re: Fix unlock_buffer() to work the same way as bit_unlock()

[Nick Piggin]

Zoltan Menyhart wrote:

> Christoph Lameter wrote:
>
> > [...]
> >  void fastcall unlock_buffer(struct buffer_head *bh)
> >  {
> > +    smp_mb__before_clear_bit();
> >      clear_buffer_locked(bh);
> > -    smp_mb__after_clear_bit();
> >      wake_up_bit(&bh->b_state, BH_Lock);
> >  }
>
>
> The sequence:
>
>     smp_mb__before_clear_bit();
>     clear_buffer_locked(bh);
>
> is correct (yet not efficient for all architectures).

Yes, this is explicitly documented in the wake_up_bit interface. I
don't really think it needs to be changed, does it? Bill did most
of this work I think, so I've cc'ed him.

> We have got here two unrelated operations: ending a critical section
> and waking up the eventual waiters. What we need is a barrier between
> these two unrelated operations.
> It is not "smp_mb__after_clear_bit()" but a simple "smp_mb()".
> The correct code is:
>
> void fastcall unlock_buffer(struct buffer_head *bh)
> {
>   smp_mb__before_clear_bit();
>   clear_buffer_locked(bh);
>   smp_mb();
>   wake_up_bit(&bh->b_state, BH_Lock);
> }

If you were going to do that, I'd prefer my suggestion.

clear_buffer_locked(); /* clear_bit_unlock */
smp_mb__after_clear_bit_unlock();
wake_up_bit()

Nick

--

Send instant messages to your online friends http://au.messenger.yahoo.com 
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/