[Linux-ia64] cmpxchg patch

[Linux-ia64] cmpxchg patch

[Ray Bryant]

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

It seems to me that the size of the result of cmpxchg(ptr,old,new)
should depend on the size of *ptr and not the size of old.  For example,
see the following:


#include <stdio.h>
#define __SMP__
#define CONFIG_SMP 
#include <asm/system.h>
 
typedef unsigned long long uint64_t;
uint64_t  long lock=0xDEADBEEF00000000;
uint64_t  long result;
 
main()
{
        result = cmpxchg(&lock,0,1);
        if (result == 0) printf("failed!!\n");
        printf("sizeof(0)=%d sizeof(1)=%d\n",sizeof(0),sizeof(1));
        result = cmpxchg(&lock,(uint64_t)0,1);
        if (result != 0) printf("succeeded!!\n");
}

If you run this, you get 

failed!!
succeeded!!

The problem is that cmpxchg() returns 0xDEADBEEF00000000 but then gcc
generates a sxt4 to truncate the result down to an int, which is the
size of old.

This is incorrect as near as I can tell.

Attached is a patch to fix.

-- 
Best Regards,
Ray
-----------------------------------------------
Ray Bryant               SGI
512-453-9679 (work)      512-507-7807 (cell)
raybry@sgi.com           raybry@austin.rr.com
     Computers are like air conditioners.
   They stop working when you open Windows.
-----------------------------------------------

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

--- ./include/asm-ia64/system.h	Sun Dec 23 02:58:36 2001
+++ /usr/people/raybry/tmp/system.h	Sat Feb  2 07:53:26 2002
@@ -345,7 +345,7 @@
 		_r_ = __cmpxchg_called_with_bad_pointer();				\
 		break;									\
 	}										\
-	(__typeof__(old)) _r_;								\
+	(__typeof__(*ptr)) _r_;								\
 })
 
 #define cmpxchg_acq(ptr,o,n)	ia64_cmpxchg("acq", (ptr), (o), (n), sizeof(*(ptr)))

Re: [Linux-ia64] cmpxchg patch

[David Mosberger]

>>>>> On Sat, 02 Feb 2002 10:26:42 -0600, Ray Bryant <raybry@engr.sgi.com> said:

  Ray> It seems to me that the size of the result of
  Ray> cmpxchg(ptr,old,new) should depend on the size of *ptr and not
  Ray> the size of old.

  Ray> Attached is a patch to fix.

Have you actually tried building a kernel with the patch applied?  I
think you'll get errors in atomic.h.  I'm willing to entertain a
better solution, but it needs to be a complete solution.

Thanks,

	--david

Re: [Linux-ia64] cmpxchg patch

[Ray Bryant]

Of course.  I'll give it a try and see what happens.  I hope that
building for a dig machine with 2.4.17 would be a good enough test.  The
easiest machine for me to boot it on would be a SNIA machine though.

Also, I see that previous versions of the in-line asm spinlock code in
IA64 used a proceudure to handle the complicated cases.  However, that
procedure is not part of the source tree at the present time.  I'm
trying to do something similar to create NUMA aware locks that are as
fast (or very nearly so) as the current spin locks, but have the good
NUMA properties of mcs locks.  Any experience you have with creating
spin lock code that calls out to a C procedure to handle the non-common
cases would be appreciated.

David Mosberger wrote:
> 
> >>>>> On Sat, 02 Feb 2002 10:26:42 -0600, Ray Bryant <raybry@engr.sgi.com> said:
> 
>   Ray> It seems to me that the size of the result of
>   Ray> cmpxchg(ptr,old,new) should depend on the size of *ptr and not
>   Ray> the size of old.
> 
>   Ray> Attached is a patch to fix.
> 
> Have you actually tried building a kernel with the patch applied?  I
> think you'll get errors in atomic.h.  I'm willing to entertain a
> better solution, but it needs to be a complete solution.
> 
> Thanks,
> 
>         --david

-- 
Best Regards,
Ray
-----------------------------------------------
Ray Bryant               SGI
512-453-9679 (work)      512-507-7807 (cell)
raybry@sgi.com           raybry@austin.rr.com
     Computers are like air conditioners.
   They stop working when you open Windows.
-----------------------------------------------

Re: [Linux-ia64] cmpxchg patch

[David Mosberger]

>>>>> On Tue, 05 Feb 2002 01:13:47 -0600, Ray Bryant <raybry@engr.sgi.com> said:

  Ray> Also, I see that previous versions of the in-line asm spinlock
  Ray> code in IA64 used a proceudure to handle the complicated cases.

Are you referring to the "#ifdef NEW_LOCK" code?  That's supposed to
be a newer version of the spinlock code, but I haven't had time to
debug and finish it since I started working on it (probably well over
a year ago... ;-/.  I think it makes a lot of sense to handle the
contended case out of line, in a shared procedure, so you can do
back-off etc.

  Ray> However, that procedure is not part of the source tree at the
  Ray> present time.  I'm trying to do something similar to create
  Ray> NUMA aware locks that are as fast (or very nearly so) as the
  Ray> current spin locks, but have the good NUMA properties of mcs
  Ray> locks.  Any experience you have with creating spin lock code
  Ray> that calls out to a C procedure to handle the non-common cases
  Ray> would be appreciated.

The trick is to do this without forcing the "caller" to become a
non-leaf function.  I believe there are a fair number of leaf routines
which play with spinlocks and if they had to do a proper function
call, they would become non-leaf routines and this could have a
significant impact on performance.  Getting this right in the face of
potentially different compilers (and compiler versions) and kernel
unwinding is quite tricky, but it should be possible.  I'd be
interested in seeing experimentation in this area.  Like you say, the
current spinlocks are unlikely to scale well beyond moderately small
numbers of cpus.

	--david