PATCH : cmpxchg does not handle int * arguments on LP64

PATCH : cmpxchg does not handle int * arguments on LP64

[Mathieu Lacage]

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

hi,

I have been using the asm-generic/system.h header to implement my
version of arch/xx/include/asm/system.h: it appears that the version of
cmpxchg defined in this generic header does not handle correctly
non-long arguments on an LP64 arch. 

The attached patch appears to work on my port: I suspect that it's not
the right 'fix'. If so, I would be happy to be pointed in the right
direction. The following inline patch was generated against
davem/net-next-2.6/ (and attached to this email since I don't know what
the proper insertion policy is)

diff --git a/include/asm-generic/system.h b/include/asm-generic/system.h
index efa403b..da31909 100644
--- a/include/asm-generic/system.h
+++ b/include/asm-generic/system.h
@@ -136,24 +136,7 @@ unsigned long __xchg(unsigned long x, volatile void *ptr, int size)
 #define xchg(ptr, x) \
 	((__typeof__(*(ptr))) __xchg((unsigned long)(x), (ptr), sizeof(*(ptr))))
 
-static inline unsigned long __cmpxchg(volatile unsigned long *m,
-				      unsigned long old, unsigned long new)
-{
-	unsigned long retval;
-	unsigned long flags;
-
-	local_irq_save(flags);
-	retval = *m;
-	if (retval == old)
-		*m = new;
-	local_irq_restore(flags);
-	return retval;
-}
-
-#define cmpxchg(ptr, o, n)					\
-	((__typeof__(*(ptr))) __cmpxchg((unsigned long *)(ptr), \
-					(unsigned long)(o),	\
-					(unsigned long)(n)))
+#define cmpxchg(ptr, o, n) cmpxchg_local(ptr, o, n)
 
 #endif /* !__ASSEMBLY__ */
 


Mathieu
-- 
Mathieu Lacage <mathieu.lacage@sophia.inria.fr>
Tel: +33 4 9238 5056

[-- Attachment #2: system.patch --]
[-- Type: text/x-patch, Size: 888 bytes --]

diff --git a/include/asm-generic/system.h b/include/asm-generic/system.h
index efa403b..da31909 100644
--- a/include/asm-generic/system.h
+++ b/include/asm-generic/system.h
@@ -136,24 +136,7 @@ unsigned long __xchg(unsigned long x, volatile void *ptr, int size)
 #define xchg(ptr, x) \
 	((__typeof__(*(ptr))) __xchg((unsigned long)(x), (ptr), sizeof(*(ptr))))
 
-static inline unsigned long __cmpxchg(volatile unsigned long *m,
-				      unsigned long old, unsigned long new)
-{
-	unsigned long retval;
-	unsigned long flags;
-
-	local_irq_save(flags);
-	retval = *m;
-	if (retval == old)
-		*m = new;
-	local_irq_restore(flags);
-	return retval;
-}
-
-#define cmpxchg(ptr, o, n)					\
-	((__typeof__(*(ptr))) __cmpxchg((unsigned long *)(ptr), \
-					(unsigned long)(o),	\
-					(unsigned long)(n)))
+#define cmpxchg(ptr, o, n) cmpxchg_local(ptr, o, n)
 
 #endif /* !__ASSEMBLY__ */
 

Re: PATCH : cmpxchg does not handle int * arguments on LP64

[Arnd Bergmann]

On Friday 18 June 2010, Mathieu Lacage wrote:
> I have been using the asm-generic/system.h header to implement my
> version of arch/xx/include/asm/system.h: it appears that the version of
> cmpxchg defined in this generic header does not handle correctly
> non-long arguments on an LP64 arch. 

That looks like a correct observation. It's also true on ILP32
architectures.

I am not sure though if it is actually supposed to operate on other
types, I think there was some disagreement on this in the past.
What code specifically did you find needs to do cmpxchg on non-long
data?

> -
> -#define cmpxchg(ptr, o, n)					\
> -	((__typeof__(*(ptr))) __cmpxchg((unsigned long *)(ptr), \
> -					(unsigned long)(o),	\
> -					(unsigned long)(n)))
> +#define cmpxchg(ptr, o, n) cmpxchg_local(ptr, o, n)

This seems to match what we have in include/asm-generic/cmpxchg.h.
Maybe the best option would be to include that in asm-generic/system.h.

Nobody so far is using asm-generic/system.h, not even tile, which
uses most of the generic headers. This makes it quite likely that
the file is not correct right now. If you think you can improve it,
go wild.

BTW, what architecture are you working on? Is this something you
plan to submit for inclusion soon?

	Arnd

Re: PATCH : cmpxchg does not handle int * arguments on LP64

[Mathieu Lacage]

On Fri, 2010-06-18 at 15:30 +0200, Arnd Bergmann wrote:

> > I have been using the asm-generic/system.h header to implement my
> > version of arch/xx/include/asm/system.h: it appears that the version of
> > cmpxchg defined in this generic header does not handle correctly
> > non-long arguments on an LP64 arch. 
> 
> That looks like a correct observation. It's also true on ILP32
> architectures.
> 
> I am not sure though if it is actually supposed to operate on other
> types, I think there was some disagreement on this in the past.
> What code specifically did you find needs to do cmpxchg on non-long
> data?

asm-generic/atomic.h defines atomic_add_unless to use atomic_cmpxchg
which uses cmpxchg and all of that is supposed to work on struct
atomic_t * which contains an int defined in include/linux/types.h

atomic_add_unless is used through atomic_inc_not_zero to do some
refcounting of sockets in the network stack so it did not take me long
to notice that it appeared to not work.

> > -
> > -#define cmpxchg(ptr, o, n)					\
> > -	((__typeof__(*(ptr))) __cmpxchg((unsigned long *)(ptr), \
> > -					(unsigned long)(o),	\
> > -					(unsigned long)(n)))
> > +#define cmpxchg(ptr, o, n) cmpxchg_local(ptr, o, n)
> 
> This seems to match what we have in include/asm-generic/cmpxchg.h.
> Maybe the best option would be to include that in asm-generic/system.h.

This appears to work, indeed.

> Nobody so far is using asm-generic/system.h, not even tile, which
> uses most of the generic headers. This makes it quite likely that
> the file is not correct right now. If you think you can improve it,
> go wild.
> 
> BTW, what architecture are you working on? Is this something you
> plan to submit for inclusion soon?

I am working on a version of the linux kernel network stack which runs
in userspace as a shared library in a network simulator. My 'arch' port
(named sim) provides glue code to replace all the code used by net/ in
kernel/, lib/, crypto/, fs/ et al. The code is pretty rough and just
started to run udp and netlink sockets yesterday but I could post it if
others are interested.

Mathieu
-- 
Mathieu Lacage <mathieu.lacage@sophia.inria.fr>
Tel: +33 4 9238 5056