[PATCH] atomic_dec_if_positive sign extension fix

[PATCH] atomic_dec_if_positive sign extension fix

[Robert Jennings]

Paulus,

Please apply for 2.6.20.  If an atomic counter is explicitly set to a
negative value the atomic_dec_if_positive function will decrement and
store the next smallest value in the atomic counter contrary to it's
intended operation.  

The comparison to determine if the decrement will make the result
negative is done by the "addic." operation which operates on a 64-bit
value.  Since the counter is 32-bit, we need to sign-extend before
the signed 64-bit addition and compare.  Adding 'extsw' just before
'addic.' will correct this.  

Also, I clarify the return value in the comments just to make it clear
that the value returned is always the decremented value, even if that
value is not stored back to the atomic counter.

Signed-off-by: Robert Jennings <rcj@linux.vnet.ibm.com>
---

 atomic.h |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/include/asm-powerpc/atomic.h b/include/asm-powerpc/atomic.h
index 53283e2..6238307 100644
--- a/include/asm-powerpc/atomic.h
+++ b/include/asm-powerpc/atomic.h
@@ -207,7 +207,8 @@ #define atomic_dec_and_test(v)		(atomic_
 
 /*
  * Atomically test *v and decrement if it is greater than 0.
- * The function returns the old value of *v minus 1.
+ * The function returns the old value of *v minus 1, even if 
+ * the atomic variable, v, was not decremented.
  */
 static __inline__ int atomic_dec_if_positive(atomic_t *v)
 {
@@ -216,6 +217,7 @@ static __inline__ int atomic_dec_if_posi
 	__asm__ __volatile__(
 	LWSYNC_ON_SMP
 "1:	lwarx	%0,0,%1		# atomic_dec_if_positive\n\
+	extsw	%0,%0\n\
 	addic.	%0,%0,-1\n\
 	blt-	2f\n"
 	PPC405_ERR77(0,%1)

Re: [PATCH] atomic_dec_if_positive sign extension fix

[Paul Mackerras]

Robert Jennings writes:

> Please apply for 2.6.20.  If an atomic counter is explicitly set to a
> negative value the atomic_dec_if_positive function will decrement and
> store the next smallest value in the atomic counter contrary to it's
> intended operation.  

[snip]

>  	__asm__ __volatile__(
>  	LWSYNC_ON_SMP
>  "1:	lwarx	%0,0,%1		# atomic_dec_if_positive\n\
> +	extsw	%0,%0\n\
>  	addic.	%0,%0,-1\n\
>  	blt-	2f\n"
>  	PPC405_ERR77(0,%1)

NAK: Good fix for 64-bit, but it will break 32-bit.  I think a better
fix would be to use a cmpwi after the lwarx, and use addi rather than
addic..

Paul.

Re: [PATCH] atomic_dec_if_positive sign extension fix

[Olof Johansson]

On Mon, 15 Jan 2007 10:09:42 +1100 Paul Mackerras <paulus@samba.org> wrote:

> NAK: Good fix for 64-bit, but it will break 32-bit.  I think a better
> fix would be to use a cmpwi after the lwarx, and use addi rather than
> addic..

Good point, I forgot about that when I suggested extsw to Rob.

Seems like spinlock.h solves the same problem by using a
__DO_SIGN_EXTEND define instead. Should we keep logic common as much as
possible and use the same method in atomic.h?


-Olof

Re: [PATCH] atomic_dec_if_positive sign extension fix

[Segher Boessenkool]

>> Please apply for 2.6.20.  If an atomic counter is explicitly set to a
>> negative value the atomic_dec_if_positive function will decrement and
>> store the next smallest value in the atomic counter contrary to it's
>> intended operation.
>
> [snip]
>
>>  	__asm__ __volatile__(
>>  	LWSYNC_ON_SMP
>>  "1:	lwarx	%0,0,%1		# atomic_dec_if_positive\n\
>> +	extsw	%0,%0\n\
>>  	addic.	%0,%0,-1\n\
>>  	blt-	2f\n"
>>  	PPC405_ERR77(0,%1)
>
> NAK: Good fix for 64-bit, but it will break 32-bit.  I think a better
> fix would be to use a cmpwi after the lwarx, and use addi rather than
> addic..

Instead of the "extsw %0,%0" you could do "rlwinm %0,%0,0,0,31"
but I guess it's not worth it.

What is this function supposed to do if it gets 0x80000000 as
input btw?  The current code happily makes it 0x7fffffff as
far as I can see?  The "rlwinm" thing would fix that ;-)  (Or
unfix, if the current behaviour is intended).


Segher

Re: [PATCH] atomic_dec_if_positive sign extension fix

[Segher Boessenkool]

> What is this function supposed to do if it gets 0x80000000 as
> input btw?  The current code happily makes it 0x7fffffff as
> far as I can see?  The "rlwinm" thing would fix that ;-)  (Or
> unfix, if the current behaviour is intended).

...except it doesn't change behaviour on 32-bit of
course.  Forget it, do one of the other suggested
things instead please :-)


Segher

Re: [PATCH] atomic_dec_if_positive sign extension fix

[Gabriel Paubert]

On Mon, Jan 15, 2007 at 10:09:42AM +1100, Paul Mackerras wrote:
> Robert Jennings writes:
> 
> > Please apply for 2.6.20.  If an atomic counter is explicitly set to a
> > negative value the atomic_dec_if_positive function will decrement and
> > store the next smallest value in the atomic counter contrary to it's
> > intended operation.  
> 
> [snip]
> 
> >  	__asm__ __volatile__(
> >  	LWSYNC_ON_SMP
> >  "1:	lwarx	%0,0,%1		# atomic_dec_if_positive\n\
> > +	extsw	%0,%0\n\
> >  	addic.	%0,%0,-1\n\
> >  	blt-	2f\n"
> >  	PPC405_ERR77(0,%1)
> 
> NAK: Good fix for 64-bit, but it will break 32-bit.  I think a better
> fix would be to use a cmpwi after the lwarx, and use addi rather than
> addic..
> 

Indeed, this will also fix the bug that 0x8000000 is considered 
positive since the test was done after the decrement, at the cost
of one additional instrucction in 32 bit mode.

It also avoids clobbering the carry, you don't know whether a future
version of GCC will require explicit clobbers for the carry. For now
they are useless: you can specify "xer" in the clobber list AFAIR but 
no pattern can be split between setters and users of the carry in the
machine description.

	Gabriel

Re: [PATCH] atomic_dec_if_positive sign extension fix

[Segher Boessenkool]

> It also avoids clobbering the carry, you don't know whether a future
> version of GCC will require explicit clobbers for the carry. For now
> they are useless: you can specify "xer" in the clobber list AFAIR but
> no pattern can be split between setters and users of the carry in the
> machine description.

You can specify "cc" in the clobber list already, don't use
"xer" unless you're clobbering something else in the XER.


Segher

Re: [PATCH] atomic_dec_if_positive sign extension fix

[Gabriel Paubert]

On Mon, Jan 15, 2007 at 05:01:12PM +0100, Segher Boessenkool wrote:
> >It also avoids clobbering the carry, you don't know whether a future
> >version of GCC will require explicit clobbers for the carry. For now
> >they are useless: you can specify "xer" in the clobber list AFAIR but
> >no pattern can be split between setters and users of the carry in the
> >machine description.
> 
> You can specify "cc" in the clobber list already, don't use
> "xer" unless you're clobbering something else in the XER.
> 

Hmm, I believed "cc" was an alias for cr0.

[Looking at gcc sources]

Ths snippet of gcc/config/rs6000.h confirms this:

  {"cr0",  68}, {"cr1",  69}, {"cr2",  70}, {"cr3",  71},       \
  {"cr4",  72}, {"cr5",  73}, {"cr6",  74}, {"cr7",  75},       \
  {"cc",   68}, {"sp",    1}, {"toc",   2} }

In this file, xer has number 76. So "cc" does not indicate a carry
clobber. There is currently no way to specify a carry clobber, but 
also gcc will never insert an inline asm between settings and uses
of the carry. 

The following comment:

/* PSImode is used for the XER register.  The XER register
   is not used for anything; perhaps it should be deleted,
   except that that would change register numbers.  */
PARTIAL_INT_MODE (SI);

in gcc/config/rs6000-modes.def is actually a bit misleading since the
XER is often clobbered by instructions. But the XER is actually never
exposed to the instruction scheduler: all the patterns that set and use 
the carry are simple lists of machine instructions which will be put in
the final object without modifications (only register number and possibly
immediate value substitution).

	Gabriel

Re: [PATCH] atomic_dec_if_positive sign extension fix

[Segher Boessenkool]

>> You can specify "cc" in the clobber list already, don't use
>> "xer" unless you're clobbering something else in the XER.
>>
>
> Hmm, I believed "cc" was an alias for cr0.

Yes it is and I'm just confused.  Sorry about that.
My excuse is that "addic." would do that to anyone ;-)

> /* PSImode is used for the XER register.  The XER register
>    is not used for anything; perhaps it should be deleted,
>    except that that would change register numbers.  */
> PARTIAL_INT_MODE (SI);
>
> in gcc/config/rs6000-modes.def is actually a bit misleading since the
> XER is often clobbered by instructions. But the XER is actually never
> exposed to the instruction scheduler: all the patterns that set and use
> the carry are simple lists of machine instructions which will be put in
> the final object without modifications (only register number and 
> possibly
> immediate value substitution).

Yes indeed.  Replacing the XER in GCC with a "carry register"
has been on my todo list for years; it is one of the last
things that still require emitting more than one machine insn
at once.


Segher

Re: [PATCH][v2] atomic_dec_if_positive sign extension fix

[Robert Jennings]

Paul,

Here is v.2 for the patch. 
Please apply for 2.6.20.  If an atomic counter is explicitly set to a
negative value the atomic_dec_if_positive function will decrement and
store the next smallest value in the atomic counter contrary to it's
intended operation.

The comparison to determine if the decrement will make the result
negative is done by the "addic." operation which operates on a 64-bit
value.  I've changed the addic to an addi (changing "=&r" to "=&g" in
the process so that r0 isn't used, and addi doesn't become li) and 
done a cmpwi prior to the add.  By comparing prior to the add I can 
pick up the case for 0x80000000, so that it is considered positive.

Also, I clarify the return value in the comments just to make it clear
that the value returned is always the decremented value, even if that
value is not stored back to the atomic counter.

Signed-off-by: Robert Jennings <rcj@linux.vnet.ibm.com>
---

 atomic.h |    8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)


diff --git a/include/asm-powerpc/atomic.h b/include/asm-powerpc/atomic.h
index 53283e2..f6f7e5f 100644
--- a/include/asm-powerpc/atomic.h
+++ b/include/asm-powerpc/atomic.h
@@ -207,7 +207,8 @@ #define atomic_dec_and_test(v)		(atomic_
 
 /*
  * Atomically test *v and decrement if it is greater than 0.
- * The function returns the old value of *v minus 1.
+ * The function returns the old value of *v minus 1, even if
+ * the atomic variable, v, was not decremented.
  */
 static __inline__ int atomic_dec_if_positive(atomic_t *v)
 {
@@ -216,14 +217,15 @@ static __inline__ int atomic_dec_if_posi
 	__asm__ __volatile__(
 	LWSYNC_ON_SMP
 "1:	lwarx	%0,0,%1		# atomic_dec_if_positive\n\
-	addic.	%0,%0,-1\n\
+	cmpwi	%0,1\n\
+	addi	%0,%0,-1\n\
 	blt-	2f\n"
 	PPC405_ERR77(0,%1)
 "	stwcx.	%0,0,%1\n\
 	bne-	1b"
 	ISYNC_ON_SMP
 	"\n\
-2:"	: "=&r" (t)
+2:"	: "=&g" (t)
 	: "r" (&v->counter)
 	: "cc", "memory");
 

Re: [PATCH][v2] atomic_dec_if_positive sign extension fix

[Gabriel Paubert]

On Tue, Jan 16, 2007 at 12:16:05PM -0600, Robert Jennings wrote:
> Paul,
> 
> Here is v.2 for the patch. 
> Please apply for 2.6.20.  If an atomic counter is explicitly set to a
> negative value the atomic_dec_if_positive function will decrement and
> store the next smallest value in the atomic counter contrary to it's
> intended operation.
> 
> The comparison to determine if the decrement will make the result
> negative is done by the "addic." operation which operates on a 64-bit
> value.  I've changed the addic to an addi (changing "=&r" to "=&g" in

I think the proper constraint is "=&b". 

	Gabriel

Re: [PATCH][v3] atomic_dec_if_positive sign extension fix

[Robert Jennings]

Thank you Gabriel for pointing out the correct constraint.

Paul, 

Here is v3 of the patch.  Please apply for 2.6.20.

If an atomic counter is explicitly set to a negative value the
atomic_dec_if_positive function will decrement and store the next smallest
value in the atomic counter contrary to it's intended operation.

The comparison to determine if the decrement will make the result negative
is done by the "addic." operation which operates on a 64-bit value.
I've changed the addic to an addi (changing "=&r" to "=&b" in the
process so that r0 isn't used, and addi doesn't become li) and done a
cmpwi prior to the add.  By comparing prior to the add I can pick up
the case for 0x80000000, so that it is considered positive.

Also, I clarify the return value in the comments just to make it clear
that the value returned is always the decremented value, even if that
value is not stored back to the atomic counter.

Signed-off-by: Robert Jennings <rcj@linux.vnet.ibm.com>
---

 atomic.h |    8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)


Index: a/include/asm-powerpc/atomic.h
===================================================================
--- a/include/asm-powerpc/atomic.h	2006-11-30 08:57:21.000000000 -0600
+++ b/include/asm-powerpc/atomic.h	2007-01-16 15:10:03.000000000 -0600
@@ -207,7 +207,8 @@
 
 /*
  * Atomically test *v and decrement if it is greater than 0.
- * The function returns the old value of *v minus 1.
+ * The function returns the old value of *v minus 1, even if
+ * the atomic variable, v, was not decremented.
  */
 static __inline__ int atomic_dec_if_positive(atomic_t *v)
 {
@@ -216,14 +217,15 @@
 	__asm__ __volatile__(
 	LWSYNC_ON_SMP
 "1:	lwarx	%0,0,%1		# atomic_dec_if_positive\n\
-	addic.	%0,%0,-1\n\
+	cmpwi	%0,1\n\
+	addi	%0,%0,-1\n\
 	blt-	2f\n"
 	PPC405_ERR77(0,%1)
 "	stwcx.	%0,0,%1\n\
 	bne-	1b"
 	ISYNC_ON_SMP
 	"\n\
-2:"	: "=&r" (t)
+2:"	: "=&b" (t)
 	: "r" (&v->counter)
 	: "cc", "memory");