[PATCH] atomic_dec_if_positive sign extension fix

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

 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)

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.

On Mon, 15 Jan 2007 10:09:42 +1100 Paul Mackerras [off-list ref] 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

quoted

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]

quoted

 	__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

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

On Mon, Jan 15, 2007 at 10:09:42AM +1100, Paul Mackerras wrote:

Robert Jennings writes:

quoted

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]

quoted

 	__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

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

On Mon, Jan 15, 2007 at 05:01:12PM +0100, Segher Boessenkool wrote:

quoted

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

quoted

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

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

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

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

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

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