Re: unlock_buffer() and clear_bit()

[Zoltan Menyhart <Zoltan.Menyhart@bull.net>]

[-- Attachment #1: Type: text/plain, Size: 3143 bytes --]

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...




[-- Attachment #2: buffer.diff --]
[-- Type: text/plain, Size: 352 bytes --]

--- 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);