Messages in this thread Patch in this message |  | | | Date | Mon, 27 Mar 2006 10:53:43 +0200 | | From | Zoltan Menyhart <> | | Subject | Re: unlock_buffer() and clear_bit() |
The patch is in the attached file.
Thanks,
Zoltan
Andrew Morton <akpm@osdl.org> wrote:
> Zoltan Menyhart <Zoltan.Menyhart@free.fr> wrote:
>
>>I'm afraid "unlock_buffer()" works incorrectly
>>(at least on the ia64 architecture).
>>
>>As far As I can see, nothing makes it sure that data modifications
>>issued inside the critical section be globally visible before the
>>"BH_Lock" bit gets cleared.
>>
>>When we acquire a resource, we need the "acq" semantics, when we
>>release the resource, we need the "rel" semantics (obviously).
>>
>>Some architectures (at least the ia64) require explicit operations
>>to ensure these semantics, the ordinary "loads" and "stores" do not
>>guarantee any ordering.
>>
>>For the "stand alone" bit operations, these semantics do not matter.
>>They are implemented by use of atomic operations in SMP mode, which
>>operations need to follow either the "acq" semantics or the "rel"
>>semantics (at least the ia64). An arbitrary choice was made to use
>>the "acq" semantics.
>>
>>We use bit operations to implement buffer locking.
>>As a consequence, the requirement of the "rel" semantics, when we
>>release the resource, is not met (at least on the ia64).
>>
>>- Either an "smp_mb__before_clear_bit()" is lacking
>> (if we want to keep the existing definition of "clear_bit()"
>> with its "acq" semantics).
>> Note that "smp_mb__before_clear_bit()" is a bidirectional fence
>> operation on ia64, it is less efficient than the simple
>> "rel" semantics.
>>
>>- Or a new bit clearing service needs to be added that includes
>> the "rel" semantics, say "release_N_clear_bit()"
>> (since we are actually _releasing_ a lock :-))
>>
>>Thanks,
>>
>>Zoltan Menyhart
>>
>>
>>
>>buffer.c:
>>
>>void fastcall unlock_buffer(struct buffer_head *bh)
>>{
>> clear_buffer_locked(bh);
>> smp_mb__after_clear_bit();
>> wake_up_bit(&bh->b_state, BH_Lock);
>>}
>>
>>
>>asm-ia64/bitops.h:
>>
>>/*
>> * clear_bit() has "acquire" semantics.
>> */
>>#define smp_mb__before_clear_bit() smp_mb()
>>#define smp_mb__after_clear_bit() do { /* skip */; } while (0)
>>
>>/**
>> * clear_bit - Clears a bit in memory
>> * @nr: Bit to clear
>> * @addr: Address to start counting from
>> *
>> * clear_bit() is atomic and may not be reordered. However, it does
>> * not contain a memory barrier, so if it is used for locking purposes,
>> * you should call smp_mb__before_clear_bit() and/or
>>smp_mb__after_clear_bit()
>> * in order to ensure changes are visible on other processors.
>> */
>
>
> alpha and powerpc define both of these as smp_mb(). sparc64 is similar,
> but smarter.
>
>
> atomic_ops.txt says:
>
> /* All memory operations before this call will
> * be globally visible before the clear_bit().
> */
> smp_mb__before_clear_bit();
> clear_bit( ... );
>
> /* The clear_bit() will be visible before all
> * subsequent memory operations.
> */
> smp_mb__after_clear_bit();
>
> So it would appear that to make all the modifications which were made to
> the buffer visible after the clear_bit(), yes, we should be using
> smp_mb__before_clear_bit();
>
> unlock_page() does both...
--- save/fs/buffer.c 2006-03-27 10:39:53.000000000 +0200
+++ linux-2.6.16/fs/buffer.c 2006-03-27 10:40:46.000000000 +0200
@@ -78,6 +78,7 @@ EXPORT_SYMBOL(__lock_buffer);
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); |  |