unlock_buffer() and clear_bit()

unlock_buffer() and clear_bit()

[Zoltan Menyhart]

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

Re: unlock_buffer() and clear_bit()

[Andrew Morton]

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

Re: unlock_buffer() and clear_bit()

[Zoltan Menyhart]

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

Re: unlock_buffer() and clear_bit()

[Andrew Morton]

Zoltan Menyhart <Zoltan.Menyhart@bull.net> wrote:
>
> The patch is in the attached file.
> 
> 
> 
> 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);
> 

This is, I think, a rather inefficient thing we're doing there.  For most
architectures, that amounts to:

	mb();
	clear_bit()
	mb();

which is probably more than is needed.  We'd need to get some other
architecture people involved to see if there's a way of improving this, and
unlock_page().

Re: unlock_buffer() and clear_bit()

[Zoltan Menyhart]

Andrew Morton wrote:

> This is, I think, a rather inefficient thing we're doing there.  For most
> architectures, that amounts to:
> 
> 	mb();
> 	clear_bit()
> 	mb();
> 
> which is probably more than is needed.  We'd need to get some other
> architecture people involved to see if there's a way of improving this, and
> unlock_page().

This is why I proposed also:

>>> Or a new bit clearing service needs to be added that includes
>>>   the "rel" semantics, say "release_N_clear_bit()"

The architecture dependent "release_N_clear_bit()" should include what
is necessary for the correct unlocking semantics (and it leaves the freedom
for the "stand alone" bit operations implementations).

Note that "lock_buffer()" works on ia64 "by chance", because all the
atomic bit operations are implemented "by chance" by use of the "acq"
semantics.

I'd like to split the bit operations according to their purposes:
- e.g. "test_and_set_bit_N_acquire()" for lock acquisition
- "test_and_set_bit()", "clear_bit()" as they are today
- "release_N_clear_bit()"...

Thaks,

Zoltan

Re: unlock_buffer() and clear_bit()

[Nick Piggin]

Zoltan Menyhart wrote:
> Andrew Morton wrote:
> 
>> This is, I think, a rather inefficient thing we're doing there.  For most
>> architectures, that amounts to:
>>
>>     mb();
>>     clear_bit()
>>     mb();
>>
>> which is probably more than is needed.  We'd need to get some other
>> architecture people involved to see if there's a way of improving 
>> this, and
>> unlock_page().
> 
> 
> This is why I proposed also:
> 
>>>> Or a new bit clearing service needs to be added that includes
>>>>   the "rel" semantics, say "release_N_clear_bit()"
> 
> 
> The architecture dependent "release_N_clear_bit()" should include what
> is necessary for the correct unlocking semantics (and it leaves the freedom
> for the "stand alone" bit operations implementations).
> 
> Note that "lock_buffer()" works on ia64 "by chance", because all the
> atomic bit operations are implemented "by chance" by use of the "acq"
> semantics.
> 
> I'd like to split the bit operations according to their purposes:
> - e.g. "test_and_set_bit_N_acquire()" for lock acquisition
> - "test_and_set_bit()", "clear_bit()" as they are today
> - "release_N_clear_bit()"...
> 

Now I could be wrong here, but it looks like ia64's
smp_mb__after_clear_bit is broken.

There is nothing in any of the memory barrier, bitop, or atomic
operations that says anything about aquire or release semantics.
The only memory barriers with those semantics currently are those
implied by locking operations.

smp_mb__after_clear_bit() is supposed to, when run directly after
a clear_bit operation, provide the equivalent of an smp_mb().

If you actually need a memory barrier _before_ the clear_bit
(ie. that orders the clear_bit with previous stores) operation,
then you have to issue a seperate barrier completely. Likewise
for a barrier after clear_bit.

The problem with ia64 is that its atomic operations always either
imply aquire or relese semantics, so acquire is simply chosen
arbitrarily. So what I think you should do is make both
smp_mb__before and after_clear_bit issue at least a release
consistency instruction -- combined I think they provide a full
barrier, regardless of their order?

-- 
SUSE Labs, Novell Inc.
Send instant messages to your online friends http://au.messenger.yahoo.com 

Re: unlock_buffer() and clear_bit()

[Nick Piggin]

Nick Piggin wrote:

> smp_mb__after_clear_bit() is supposed to, when run directly after
> a clear_bit operation, provide the equivalent of an smp_mb().
> 

Actually I guess I'm wrong here: it appears that it really should
order before, and after the clear_bit, respectively (looking at
its usage in unlock_page.

So ia64's smp_mb__before_clear_bit needs to be a full barrier, but
__after_clear_bit can be a release. I think?

By the way unlock_page is issuing extra barriers:
Documentation/atomic_ops.txt defines test_and_clear_bit operations
to provide full memory barriers before and after them, so no need
for smp_mb__before/after there.

...ia64 gets these test_and_x_bit operations wrong as well...

-- 
SUSE Labs, Novell Inc.
Send instant messages to your online friends http://au.messenger.yahoo.com