Re: unlock_buffer() and clear_bit()

[Andrew Morton <akpm@osdl.org>]

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