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

[Nick Piggin <nickpiggin@yahoo.com.au>]

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