Skip to contentSkip to conversation

powerpc: Implement {cmp}xchg for u8 and u16

PATCH Superseded Pan Xinhui 4 participants 22 messages Apr 19, 2016 – Nov 25, 2016 PowerPC

lore
Share & tools
lore

Pan Xinhui

author
#0 lore raw

PATCH v2 1/1 powerpc: Implement {cmp}xchg for u8 and u16

From: Pan Xinhui <[email protected]>

Implement xchg{u8,u16}{local,relaxed}, and
cmpxchg{u8,u16}{,local,acquire,relaxed}.

It works on all ppc.

---
change from V1:
rework totally.

---
 arch/powerpc/include/asm/cmpxchg.h | 83 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 83 insertions(+)
arch/powerpc/include/asm/cmpxchg.h +83 -0
index 44efe73..79a1f45 100644
@@ -7,6 +7,37 @@
 #include <asm/asm-compat.h>
 #include <linux/bug.h>
 
+#ifdef __BIG_ENDIAN
+#define BITOFF_CAL(size, off)	((sizeof(u32) - size - off) * BITS_PER_BYTE)
+#else
+#define BITOFF_CAL(size, off)	(off * BITS_PER_BYTE)
+#endif
+
+static __always_inline unsigned long
+__cmpxchg_u32_local(volatile unsigned int *p, unsigned long old,
+			unsigned long new);
+
+#define __XCHG_GEN(cmp, type, sfx, u32sfx, skip, v)			\
+static __always_inline u32						\
+__##cmp##xchg_##type##sfx(v void *ptr, u32 old, u32 new)		\
+{									\
+	int size = sizeof (type);					\
+	int off = (unsigned long)ptr % sizeof(u32);			\
+	volatile u32 *p = ptr - off;					\
+	int bitoff = BITOFF_CAL(size, off);				\
+	u32 bitmask = ((0x1 << size * BITS_PER_BYTE) - 1) << bitoff;	\
+	u32 oldv, newv;							\
+	u32 ret;							\
+	do {								\
+		oldv = READ_ONCE(*p);					\
+		ret = (oldv & bitmask) >> bitoff;			\
+		if (skip && ret != old)					\
+			break;						\
+		newv = (oldv & ~bitmask) | (new << bitoff);		\
+	} while (__cmpxchg_u32##u32sfx((v void*)p, oldv, newv) != oldv);\
+	return ret;							\
+}
+
 /*
  * Atomic exchange
  *
@@ -14,6 +45,19 @@
  * the previous value stored there.
  */
 
+#define XCHG_GEN(type, sfx, v)						\
+		__XCHG_GEN(_, type, sfx, _local, 0, v)			\
+static __always_inline u32 __xchg_##type##sfx(v void *p, u32 n)	\
+{									\
+	return ___xchg_##type##sfx(p, 0, n);				\
+}
+
+XCHG_GEN(u8, _local, volatile);
+XCHG_GEN(u8, _relaxed, );
+XCHG_GEN(u16, _local, volatile);
+XCHG_GEN(u16, _relaxed, );
+#undef XCHG_GEN
+
 static __always_inline unsigned long
 __xchg_u32_local(volatile void *p, unsigned long val)
 {
@@ -88,6 +132,10 @@ static __always_inline unsigned long
 __xchg_local(volatile void *ptr, unsigned long x, unsigned int size)
 {
 	switch (size) {
+	case 1:
+		return __xchg_u8_local(ptr, x);
+	case 2:
+		return __xchg_u16_local(ptr, x);
 	case 4:
 		return __xchg_u32_local(ptr, x);
 #ifdef CONFIG_PPC64
@@ -103,6 +151,10 @@ static __always_inline unsigned long
 __xchg_relaxed(void *ptr, unsigned long x, unsigned int size)
 {
 	switch (size) {
+	case 1:
+		return __xchg_u8_relaxed(ptr, x);
+	case 2:
+		return __xchg_u16_relaxed(ptr, x);
 	case 4:
 		return __xchg_u32_relaxed(ptr, x);
 #ifdef CONFIG_PPC64
@@ -226,6 +278,21 @@ __cmpxchg_u32_acquire(u32 *p, unsigned long old, unsigned long new)
 	return prev;
 }
 
+
+#define CMPXCHG_GEN(type, sfx, v)				\
+	__XCHG_GEN(cmp, type, sfx, sfx, 1, v)
+
+CMPXCHG_GEN(u8, , volatile);
+CMPXCHG_GEN(u8, _local, volatile);
+CMPXCHG_GEN(u8, _relaxed, );
+CMPXCHG_GEN(u8, _acquire, );
+CMPXCHG_GEN(u16, , volatile);
+CMPXCHG_GEN(u16, _local, volatile);
+CMPXCHG_GEN(u16, _relaxed, );
+CMPXCHG_GEN(u16, _acquire, );
+#undef CMPXCHG_GEN
+#undef __XCHG_GEN
+
 #ifdef CONFIG_PPC64
 static __always_inline unsigned long
 __cmpxchg_u64(volatile unsigned long *p, unsigned long old, unsigned long new)
@@ -316,6 +383,10 @@ __cmpxchg(volatile void *ptr, unsigned long old, unsigned long new,
 	  unsigned int size)
 {
 	switch (size) {
+	case 1:
+		return __cmpxchg_u8(ptr, old, new);
+	case 2:
+		return __cmpxchg_u16(ptr, old, new);
 	case 4:
 		return __cmpxchg_u32(ptr, old, new);
 #ifdef CONFIG_PPC64
@@ -332,6 +403,10 @@ __cmpxchg_local(volatile void *ptr, unsigned long old, unsigned long new,
 	  unsigned int size)
 {
 	switch (size) {
+	case 1:
+		return __cmpxchg_u8_local(ptr, old, new);
+	case 2:
+		return __cmpxchg_u16_local(ptr, old, new);
 	case 4:
 		return __cmpxchg_u32_local(ptr, old, new);
 #ifdef CONFIG_PPC64
@@ -348,6 +423,10 @@ __cmpxchg_relaxed(void *ptr, unsigned long old, unsigned long new,
 		  unsigned int size)
 {
 	switch (size) {
+	case 1:
+		return __cmpxchg_u8_relaxed(ptr, old, new);
+	case 2:
+		return __cmpxchg_u16_relaxed(ptr, old, new);
 	case 4:
 		return __cmpxchg_u32_relaxed(ptr, old, new);
 #ifdef CONFIG_PPC64
@@ -364,6 +443,10 @@ __cmpxchg_acquire(void *ptr, unsigned long old, unsigned long new,
 		  unsigned int size)
 {
 	switch (size) {
+	case 1:
+		return __cmpxchg_u8_acquire(ptr, old, new);
+	case 2:
+		return __cmpxchg_u16_acquire(ptr, old, new);
 	case 4:
 		return __cmpxchg_u32_acquire(ptr, old, new);
 #ifdef CONFIG_PPC64
Signature · 1 line
2.4.3
#1 lore raw

Hi Xinhui,

Quoted 7 lines · Pan Xinhui

From: Pan Xinhui <[email protected]>
=20
Implement xchg{u8,u16}{local,relaxed}, and
cmpxchg{u8,u16}{,local,acquire,relaxed}.
=20
It works on all ppc.
=20

Nice work!

AFAICT, your work doesn't depend on anything that ppc-specific, right? So maybe we can use it as a general approach for a fallback implementation on the archs without u8/u16 atomics. ;-)

Quoted 7 lines

---
change from V1:
rework totally.
---
arch/powerpc/include/asm/cmpxchg.h | 83 ++++++++++++++++++++++++++++++++=

++++++

arch/powerpc/include/as=

Quoted 3 lines
 1 file changed, 83 insertions(+)

=20

arch/powerpc/include/as= renamed from arch/powerpc/include/asm/cmpxchg.h +0 -0

m/cmpxchg.h

arch/powerpc/include/asm/cmpxchg.h

Quoted 9 lines

index 44efe73..79a1f45 100644

arch/powerpc/include/asm/cmpxchg.h +0 -0
@@ -7,6 +7,37 @@
 #include <asm/asm-compat.h>
 #include <linux/bug.h>

=20

arch/powerpc/include/asm/cmpxchg.h +2 -0
+#ifdef __BIG_ENDIAN
+#define BITOFF_CAL(size, off)	((sizeof(u32) - size - off) * BITS_PER_BYT=

E)

Quoted 16 lines
arch/powerpc/include/asm/cmpxchg.h +16 -0
+#else
+#define BITOFF_CAL(size, off)	(off * BITS_PER_BYTE)
+#endif
+
+static __always_inline unsigned long
+__cmpxchg_u32_local(volatile unsigned int *p, unsigned long old,
+			unsigned long new);
+
+#define __XCHG_GEN(cmp, type, sfx, u32sfx, skip, v)			\
+static __always_inline u32						\
+__##cmp##xchg_##type##sfx(v void *ptr, u32 old, u32 new)		\
+{									\
+	int size =3D sizeof (type);					\
+	int off =3D (unsigned long)ptr % sizeof(u32);			\
+	volatile u32 *p =3D ptr - off;					\
+	int bitoff =3D BITOFF_CAL(size, off);				\

arch/powerpc/include/asm/cmpxchg.h

Quoted 10 lines
arch/powerpc/include/asm/cmpxchg.h +10 -0
+	u32 bitmask =3D ((0x1 << size * BITS_PER_BYTE) - 1) << bitoff;	\
+	u32 oldv, newv;							\
+	u32 ret;							\
+	do {								\
+		oldv =3D READ_ONCE(*p);					\
+		ret =3D (oldv & bitmask) >> bitoff;			\
+		if (skip && ret !=3D old)					\
+			break;						\
+		newv =3D (oldv & ~bitmask) | (new << bitoff);		\
+	} while (__cmpxchg_u32##u32sfx((v void*)p, oldv, newv) !=3D oldv);\

Forgive me if this is too paranoid, but I think we can save the READ_ONCE() in the loop if we change the code into the following, because cmpxchg will return the "new" value, if the cmp part fails.

	newv =3D READ_ONCE(*p);

	do {
		oldv =3D newv;
		ret =3D (oldv & bitmask) >> bitoff;
		if (skip && ret !=3D old)
			break;
		newv =3D (oldv & ~bitmask) | (new << bitoff);
		newv =3D __cmpxchg_u32##u32sfx((void *)p, oldv, newv);
	} while(newv !=3D oldv);

arch/powerpc/include/asm/cmpxchg.h line 49

Quoted 12 lines
arch/powerpc/include/asm/cmpxchg.h +3 -0
+	return ret;							\
+}
+
 /*
  * Atomic exchange
  *
@@ -14,6 +45,19 @@
  * the previous value stored there.
  */

=20

arch/powerpc/include/asm/cmpxchg.h +2 -0
+#define XCHG_GEN(type, sfx, v)						\
+		__XCHG_GEN(_, type, sfx, _local, 0, v)			\
                                         ^^^^^^^

This should be sfx, right? Otherwise, all the newly added xchg will call __cmpxchg_u32_local, this will result in wrong ordering guarantees.

arch/powerpc/include/asm/cmpxchg.h line 55

Quoted 6 lines
arch/powerpc/include/asm/cmpxchg.h +6 -0
+static __always_inline u32 __xchg_##type##sfx(v void *p, u32 n)	\
+{									\
+	return ___xchg_##type##sfx(p, 0, n);				\
+}
+
+XCHG_GEN(u8, _local, volatile);

I don't think we need the "volatile" modifier here, because READ_ONCE() and __cmpxchg_u32_* all have "volatile" semantics IIUC, so maybe we can save a paramter for the __XCHG_GEN macro.

Regards,
Boqun

arch/powerpc/include/asm/cmpxchg.h __cmpxchg_u32_acquire(u32 *p, unsigned long old, uns=

Quoted 31 lines
arch/powerpc/include/asm/cmpxchg.h +13 -0
+XCHG_GEN(u8, _relaxed, );
+XCHG_GEN(u16, _local, volatile);
+XCHG_GEN(u16, _relaxed, );
+#undef XCHG_GEN
+
 static __always_inline unsigned long
 __xchg_u32_local(volatile void *p, unsigned long val)
 {
@@ -88,6 +132,10 @@ static __always_inline unsigned long
 __xchg_local(volatile void *ptr, unsigned long x, unsigned int size)
 {
 	switch (size) {
+	case 1:
+		return __xchg_u8_local(ptr, x);
+	case 2:
+		return __xchg_u16_local(ptr, x);
 	case 4:
 		return __xchg_u32_local(ptr, x);
 #ifdef CONFIG_PPC64
@@ -103,6 +151,10 @@ static __always_inline unsigned long
 __xchg_relaxed(void *ptr, unsigned long x, unsigned int size)
 {
 	switch (size) {
+	case 1:
+		return __xchg_u8_relaxed(ptr, x);
+	case 2:
+		return __xchg_u16_relaxed(ptr, x);
 	case 4:
 		return __xchg_u32_relaxed(ptr, x);
 #ifdef CONFIG_PPC64
@@ -226,6 +278,21 @@ __cmpxchg_u32_acquire(u32 *p, unsigned long old, uns=

igned long new)

Quoted 11 lines
arch/powerpc/include/asm/cmpxchg.h +0 -0
 	return prev;
 }

=20

arch/powerpc/include/asm/cmpxchg.h +8 -0
+
+#define CMPXCHG_GEN(type, sfx, v)				\
+	__XCHG_GEN(cmp, type, sfx, sfx, 1, v)
+
+CMPXCHG_GEN(u8, , volatile);
+CMPXCHG_GEN(u8, _local, volatile);
+CMPXCHG_GEN(u8, _relaxed, );
+CMPXCHG_GEN(u8, _acquire, );

arch/powerpc/include/asm/cmpxchg.h __cmpxchg_u32_acquire(u32 *p, unsigned long old, unsigned long new)

Quoted 10 lines
arch/powerpc/include/asm/cmpxchg.h +7 -0
+CMPXCHG_GEN(u16, , volatile);
+CMPXCHG_GEN(u16, _local, volatile);
+CMPXCHG_GEN(u16, _relaxed, );
+CMPXCHG_GEN(u16, _acquire, );
+#undef CMPXCHG_GEN
+#undef __XCHG_GEN
+
 #ifdef CONFIG_PPC64
 static __always_inline unsigned long
 __cmpxchg_u64(volatile unsigned long *p, unsigned long old, unsigned lon=

g new)

arch/powerpc/include/asm/cmpxchg.h __cmpxchg(volatile void *ptr, unsigned long old, uns=

Quoted 1 line
arch/powerpc/include/asm/cmpxchg.h +0 -0
@@ -316,6 +383,10 @@ __cmpxchg(volatile void *ptr, unsigned long old, uns=

igned long new,

arch/powerpc/include/asm/cmpxchg.h __cmpxchg_local(volatile void *ptr, unsigned long ol=

Quoted 11 lines
arch/powerpc/include/asm/cmpxchg.h +4 -0
 	  unsigned int size)
 {
 	switch (size) {
+	case 1:
+		return __cmpxchg_u8(ptr, old, new);
+	case 2:
+		return __cmpxchg_u16(ptr, old, new);
 	case 4:
 		return __cmpxchg_u32(ptr, old, new);
 #ifdef CONFIG_PPC64
@@ -332,6 +403,10 @@ __cmpxchg_local(volatile void *ptr, unsigned long ol=

d, unsigned long new,

arch/powerpc/include/asm/cmpxchg.h __cmpxchg_relaxed(void *ptr, unsigned long old, unsi=

Quoted 11 lines
arch/powerpc/include/asm/cmpxchg.h +4 -0
 	  unsigned int size)
 {
 	switch (size) {
+	case 1:
+		return __cmpxchg_u8_local(ptr, old, new);
+	case 2:
+		return __cmpxchg_u16_local(ptr, old, new);
 	case 4:
 		return __cmpxchg_u32_local(ptr, old, new);
 #ifdef CONFIG_PPC64
@@ -348,6 +423,10 @@ __cmpxchg_relaxed(void *ptr, unsigned long old, unsi=

gned long new,

arch/powerpc/include/asm/cmpxchg.h __cmpxchg_acquire(void *ptr, unsigned long old, unsi=

Quoted 11 lines
arch/powerpc/include/asm/cmpxchg.h +4 -0
 		  unsigned int size)
 {
 	switch (size) {
+	case 1:
+		return __cmpxchg_u8_relaxed(ptr, old, new);
+	case 2:
+		return __cmpxchg_u16_relaxed(ptr, old, new);
 	case 4:
 		return __cmpxchg_u32_relaxed(ptr, old, new);
 #ifdef CONFIG_PPC64
@@ -364,6 +443,10 @@ __cmpxchg_acquire(void *ptr, unsigned long old, unsi=

gned long new,

Quoted 13 lines
arch/powerpc/include/asm/cmpxchg.h +4 -1
 		  unsigned int size)
 {
 	switch (size) {
+	case 1:
+		return __cmpxchg_u8_acquire(ptr, old, new);
+	case 2:
+		return __cmpxchg_u16_acquire(ptr, old, new);
 	case 4:
 		return __cmpxchg_u32_acquire(ptr, old, new);
 #ifdef CONFIG_PPC64
--=20

2.4.3
=20

author
#2 lore raw

Hello, boqun

Quoted 5 lines · Boqun Feng

Hi Xinhui,

Quoted 2 lines · Pan Xinhui

From: Pan Xinhui <[email protected]>

Quoted 8 lines
Quoted 5 lines

Implement xchg{u8,u16}{local,relaxed}, and
cmpxchg{u8,u16}{,local,acquire,relaxed}.

It works on all ppc.

Nice work!

thank you.

Quoted 65 lines

AFAICT, your work doesn't depend on anything that ppc-specific, right? So maybe we can use it as a general approach for a fallback implementation on the archs without u8/u16 atomics. ;-)

Quoted 35 lines

---
change from V1:
rework totally.

---
 arch/powerpc/include/asm/cmpxchg.h | 83 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 83 insertions(+)
arch/powerpc/include/asm/cmpxchg.h +18 -0
index 44efe73..79a1f45 100644
@@ -7,6 +7,37 @@
 #include <asm/asm-compat.h>
 #include <linux/bug.h>
 
+#ifdef __BIG_ENDIAN
+#define BITOFF_CAL(size, off)	((sizeof(u32) - size - off) * BITS_PER_BYTE)
+#else
+#define BITOFF_CAL(size, off)	(off * BITS_PER_BYTE)
+#endif
+
+static __always_inline unsigned long
+__cmpxchg_u32_local(volatile unsigned int *p, unsigned long old,
+			unsigned long new);
+
+#define __XCHG_GEN(cmp, type, sfx, u32sfx, skip, v)			\
+static __always_inline u32						\
+__##cmp##xchg_##type##sfx(v void *ptr, u32 old, u32 new)		\
+{									\
+	int size = sizeof (type);					\
+	int off = (unsigned long)ptr % sizeof(u32);			\
+	volatile u32 *p = ptr - off;					\
+	int bitoff = BITOFF_CAL(size, off);				\

arch/powerpc/include/asm/cmpxchg.h line 37

Quoted 10 lines
arch/powerpc/include/asm/cmpxchg.h +10 -0
+	u32 bitmask = ((0x1 << size * BITS_PER_BYTE) - 1) << bitoff;	\
+	u32 oldv, newv;							\
+	u32 ret;							\
+	do {								\
+		oldv = READ_ONCE(*p);					\
+		ret = (oldv & bitmask) >> bitoff;			\
+		if (skip && ret != old)					\
+			break;						\
+		newv = (oldv & ~bitmask) | (new << bitoff);		\
+	} while (__cmpxchg_u32##u32sfx((v void*)p, oldv, newv) != oldv);\

Forgive me if this is too paranoid, but I think we can save the READ_ONCE() in the loop if we change the code into the following, because cmpxchg will return the "new" value, if the cmp part fails.

	newv = READ_ONCE(*p);

	do {
		oldv = newv;
		ret = (oldv & bitmask) >> bitoff;
		if (skip && ret != old)
			break;
		newv = (oldv & ~bitmask) | (new << bitoff);
		newv = __cmpxchg_u32##u32sfx((void *)p, oldv, newv);
	} while(newv != oldv);
Quoted 2 lines

arch/powerpc/include/asm/cmpxchg.h line 39

Quoted 2 lines
arch/powerpc/include/asm/cmpxchg.h +2 -0
+	return ret;							\
+}

a little optimization. Patch V3 will include your code, thanks.

Quoted 8 lines
Quoted 8 lines
arch/powerpc/include/asm/cmpxchg.h +1 -0
+
 /*
  * Atomic exchange
  *
@@ -14,6 +45,19 @@
  * the previous value stored there.
  */
 
Quoted 7 lines

arch/powerpc/include/asm/cmpxchg.h line 49

Quoted 2 lines
arch/powerpc/include/asm/cmpxchg.h +2 -0
+#define XCHG_GEN(type, sfx, v)						\
+		__XCHG_GEN(_, type, sfx, _local, 0, v)			\
                                         ^^^^^^^

This should be sfx, right? Otherwise, all the newly added xchg will call __cmpxchg_u32_local, this will result in wrong ordering guarantees.

I mean that. But I will think of the ordering issue for a while. :)

Quoted 11 lines

arch/powerpc/include/asm/cmpxchg.h line 55

Quoted 6 lines
arch/powerpc/include/asm/cmpxchg.h +6 -0
+static __always_inline u32 __xchg_##type##sfx(v void *p, u32 n)	\
+{									\
+	return ___xchg_##type##sfx(p, 0, n);				\
+}
+
+XCHG_GEN(u8, _local, volatile);

I don't think we need the "volatile" modifier here, because READ_ONCE() and __cmpxchg_u32_* all have "volatile" semantics IIUC, so maybe we can save a paramter for the __XCHG_GEN macro.

such cleanup work can be done in separated patch. Here I just make the compiler happy.

thanks
xinhui

Quoted 103 lines

Regards,
Boqun

Quoted 99 lines
arch/powerpc/include/asm/cmpxchg.h +44 -0
+XCHG_GEN(u8, _relaxed, );
+XCHG_GEN(u16, _local, volatile);
+XCHG_GEN(u16, _relaxed, );
+#undef XCHG_GEN
+
 static __always_inline unsigned long
 __xchg_u32_local(volatile void *p, unsigned long val)
 {
@@ -88,6 +132,10 @@ static __always_inline unsigned long
 __xchg_local(volatile void *ptr, unsigned long x, unsigned int size)
 {
 	switch (size) {
+	case 1:
+		return __xchg_u8_local(ptr, x);
+	case 2:
+		return __xchg_u16_local(ptr, x);
 	case 4:
 		return __xchg_u32_local(ptr, x);
 #ifdef CONFIG_PPC64
@@ -103,6 +151,10 @@ static __always_inline unsigned long
 __xchg_relaxed(void *ptr, unsigned long x, unsigned int size)
 {
 	switch (size) {
+	case 1:
+		return __xchg_u8_relaxed(ptr, x);
+	case 2:
+		return __xchg_u16_relaxed(ptr, x);
 	case 4:
 		return __xchg_u32_relaxed(ptr, x);
 #ifdef CONFIG_PPC64
@@ -226,6 +278,21 @@ __cmpxchg_u32_acquire(u32 *p, unsigned long old, unsigned long new)
 	return prev;
 }
 
+
+#define CMPXCHG_GEN(type, sfx, v)				\
+	__XCHG_GEN(cmp, type, sfx, sfx, 1, v)
+
+CMPXCHG_GEN(u8, , volatile);
+CMPXCHG_GEN(u8, _local, volatile);
+CMPXCHG_GEN(u8, _relaxed, );
+CMPXCHG_GEN(u8, _acquire, );
+CMPXCHG_GEN(u16, , volatile);
+CMPXCHG_GEN(u16, _local, volatile);
+CMPXCHG_GEN(u16, _relaxed, );
+CMPXCHG_GEN(u16, _acquire, );
+#undef CMPXCHG_GEN
+#undef __XCHG_GEN
+
 #ifdef CONFIG_PPC64
 static __always_inline unsigned long
 __cmpxchg_u64(volatile unsigned long *p, unsigned long old, unsigned long new)
@@ -316,6 +383,10 @@ __cmpxchg(volatile void *ptr, unsigned long old, unsigned long new,
 	  unsigned int size)
 {
 	switch (size) {
+	case 1:
+		return __cmpxchg_u8(ptr, old, new);
+	case 2:
+		return __cmpxchg_u16(ptr, old, new);
 	case 4:
 		return __cmpxchg_u32(ptr, old, new);
 #ifdef CONFIG_PPC64
@@ -332,6 +403,10 @@ __cmpxchg_local(volatile void *ptr, unsigned long old, unsigned long new,
 	  unsigned int size)
 {
 	switch (size) {
+	case 1:
+		return __cmpxchg_u8_local(ptr, old, new);
+	case 2:
+		return __cmpxchg_u16_local(ptr, old, new);
 	case 4:
 		return __cmpxchg_u32_local(ptr, old, new);
 #ifdef CONFIG_PPC64
@@ -348,6 +423,10 @@ __cmpxchg_relaxed(void *ptr, unsigned long old, unsigned long new,
 		  unsigned int size)
 {
 	switch (size) {
+	case 1:
+		return __cmpxchg_u8_relaxed(ptr, old, new);
+	case 2:
+		return __cmpxchg_u16_relaxed(ptr, old, new);
 	case 4:
 		return __cmpxchg_u32_relaxed(ptr, old, new);
 #ifdef CONFIG_PPC64
@@ -364,6 +443,10 @@ __cmpxchg_acquire(void *ptr, unsigned long old, unsigned long new,
 		  unsigned int size)
 {
 	switch (size) {
+	case 1:
+		return __cmpxchg_u8_acquire(ptr, old, new);
+	case 2:
+		return __cmpxchg_u16_acquire(ptr, old, new);
 	case 4:
 		return __cmpxchg_u32_acquire(ptr, old, new);
 #ifdef CONFIG_PPC64
Signature · 1 line
2.4.3
author
#3 lore raw

PATCH v3 1/1 powerpc: Implement {cmp}xchg for u8 and u16

In reply to Pan Xinhui

From: Pan Xinhui <[email protected]>

Implement xchg{u8,u16}{local,relaxed}, and
cmpxchg{u8,u16}{,local,acquire,relaxed}.

It works on all ppc.
The basic idea is from commit 3226aad81aa6 ("sh: support 1 and 2 byte xchg")

---
change from v2:
in the do{}while(), we save one load and use corresponding cmpxchg suffix. Also add corresponding __cmpxchg_u32 function declaration in the __XCHG_GEN
change from V1:
rework totally.

---
 arch/powerpc/include/asm/cmpxchg.h | 83 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 83 insertions(+)
arch/powerpc/include/asm/cmpxchg.h +83 -0
index 44efe73..2aec04e 100644
@@ -7,6 +7,38 @@
 #include <asm/asm-compat.h>
 #include <linux/bug.h>
 
+#ifdef __BIG_ENDIAN
+#define BITOFF_CAL(size, off)	((sizeof(u32) - size - off) * BITS_PER_BYTE)
+#else
+#define BITOFF_CAL(size, off)	(off * BITS_PER_BYTE)
+#endif
+
+#define __XCHG_GEN(cmp, type, sfx, skip, v)				\
+static __always_inline unsigned long					\
+__cmpxchg_u32##sfx(v unsigned int *p, unsigned long old,		\
+			 unsigned long new);				\
+static __always_inline u32						\
+__##cmp##xchg_##type##sfx(v void *ptr, u32 old, u32 new)		\
+{									\
+	int size = sizeof (type);					\
+	int off = (unsigned long)ptr % sizeof(u32);			\
+	volatile u32 *p = ptr - off;					\
+	int bitoff = BITOFF_CAL(size, off);				\
+	u32 bitmask = ((0x1 << size * BITS_PER_BYTE) - 1) << bitoff;	\
+	u32 oldv, newv, tmp;						\
+	u32 ret;							\
+	oldv = READ_ONCE(*p);						\
+	do {								\
+		ret = (oldv & bitmask) >> bitoff;			\
+		if (skip && ret != old)					\
+			break;						\
+		newv = (oldv & ~bitmask) | (new << bitoff);		\
+		tmp = oldv;						\
+		oldv = __cmpxchg_u32##sfx((v u32*)p, oldv, newv);	\
+	} while (tmp != oldv);						\
+	return ret;							\
+}
+
 /*
  * Atomic exchange
  *
@@ -14,6 +46,19 @@
  * the previous value stored there.
  */
 
+#define XCHG_GEN(type, sfx, v)						\
+		__XCHG_GEN(_, type, sfx, 0, v)				\
+static __always_inline u32 __xchg_##type##sfx(v void *p, u32 n)		\
+{									\
+	return ___xchg_##type##sfx(p, 0, n);				\
+}
+
+XCHG_GEN(u8, _local, volatile);
+XCHG_GEN(u8, _relaxed, );
+XCHG_GEN(u16, _local, volatile);
+XCHG_GEN(u16, _relaxed, );
+#undef XCHG_GEN
+
 static __always_inline unsigned long
 __xchg_u32_local(volatile void *p, unsigned long val)
 {
@@ -88,6 +133,10 @@ static __always_inline unsigned long
 __xchg_local(volatile void *ptr, unsigned long x, unsigned int size)
 {
 	switch (size) {
+	case 1:
+		return __xchg_u8_local(ptr, x);
+	case 2:
+		return __xchg_u16_local(ptr, x);
 	case 4:
 		return __xchg_u32_local(ptr, x);
 #ifdef CONFIG_PPC64
@@ -103,6 +152,10 @@ static __always_inline unsigned long
 __xchg_relaxed(void *ptr, unsigned long x, unsigned int size)
 {
 	switch (size) {
+	case 1:
+		return __xchg_u8_relaxed(ptr, x);
+	case 2:
+		return __xchg_u16_relaxed(ptr, x);
 	case 4:
 		return __xchg_u32_relaxed(ptr, x);
 #ifdef CONFIG_PPC64
@@ -131,6 +184,20 @@ __xchg_relaxed(void *ptr, unsigned long x, unsigned int size)
  * and return the old value of *p.
  */
 
+#define CMPXCHG_GEN(type, sfx, v)				\
+	__XCHG_GEN(cmp, type, sfx, 1, v)
+
+CMPXCHG_GEN(u8, , volatile);
+CMPXCHG_GEN(u8, _local,	volatile);
+CMPXCHG_GEN(u8, _relaxed, );
+CMPXCHG_GEN(u8, _acquire, );
+CMPXCHG_GEN(u16, , volatile);
+CMPXCHG_GEN(u16, _local, volatile);
+CMPXCHG_GEN(u16, _relaxed, );
+CMPXCHG_GEN(u16, _acquire, );
+#undef CMPXCHG_GEN
+#undef __XCHG_GEN
+
 static __always_inline unsigned long
 __cmpxchg_u32(volatile unsigned int *p, unsigned long old, unsigned long new)
 {
@@ -316,6 +383,10 @@ __cmpxchg(volatile void *ptr, unsigned long old, unsigned long new,
 	  unsigned int size)
 {
 	switch (size) {
+	case 1:
+		return __cmpxchg_u8(ptr, old, new);
+	case 2:
+		return __cmpxchg_u16(ptr, old, new);
 	case 4:
 		return __cmpxchg_u32(ptr, old, new);
 #ifdef CONFIG_PPC64
@@ -332,6 +403,10 @@ __cmpxchg_local(volatile void *ptr, unsigned long old, unsigned long new,
 	  unsigned int size)
 {
 	switch (size) {
+	case 1:
+		return __cmpxchg_u8_local(ptr, old, new);
+	case 2:
+		return __cmpxchg_u16_local(ptr, old, new);
 	case 4:
 		return __cmpxchg_u32_local(ptr, old, new);
 #ifdef CONFIG_PPC64
@@ -348,6 +423,10 @@ __cmpxchg_relaxed(void *ptr, unsigned long old, unsigned long new,
 		  unsigned int size)
 {
 	switch (size) {
+	case 1:
+		return __cmpxchg_u8_relaxed(ptr, old, new);
+	case 2:
+		return __cmpxchg_u16_relaxed(ptr, old, new);
 	case 4:
 		return __cmpxchg_u32_relaxed(ptr, old, new);
 #ifdef CONFIG_PPC64
@@ -364,6 +443,10 @@ __cmpxchg_acquire(void *ptr, unsigned long old, unsigned long new,
 		  unsigned int size)
 {
 	switch (size) {
+	case 1:
+		return __cmpxchg_u8_acquire(ptr, old, new);
+	case 2:
+		return __cmpxchg_u16_acquire(ptr, old, new);
 	case 4:
 		return __cmpxchg_u32_acquire(ptr, old, new);
 #ifdef CONFIG_PPC64
Signature · 1 line
2.4.3
#4 lore raw
Quoted 15 lines · Pan Xinhui
arch/powerpc/include/asm/cmpxchg.h +15 -0
+#define __XCHG_GEN(cmp, type, sfx, skip, v)				\
+static __always_inline unsigned long					\
+__cmpxchg_u32##sfx(v unsigned int *p, unsigned long old,		\
+			 unsigned long new);				\
+static __always_inline u32						\
+__##cmp##xchg_##type##sfx(v void *ptr, u32 old, u32 new)		\
+{									\
+	int size = sizeof (type);					\
+	int off = (unsigned long)ptr % sizeof(u32);			\
+	volatile u32 *p = ptr - off;					\
+	int bitoff = BITOFF_CAL(size, off);				\
+	u32 bitmask = ((0x1 << size * BITS_PER_BYTE) - 1) << bitoff;	\
+	u32 oldv, newv, tmp;						\
+	u32 ret;							\
+	oldv = READ_ONCE(*p);						\

arch/powerpc/include/asm/cmpxchg.h line 40

Quoted 10 lines
arch/powerpc/include/asm/cmpxchg.h +10 -0
+	do {								\
+		ret = (oldv & bitmask) >> bitoff;			\
+		if (skip && ret != old)					\
+			break;						\
+		newv = (oldv & ~bitmask) | (new << bitoff);		\
+		tmp = oldv;						\
+		oldv = __cmpxchg_u32##sfx((v u32*)p, oldv, newv);	\
+	} while (tmp != oldv);						\
+	return ret;							\
+}

So for an LL/SC based arch using cmpxchg() like that is sub-optimal.

Why did you choose to write it entirely in C?

author
#5 lore raw
Quoted 30 lines · Peter Zijlstra
Quoted 15 lines · Pan Xinhui
arch/powerpc/include/asm/cmpxchg.h +15 -0
+#define __XCHG_GEN(cmp, type, sfx, skip, v)				\
+static __always_inline unsigned long					\
+__cmpxchg_u32##sfx(v unsigned int *p, unsigned long old,		\
+			 unsigned long new);				\
+static __always_inline u32						\
+__##cmp##xchg_##type##sfx(v void *ptr, u32 old, u32 new)		\
+{									\
+	int size = sizeof (type);					\
+	int off = (unsigned long)ptr % sizeof(u32);			\
+	volatile u32 *p = ptr - off;					\
+	int bitoff = BITOFF_CAL(size, off);				\
+	u32 bitmask = ((0x1 << size * BITS_PER_BYTE) - 1) << bitoff;	\
+	u32 oldv, newv, tmp;						\
+	u32 ret;							\
+	oldv = READ_ONCE(*p);						\

arch/powerpc/include/asm/cmpxchg.h line 40

Quoted 10 lines
arch/powerpc/include/asm/cmpxchg.h +10 -0
+	do {								\
+		ret = (oldv & bitmask) >> bitoff;			\
+		if (skip && ret != old)					\
+			break;						\
+		newv = (oldv & ~bitmask) | (new << bitoff);		\
+		tmp = oldv;						\
+		oldv = __cmpxchg_u32##sfx((v u32*)p, oldv, newv);	\
+	} while (tmp != oldv);						\
+	return ret;							\
+}

So for an LL/SC based arch using cmpxchg() like that is sub-optimal.

Quoted 2 lines

Why did you choose to write it entirely in C?

yes, you are right. more load/store will be done in C code.
However such xchg_u8/u16 is just used by qspinlock now. and I did not see any performance regression. So just wrote in C, for simple. :)

Of course I have done xchg tests.
we run code just like xchg((u8*)&v, j++); in several threads. and the result is,

[  768.374264] use time[1550072]ns in xchg_u8_asm
[  768.377102] use time[2826802]ns in xchg_u8_c

I think this is because there is one more load in C. If possible, we can move such code in asm-generic/.

thanks
xinhui

#6 lore raw
Quoted 33 lines · Pan Xinhui
Quoted 30 lines · Peter Zijlstra
Quoted 15 lines · Pan Xinhui
arch/powerpc/include/asm/cmpxchg.h +15 -0
+#define __XCHG_GEN(cmp, type, sfx, skip, v)				\
+static __always_inline unsigned long					\
+__cmpxchg_u32##sfx(v unsigned int *p, unsigned long old,		\
+			 unsigned long new);				\
+static __always_inline u32						\
+__##cmp##xchg_##type##sfx(v void *ptr, u32 old, u32 new)		\
+{									\
+	int size = sizeof (type);					\
+	int off = (unsigned long)ptr % sizeof(u32);			\
+	volatile u32 *p = ptr - off;					\
+	int bitoff = BITOFF_CAL(size, off);				\
+	u32 bitmask = ((0x1 << size * BITS_PER_BYTE) - 1) << bitoff;	\
+	u32 oldv, newv, tmp;						\
+	u32 ret;							\
+	oldv = READ_ONCE(*p);						\

arch/powerpc/include/asm/cmpxchg.h line 40

Quoted 10 lines
arch/powerpc/include/asm/cmpxchg.h +10 -0
+	do {								\
+		ret = (oldv & bitmask) >> bitoff;			\
+		if (skip && ret != old)					\
+			break;						\
+		newv = (oldv & ~bitmask) | (new << bitoff);		\
+		tmp = oldv;						\
+		oldv = __cmpxchg_u32##sfx((v u32*)p, oldv, newv);	\
+	} while (tmp != oldv);						\
+	return ret;							\
+}

So for an LL/SC based arch using cmpxchg() like that is sub-optimal.

Quoted 2 lines

Why did you choose to write it entirely in C?

Quoted 8 lines

yes, you are right. more load/store will be done in C code.
However such xchg_u8/u16 is just used by qspinlock now. and I did not see any performance regression. So just wrote in C, for simple. :)

Of course I have done xchg tests.
we run code just like xchg((u8*)&v, j++); in several threads. and the result is,

[  768.374264] use time[1550072]ns in xchg_u8_asm

How was xchg_u8_asm() implemented, using lbarx or using a 32bit ll/sc loop with shifting and masking in it?

Regards,
Boqun

Quoted 8 lines
[  768.377102] use time[2826802]ns in xchg_u8_c

I think this is because there is one more load in C. If possible, we can move such code in asm-generic/.

thanks
xinhui

author
#7 lore raw
Quoted 38 lines · Boqun Feng
Quoted 37 lines · Pan Xinhui
Quoted 30 lines · Peter Zijlstra
Quoted 15 lines · Pan Xinhui
arch/powerpc/include/asm/cmpxchg.h +15 -0
+#define __XCHG_GEN(cmp, type, sfx, skip, v)				\
+static __always_inline unsigned long					\
+__cmpxchg_u32##sfx(v unsigned int *p, unsigned long old,		\
+			 unsigned long new);				\
+static __always_inline u32						\
+__##cmp##xchg_##type##sfx(v void *ptr, u32 old, u32 new)		\
+{									\
+	int size = sizeof (type);					\
+	int off = (unsigned long)ptr % sizeof(u32);			\
+	volatile u32 *p = ptr - off;					\
+	int bitoff = BITOFF_CAL(size, off);				\
+	u32 bitmask = ((0x1 << size * BITS_PER_BYTE) - 1) << bitoff;	\
+	u32 oldv, newv, tmp;						\
+	u32 ret;							\
+	oldv = READ_ONCE(*p);						\

arch/powerpc/include/asm/cmpxchg.h line 40

Quoted 10 lines
arch/powerpc/include/asm/cmpxchg.h +10 -0
+	do {								\
+		ret = (oldv & bitmask) >> bitoff;			\
+		if (skip && ret != old)					\
+			break;						\
+		newv = (oldv & ~bitmask) | (new << bitoff);		\
+		tmp = oldv;						\
+		oldv = __cmpxchg_u32##sfx((v u32*)p, oldv, newv);	\
+	} while (tmp != oldv);						\
+	return ret;							\
+}

So for an LL/SC based arch using cmpxchg() like that is sub-optimal.

Quoted 2 lines

Why did you choose to write it entirely in C?

yes, you are right. more load/store will be done in C code.
However such xchg_u8/u16 is just used by qspinlock now. and I did not see any performance regression. So just wrote in C, for simple. :)

Quoted 8 lines
Quoted 4 lines

Of course I have done xchg tests.
we run code just like xchg((u8*)&v, j++); in several threads. and the result is,

[  768.374264] use time[1550072]ns in xchg_u8_asm

How was xchg_u8_asm() implemented, using lbarx or using a 32bit ll/sc loop with shifting and masking in it?

yes, using 32bit ll/sc loops.

looks like:

        __asm__ __volatile__(
"1:     lwarx   %0,0,%3\n"
"       and %1,%0,%5\n"
"       or %1,%1,%4\n"
       PPC405_ERR77(0,%2)
"       stwcx.  %1,0,%3\n"
"       bne-    1b"
        : "=&r" (_oldv), "=&r" (tmp), "+m" (*(volatile unsigned int *)_p)
        : "r" (_p), "r" (_newv), "r" (_oldv_mask)
        : "cc", "memory");
Quoted 11 lines

Regards,
Boqun

Quoted 8 lines
[  768.377102] use time[2826802]ns in xchg_u8_c

I think this is because there is one more load in C. If possible, we can move such code in asm-generic/.

thanks
xinhui

#8 lore raw
Quoted 50 lines · Pan Xinhui
Quoted 38 lines · Boqun Feng
Quoted 37 lines · Pan Xinhui
Quoted 30 lines · Peter Zijlstra
Quoted 15 lines · Pan Xinhui
arch/powerpc/include/asm/cmpxchg.h +15 -0
+#define __XCHG_GEN(cmp, type, sfx, skip, v)				\
+static __always_inline unsigned long					\
+__cmpxchg_u32##sfx(v unsigned int *p, unsigned long old,		\
+			 unsigned long new);				\
+static __always_inline u32						\
+__##cmp##xchg_##type##sfx(v void *ptr, u32 old, u32 new)		\
+{									\
+	int size = sizeof (type);					\
+	int off = (unsigned long)ptr % sizeof(u32);			\
+	volatile u32 *p = ptr - off;					\
+	int bitoff = BITOFF_CAL(size, off);				\
+	u32 bitmask = ((0x1 << size * BITS_PER_BYTE) - 1) << bitoff;	\
+	u32 oldv, newv, tmp;						\
+	u32 ret;							\
+	oldv = READ_ONCE(*p);						\

arch/powerpc/include/asm/cmpxchg.h line 40

Quoted 10 lines
arch/powerpc/include/asm/cmpxchg.h +10 -0
+	do {								\
+		ret = (oldv & bitmask) >> bitoff;			\
+		if (skip && ret != old)					\
+			break;						\
+		newv = (oldv & ~bitmask) | (new << bitoff);		\
+		tmp = oldv;						\
+		oldv = __cmpxchg_u32##sfx((v u32*)p, oldv, newv);	\
+	} while (tmp != oldv);						\
+	return ret;							\
+}

So for an LL/SC based arch using cmpxchg() like that is sub-optimal.

Quoted 2 lines

Why did you choose to write it entirely in C?

yes, you are right. more load/store will be done in C code.
However such xchg_u8/u16 is just used by qspinlock now. and I did not see any performance regression. So just wrote in C, for simple. :)

Quoted 8 lines
Quoted 4 lines

Of course I have done xchg tests.
we run code just like xchg((u8*)&v, j++); in several threads. and the result is,

[  768.374264] use time[1550072]ns in xchg_u8_asm

How was xchg_u8_asm() implemented, using lbarx or using a 32bit ll/sc loop with shifting and masking in it?

yes, using 32bit ll/sc loops.

looks like:

Quoted 11 lines
        __asm__ __volatile__(
"1:     lwarx   %0,0,%3\n"
"       and %1,%0,%5\n"
"       or %1,%1,%4\n"
       PPC405_ERR77(0,%2)
"       stwcx.  %1,0,%3\n"
"       bne-    1b"
        : "=&r" (_oldv), "=&r" (tmp), "+m" (*(volatile unsigned int *)_p)
        : "r" (_p), "r" (_newv), "r" (_oldv_mask)
        : "cc", "memory");

Good, so this works for all ppc ISAs too.

Given the performance benefit(maybe caused by the reason Peter mentioned), I think we should use this as the implementation of u8/u16 {cmp}xchg for now. For Power7 and later, we can always switch to the lbarx/lharx version if observable performance benefit can be achieved.

But the choice is left to you. After all, as you said, qspinlock is the only user ;-)

Regards,
Boqun

Quoted 13 lines
Quoted 12 lines

Regards,
Boqun

Quoted 8 lines
[  768.377102] use time[2826802]ns in xchg_u8_c

I think this is because there is one more load in C. If possible, we can move such code in asm-generic/.

thanks
xinhui

#9 lore raw
In reply to Pan Xinhui
Quoted 3 lines · Pan Xinhui

yes, you are right. more load/store will be done in C code.
However such xchg_u8/u16 is just used by qspinlock now. and I did not see any performance regression. So just wrote in C, for simple. :)

Which is fine; but worthy of a note in your Changelog.

Quoted 8 lines

Of course I have done xchg tests.
we run code just like xchg((u8*)&v, j++); in several threads. and the result is,

[  768.374264] use time[1550072]ns in xchg_u8_asm
[  768.377102] use time[2826802]ns in xchg_u8_c

I think this is because there is one more load in C. If possible, we can move such code in asm-generic/.

So I'm not actually _that_ familiar with the PPC LL/SC implementation; but there are things a CPU can do to optimize these loops.

For example, a CPU might choose to not release the exclusive hold of the line for a number of cycles, except when it passes SC or an interrupt happens. This way there's a smaller chance the SC fails and inhibits forward progress.

By doing the modification outside of the LL/SC you loose such advantages.

And yes, doing a !exclusive load prior to the exclusive load leads to an even bigger window where the data can get changed out from under you.

author
#10 lore raw
Quoted 7 lines · Peter Zijlstra
Quoted 3 lines · Pan Xinhui

yes, you are right. more load/store will be done in C code.
However such xchg_u8/u16 is just used by qspinlock now. and I did not see any performance regression. So just wrote in C, for simple. :)

Which is fine; but worthy of a note in your Changelog.

will do that.

Quoted 9 lines
Quoted 8 lines

Of course I have done xchg tests.
we run code just like xchg((u8*)&v, j++); in several threads. and the result is,

[  768.374264] use time[1550072]ns in xchg_u8_asm
[  768.377102] use time[2826802]ns in xchg_u8_c

I think this is because there is one more load in C. If possible, we can move such code in asm-generic/.

Quoted 7 lines

So I'm not actually _that_ familiar with the PPC LL/SC implementation; but there are things a CPU can do to optimize these loops.

For example, a CPU might choose to not release the exclusive hold of the line for a number of cycles, except when it passes SC or an interrupt happens. This way there's a smaller chance the SC fails and inhibits forward progress.

I am not sure if there is such hardware optimization.

Quoted 7 lines

By doing the modification outside of the LL/SC you loose such advantages.

And yes, doing a !exclusive load prior to the exclusive load leads to an even bigger window where the data can get changed out from under you.

you are right.
We have observed such data change during the two different loads.

#11 lore raw
Quoted 9 lines · Pan Xinhui
Quoted 7 lines

So I'm not actually _that_ familiar with the PPC LL/SC implementation; but there are things a CPU can do to optimize these loops.

For example, a CPU might choose to not release the exclusive hold of the line for a number of cycles, except when it passes SC or an interrupt happens. This way there's a smaller chance the SC fails and inhibits forward progress.

I am not sure if there is such hardware optimization.

So I think the hardware must do _something_, otherwise competing cores doing load-exlusive could life-lock a system, each one endlessly breaking the exclusive ownership of the other and the store-conditional always failing.

Of course, there are such implementations, and they tend to have to put in explicit backoff loops; however, IIRC, PPC doesn't need that. (See ARC for an example that needs to do this.)

author
#12 lore raw
Quoted 9 lines · Peter Zijlstra
Quoted 7 lines · Pan Xinhui
Quoted 7 lines

So I'm not actually _that_ familiar with the PPC LL/SC implementation; but there are things a CPU can do to optimize these loops.

For example, a CPU might choose to not release the exclusive hold of the line for a number of cycles, except when it passes SC or an interrupt happens. This way there's a smaller chance the SC fails and inhibits forward progress.

Quoted 7 lines
Quoted 1 line

I am not sure if there is such hardware optimization.

So I think the hardware must do _something_, otherwise competing cores doing load-exlusive could life-lock a system, each one endlessly breaking the exclusive ownership of the other and the store-conditional always failing.

Seems there is no such optimization.

We haver observed SC fails almost all the time in a contention tests, then got stuck in the loop. :( one thread modify val with LL/SC, and other threads just modify val without any respect to LL/SC.

So in the end, I choose to rewrite this patch in asm. :)

Quoted 4 lines

Of course, there are such implementations, and they tend to have to put in explicit backoff loops; however, IIRC, PPC doesn't need that. (See ARC for an example that needs to do this.)

author
#13 lore raw

PATCH v4 1/1 powerpc: Implement {cmp}xchg for u8 and u16

In reply to Pan Xinhui

From: Pan Xinhui <[email protected]>

Implement xchg{u8,u16}{local,relaxed}, and
cmpxchg{u8,u16}{,local,acquire,relaxed}.

It works on all ppc.

remove volatile of first parameter in __cmpxchg_local and __cmpxchg

---
change from v3:
rewrite in asm for the LL/SC.
remove volatile in __cmpxchg_local and __cmpxchg.
change from v2:
in the do{}while(), we save one load and use corresponding cmpxchg suffix. Also add corresponding __cmpxchg_u32 function declaration in the __XCHG_GEN
change from V1:
rework totally.

---
 arch/powerpc/include/asm/cmpxchg.h | 109 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 106 insertions(+), 3 deletions(-)
arch/powerpc/include/asm/cmpxchg.h +106 -3
index 44efe73..8a3735f 100644
@@ -7,6 +7,71 @@
 #include <asm/asm-compat.h>
 #include <linux/bug.h>
 
+#ifdef __BIG_ENDIAN
+#define BITOFF_CAL(size, off)	((sizeof(u32) - size - off) * BITS_PER_BYTE)
+#else
+#define BITOFF_CAL(size, off)	(off * BITS_PER_BYTE)
+#endif
+
+#define XCHG_GEN(type, sfx, cl)				\
+static inline u32 __xchg_##type##sfx(void *p, u32 val)		\
+{								\
+	unsigned int prev, prev_mask, tmp, bitoff, off;		\
+								\
+	off = (unsigned long)p % sizeof(u32);			\
+	bitoff = BITOFF_CAL(sizeof(type), off);			\
+	p -= off;						\
+	val <<= bitoff;						\
+	prev_mask = (u32)(type)-1 << bitoff;			\
+								\
+	__asm__ __volatile__(					\
+"1:	lwarx   %0,0,%3\n"					\
+"	andc	%1,%0,%5\n"					\
+"	or	%1,%1,%4\n"					\
+	PPC405_ERR77(0,%3)					\
+"	stwcx.	%1,0,%3\n"					\
+"	bne-	1b\n"						\
+	: "=&r" (prev), "=&r" (tmp), "+m" (*(u32*)p)		\
+	: "r" (p), "r" (val), "r" (prev_mask)			\
+	: "cc", cl);						\
+								\
+	return prev >> bitoff;					\
+}
+
+#define CMPXCHG_GEN(type, sfx, br, br2, cl)			\
+static inline							\
+u32 __cmpxchg_##type##sfx(void *p, u32 old, u32 new)		\
+{								\
+	unsigned int prev, prev_mask, tmp, bitoff, off;		\
+								\
+	off = (unsigned long)p % sizeof(u32);			\
+	bitoff = BITOFF_CAL(sizeof(type), off);			\
+	p -= off;						\
+	old <<= bitoff;						\
+	new <<= bitoff;						\
+	prev_mask = (u32)(type)-1 << bitoff;			\
+								\
+	__asm__ __volatile__(					\
+	br							\
+"1:	lwarx   %0,0,%3\n"					\
+"	and	%1,%0,%6\n"					\
+"	cmpw	0,%1,%4\n"					\
+"	bne-	2f\n"						\
+"	andc	%1,%0,%6\n"					\
+"	or	%1,%1,%5\n"					\
+	PPC405_ERR77(0,%3)					\
+"	stwcx.  %1,0,%3\n"					\
+"	bne-    1b\n"						\
+	br2							\
+	"\n"							\
+"2:"								\
+	: "=&r" (prev), "=&r" (tmp), "+m" (*(u32*)p)		\
+	: "r" (p), "r" (old), "r" (new), "r" (prev_mask)	\
+	: "cc", cl);						\
+								\
+	return prev >> bitoff;					\
+}
+
 /*
  * Atomic exchange
  *
@@ -14,6 +79,11 @@
  * the previous value stored there.
  */
 
+XCHG_GEN(u8, _local, "memory");
+XCHG_GEN(u8, _relaxed, "cc");
+XCHG_GEN(u16, _local, "memory");
+XCHG_GEN(u16, _relaxed, "cc");
+
 static __always_inline unsigned long
 __xchg_u32_local(volatile void *p, unsigned long val)
 {
@@ -85,9 +155,13 @@ __xchg_u64_relaxed(u64 *p, unsigned long val)
 #endif
 
 static __always_inline unsigned long
-__xchg_local(volatile void *ptr, unsigned long x, unsigned int size)
+__xchg_local(void *ptr, unsigned long x, unsigned int size)
 {
 	switch (size) {
+	case 1:
+		return __xchg_u8_local(ptr, x);
+	case 2:
+		return __xchg_u16_local(ptr, x);
 	case 4:
 		return __xchg_u32_local(ptr, x);
 #ifdef CONFIG_PPC64
@@ -103,6 +177,10 @@ static __always_inline unsigned long
 __xchg_relaxed(void *ptr, unsigned long x, unsigned int size)
 {
 	switch (size) {
+	case 1:
+		return __xchg_u8_relaxed(ptr, x);
+	case 2:
+		return __xchg_u16_relaxed(ptr, x);
 	case 4:
 		return __xchg_u32_relaxed(ptr, x);
 #ifdef CONFIG_PPC64
@@ -131,6 +209,15 @@ __xchg_relaxed(void *ptr, unsigned long x, unsigned int size)
  * and return the old value of *p.
  */
 
+CMPXCHG_GEN(u8, , PPC_ATOMIC_ENTRY_BARRIER, PPC_ATOMIC_EXIT_BARRIER, "memory");
+CMPXCHG_GEN(u8, _local, , , "memory");
+CMPXCHG_GEN(u8, _acquire, , PPC_ACQUIRE_BARRIER, "memory");
+CMPXCHG_GEN(u8, _relaxed, , , "cc");
+CMPXCHG_GEN(u16, , PPC_ATOMIC_ENTRY_BARRIER, PPC_ATOMIC_EXIT_BARRIER, "memory");
+CMPXCHG_GEN(u16, _local, , , "memory");
+CMPXCHG_GEN(u16, _acquire, , PPC_ACQUIRE_BARRIER, "memory");
+CMPXCHG_GEN(u16, _relaxed, , , "cc");
+
 static __always_inline unsigned long
 __cmpxchg_u32(volatile unsigned int *p, unsigned long old, unsigned long new)
 {
@@ -312,10 +399,14 @@ __cmpxchg_u64_acquire(u64 *p, unsigned long old, unsigned long new)
 #endif
 
 static __always_inline unsigned long
-__cmpxchg(volatile void *ptr, unsigned long old, unsigned long new,
+__cmpxchg(void *ptr, unsigned long old, unsigned long new,
 	  unsigned int size)
 {
 	switch (size) {
+	case 1:
+		return __cmpxchg_u8(ptr, old, new);
+	case 2:
+		return __cmpxchg_u16(ptr, old, new);
 	case 4:
 		return __cmpxchg_u32(ptr, old, new);
 #ifdef CONFIG_PPC64
@@ -328,10 +419,14 @@ __cmpxchg(volatile void *ptr, unsigned long old, unsigned long new,
 }
 
 static __always_inline unsigned long
-__cmpxchg_local(volatile void *ptr, unsigned long old, unsigned long new,
+__cmpxchg_local(void *ptr, unsigned long old, unsigned long new,
 	  unsigned int size)
 {
 	switch (size) {
+	case 1:
+		return __cmpxchg_u8_local(ptr, old, new);
+	case 2:
+		return __cmpxchg_u16_local(ptr, old, new);
 	case 4:
 		return __cmpxchg_u32_local(ptr, old, new);
 #ifdef CONFIG_PPC64
@@ -348,6 +443,10 @@ __cmpxchg_relaxed(void *ptr, unsigned long old, unsigned long new,
 		  unsigned int size)
 {
 	switch (size) {
+	case 1:
+		return __cmpxchg_u8_relaxed(ptr, old, new);
+	case 2:
+		return __cmpxchg_u16_relaxed(ptr, old, new);
 	case 4:
 		return __cmpxchg_u32_relaxed(ptr, old, new);
 #ifdef CONFIG_PPC64
@@ -364,6 +463,10 @@ __cmpxchg_acquire(void *ptr, unsigned long old, unsigned long new,
 		  unsigned int size)
 {
 	switch (size) {
+	case 1:
+		return __cmpxchg_u8_acquire(ptr, old, new);
+	case 2:
+		return __cmpxchg_u16_acquire(ptr, old, new);
 	case 4:
 		return __cmpxchg_u32_acquire(ptr, old, new);
 #ifdef CONFIG_PPC64
Signature · 1 line
2.4.3
#14 lore raw
Quoted 47 lines · Pan Xinhui

From: Pan Xinhui <[email protected]>

Implement xchg{u8,u16}{local,relaxed}, and
cmpxchg{u8,u16}{,local,acquire,relaxed}.

It works on all ppc.

remove volatile of first parameter in __cmpxchg_local and __cmpxchg

---
change from v3:
rewrite in asm for the LL/SC.
remove volatile in __cmpxchg_local and __cmpxchg.
change from v2:
in the do{}while(), we save one load and use corresponding cmpxchg suffix. Also add corresponding __cmpxchg_u32 function declaration in the __XCHG_GEN
change from V1:
rework totally.

---
 arch/powerpc/include/asm/cmpxchg.h | 109 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 106 insertions(+), 3 deletions(-)
arch/powerpc/include/asm/cmpxchg.h +15 -0
index 44efe73..8a3735f 100644
@@ -7,6 +7,71 @@
 #include <asm/asm-compat.h>
 #include <linux/bug.h>
 
+#ifdef __BIG_ENDIAN
+#define BITOFF_CAL(size, off)	((sizeof(u32) - size - off) * BITS_PER_BYTE)
+#else
+#define BITOFF_CAL(size, off)	(off * BITS_PER_BYTE)
+#endif
+
+#define XCHG_GEN(type, sfx, cl)				\
+static inline u32 __xchg_##type##sfx(void *p, u32 val)		\
+{								\
+	unsigned int prev, prev_mask, tmp, bitoff, off;		\
+								\
+	off = (unsigned long)p % sizeof(u32);			\
+	bitoff = BITOFF_CAL(sizeof(type), off);			\
+	p -= off;						\
+	val <<= bitoff;						\

arch/powerpc/include/asm/cmpxchg.h line 34

Quoted 10 lines
arch/powerpc/include/asm/cmpxchg.h +10 -0
+	prev_mask = (u32)(type)-1 << bitoff;			\
+								\
+	__asm__ __volatile__(					\
+"1:	lwarx   %0,0,%3\n"					\
+"	andc	%1,%0,%5\n"					\
+"	or	%1,%1,%4\n"					\
+	PPC405_ERR77(0,%3)					\
+"	stwcx.	%1,0,%3\n"					\
+"	bne-	1b\n"						\
+	: "=&r" (prev), "=&r" (tmp), "+m" (*(u32*)p)		\

I think we can save the "tmp" here by:

	__asm__ volatile__(
"1:	lwarx	%0,0,%2\n"
"	andc	%0,%0,%4\n"
"	or	%0,%0,%3\n"
	PPC405_ERR77(0,%2)
"	stwcx.	%0,0,%2\n"
"	bne-	1b\n"
	: "=&r" (prev), "+m" (*(u32*)p)
	: "r" (p), "r" (val), "r" (prev_mask)
	: "cc", cl);

right?

Quoted 24 lines
arch/powerpc/include/asm/cmpxchg.h +24 -0
+	: "r" (p), "r" (val), "r" (prev_mask)			\
+	: "cc", cl);						\
+								\
+	return prev >> bitoff;					\
+}
+
+#define CMPXCHG_GEN(type, sfx, br, br2, cl)			\
+static inline							\
+u32 __cmpxchg_##type##sfx(void *p, u32 old, u32 new)		\
+{								\
+	unsigned int prev, prev_mask, tmp, bitoff, off;		\
+								\
+	off = (unsigned long)p % sizeof(u32);			\
+	bitoff = BITOFF_CAL(sizeof(type), off);			\
+	p -= off;						\
+	old <<= bitoff;						\
+	new <<= bitoff;						\
+	prev_mask = (u32)(type)-1 << bitoff;			\
+								\
+	__asm__ __volatile__(					\
+	br							\
+"1:	lwarx   %0,0,%3\n"					\
+"	and	%1,%0,%6\n"					\
+"	cmpw	0,%1,%4\n"					\

arch/powerpc/include/asm/cmpxchg.h line 68

Quoted 10 lines
arch/powerpc/include/asm/cmpxchg.h +10 -0
+"	bne-	2f\n"						\
+"	andc	%1,%0,%6\n"					\
+"	or	%1,%1,%5\n"					\
+	PPC405_ERR77(0,%3)					\
+"	stwcx.  %1,0,%3\n"					\
+"	bne-    1b\n"						\
+	br2							\
+	"\n"							\
+"2:"								\
+	: "=&r" (prev), "=&r" (tmp), "+m" (*(u32*)p)		\

And "tmp" here could also be saved by:

"1: lwarx %0,0,%2\n" \
" xor %3,%0,%3\n" \
" and. %3,%3,%5\n" \
" bne- 2f\n" \
" andc %0,%0,%5\n" \

"	or	%0,%0,%4\n"					\
	PPC405_ERR77(0,%2)					\
"	stwcx.  %0,0,%2\n"					\
"	bne-    1b\n"						\
	br2							\
	"\n"							\
"2:"								\
	: "=&r" (prev), "+m" (*(u32*)p)		\
	: "r" (p), "r" (old), "r" (new), "r" (prev_mask)	\
	: "cc", cl);						\

right?

IIUC, saving the local variable "tmp" will result in saving a general register for the compilers to use for other variables.

So thoughts?

Regards,
Boqun

Quoted 120 lines
arch/powerpc/include/asm/cmpxchg.h +47 -3
+	: "r" (p), "r" (old), "r" (new), "r" (prev_mask)	\
+	: "cc", cl);						\
+								\
+	return prev >> bitoff;					\
+}
+
 /*
  * Atomic exchange
  *
@@ -14,6 +79,11 @@
  * the previous value stored there.
  */
 
+XCHG_GEN(u8, _local, "memory");
+XCHG_GEN(u8, _relaxed, "cc");
+XCHG_GEN(u16, _local, "memory");
+XCHG_GEN(u16, _relaxed, "cc");
+
 static __always_inline unsigned long
 __xchg_u32_local(volatile void *p, unsigned long val)
 {
@@ -85,9 +155,13 @@ __xchg_u64_relaxed(u64 *p, unsigned long val)
 #endif
 
 static __always_inline unsigned long
-__xchg_local(volatile void *ptr, unsigned long x, unsigned int size)
+__xchg_local(void *ptr, unsigned long x, unsigned int size)
 {
 	switch (size) {
+	case 1:
+		return __xchg_u8_local(ptr, x);
+	case 2:
+		return __xchg_u16_local(ptr, x);
 	case 4:
 		return __xchg_u32_local(ptr, x);
 #ifdef CONFIG_PPC64
@@ -103,6 +177,10 @@ static __always_inline unsigned long
 __xchg_relaxed(void *ptr, unsigned long x, unsigned int size)
 {
 	switch (size) {
+	case 1:
+		return __xchg_u8_relaxed(ptr, x);
+	case 2:
+		return __xchg_u16_relaxed(ptr, x);
 	case 4:
 		return __xchg_u32_relaxed(ptr, x);
 #ifdef CONFIG_PPC64
@@ -131,6 +209,15 @@ __xchg_relaxed(void *ptr, unsigned long x, unsigned int size)
  * and return the old value of *p.
  */
 
+CMPXCHG_GEN(u8, , PPC_ATOMIC_ENTRY_BARRIER, PPC_ATOMIC_EXIT_BARRIER, "memory");
+CMPXCHG_GEN(u8, _local, , , "memory");
+CMPXCHG_GEN(u8, _acquire, , PPC_ACQUIRE_BARRIER, "memory");
+CMPXCHG_GEN(u8, _relaxed, , , "cc");
+CMPXCHG_GEN(u16, , PPC_ATOMIC_ENTRY_BARRIER, PPC_ATOMIC_EXIT_BARRIER, "memory");
+CMPXCHG_GEN(u16, _local, , , "memory");
+CMPXCHG_GEN(u16, _acquire, , PPC_ACQUIRE_BARRIER, "memory");
+CMPXCHG_GEN(u16, _relaxed, , , "cc");
+
 static __always_inline unsigned long
 __cmpxchg_u32(volatile unsigned int *p, unsigned long old, unsigned long new)
 {
@@ -312,10 +399,14 @@ __cmpxchg_u64_acquire(u64 *p, unsigned long old, unsigned long new)
 #endif
 
 static __always_inline unsigned long
-__cmpxchg(volatile void *ptr, unsigned long old, unsigned long new,
+__cmpxchg(void *ptr, unsigned long old, unsigned long new,
 	  unsigned int size)
 {
 	switch (size) {
+	case 1:
+		return __cmpxchg_u8(ptr, old, new);
+	case 2:
+		return __cmpxchg_u16(ptr, old, new);
 	case 4:
 		return __cmpxchg_u32(ptr, old, new);
 #ifdef CONFIG_PPC64
@@ -328,10 +419,14 @@ __cmpxchg(volatile void *ptr, unsigned long old, unsigned long new,
 }
 
 static __always_inline unsigned long
-__cmpxchg_local(volatile void *ptr, unsigned long old, unsigned long new,
+__cmpxchg_local(void *ptr, unsigned long old, unsigned long new,
 	  unsigned int size)
 {
 	switch (size) {
+	case 1:
+		return __cmpxchg_u8_local(ptr, old, new);
+	case 2:
+		return __cmpxchg_u16_local(ptr, old, new);
 	case 4:
 		return __cmpxchg_u32_local(ptr, old, new);
 #ifdef CONFIG_PPC64
@@ -348,6 +443,10 @@ __cmpxchg_relaxed(void *ptr, unsigned long old, unsigned long new,
 		  unsigned int size)
 {
 	switch (size) {
+	case 1:
+		return __cmpxchg_u8_relaxed(ptr, old, new);
+	case 2:
+		return __cmpxchg_u16_relaxed(ptr, old, new);
 	case 4:
 		return __cmpxchg_u32_relaxed(ptr, old, new);
 #ifdef CONFIG_PPC64
@@ -364,6 +463,10 @@ __cmpxchg_acquire(void *ptr, unsigned long old, unsigned long new,
 		  unsigned int size)
 {
 	switch (size) {
+	case 1:
+		return __cmpxchg_u8_acquire(ptr, old, new);
+	case 2:
+		return __cmpxchg_u16_acquire(ptr, old, new);
 	case 4:
 		return __cmpxchg_u32_acquire(ptr, old, new);
 #ifdef CONFIG_PPC64
Signature · 1 line
2.4.3
#15 lore raw
Quoted 116 lines · Boqun Feng
Quoted 47 lines · Pan Xinhui

From: Pan Xinhui <[email protected]>

Implement xchg{u8,u16}{local,relaxed}, and
cmpxchg{u8,u16}{,local,acquire,relaxed}.

It works on all ppc.

remove volatile of first parameter in __cmpxchg_local and __cmpxchg

---
change from v3:
rewrite in asm for the LL/SC.
remove volatile in __cmpxchg_local and __cmpxchg.
change from v2:
in the do{}while(), we save one load and use corresponding cmpxchg suffix. Also add corresponding __cmpxchg_u32 function declaration in the __XCHG_GEN
change from V1:
rework totally.

---
 arch/powerpc/include/asm/cmpxchg.h | 109 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 106 insertions(+), 3 deletions(-)
arch/powerpc/include/asm/cmpxchg.h +15 -0
index 44efe73..8a3735f 100644
@@ -7,6 +7,71 @@
 #include <asm/asm-compat.h>
 #include <linux/bug.h>
 
+#ifdef __BIG_ENDIAN
+#define BITOFF_CAL(size, off)	((sizeof(u32) - size - off) * BITS_PER_BYTE)
+#else
+#define BITOFF_CAL(size, off)	(off * BITS_PER_BYTE)
+#endif
+
+#define XCHG_GEN(type, sfx, cl)				\
+static inline u32 __xchg_##type##sfx(void *p, u32 val)		\
+{								\
+	unsigned int prev, prev_mask, tmp, bitoff, off;		\
+								\
+	off = (unsigned long)p % sizeof(u32);			\
+	bitoff = BITOFF_CAL(sizeof(type), off);			\
+	p -= off;						\
+	val <<= bitoff;						\

arch/powerpc/include/asm/cmpxchg.h line 34

Quoted 10 lines
arch/powerpc/include/asm/cmpxchg.h +10 -0
+	prev_mask = (u32)(type)-1 << bitoff;			\
+								\
+	__asm__ __volatile__(					\
+"1:	lwarx   %0,0,%3\n"					\
+"	andc	%1,%0,%5\n"					\
+"	or	%1,%1,%4\n"					\
+	PPC405_ERR77(0,%3)					\
+"	stwcx.	%1,0,%3\n"					\
+"	bne-	1b\n"						\
+	: "=&r" (prev), "=&r" (tmp), "+m" (*(u32*)p)		\

I think we can save the "tmp" here by:

	__asm__ volatile__(
"1:	lwarx	%0,0,%2\n"
"	andc	%0,%0,%4\n"
"	or	%0,%0,%3\n"
	PPC405_ERR77(0,%2)
"	stwcx.	%0,0,%2\n"
"	bne-	1b\n"
	: "=&r" (prev), "+m" (*(u32*)p)
	: "r" (p), "r" (val), "r" (prev_mask)
	: "cc", cl);

right?

Quoted 24 lines
arch/powerpc/include/asm/cmpxchg.h +24 -0
+	: "r" (p), "r" (val), "r" (prev_mask)			\
+	: "cc", cl);						\
+								\
+	return prev >> bitoff;					\
+}
+
+#define CMPXCHG_GEN(type, sfx, br, br2, cl)			\
+static inline							\
+u32 __cmpxchg_##type##sfx(void *p, u32 old, u32 new)		\
+{								\
+	unsigned int prev, prev_mask, tmp, bitoff, off;		\
+								\
+	off = (unsigned long)p % sizeof(u32);			\
+	bitoff = BITOFF_CAL(sizeof(type), off);			\
+	p -= off;						\
+	old <<= bitoff;						\
+	new <<= bitoff;						\
+	prev_mask = (u32)(type)-1 << bitoff;			\
+								\
+	__asm__ __volatile__(					\
+	br							\
+"1:	lwarx   %0,0,%3\n"					\
+"	and	%1,%0,%6\n"					\
+"	cmpw	0,%1,%4\n"					\

arch/powerpc/include/asm/cmpxchg.h line 68

Quoted 10 lines
arch/powerpc/include/asm/cmpxchg.h +10 -0
+"	bne-	2f\n"						\
+"	andc	%1,%0,%6\n"					\
+"	or	%1,%1,%5\n"					\
+	PPC405_ERR77(0,%3)					\
+"	stwcx.  %1,0,%3\n"					\
+"	bne-    1b\n"						\
+	br2							\
+	"\n"							\
+"2:"								\
+	: "=&r" (prev), "=&r" (tmp), "+m" (*(u32*)p)		\

And "tmp" here could also be saved by:

"1: lwarx %0,0,%2\n" \
" xor %3,%0,%3\n" \
" and. %3,%3,%5\n" \
" bne- 2f\n" \
" andc %0,%0,%5\n" \

Quoted 11 lines
"	or	%0,%0,%4\n"					\
	PPC405_ERR77(0,%2)					\
"	stwcx.  %0,0,%2\n"					\
"	bne-    1b\n"						\
	br2							\
	"\n"							\
"2:"								\
	: "=&r" (prev), "+m" (*(u32*)p)		\
	: "r" (p), "r" (old), "r" (new), "r" (prev_mask)	\
	: "cc", cl);						\

Oops, this should be:

"1: lwarx %0,0,%3\n" \
" xor %2,%0,%2\n" \
" and. %2,%2,%5\n" \
" bne- 2f\n" \
" andc %0,%0,%5\n" \

"	or	%0,%0,%4\n"					\
	PPC405_ERR77(0,%3)					\
"	stwcx.  %0,0,%3\n"					\
"	bne-    1b\n"						\
	br2							\
	"\n"							\
"2:"								\
	: "=&r" (prev), "+m" (*(u32*)p), "+&r" (old)		\
	: "r" (p), "r" (new), "r" (prev_mask)			\
	: "cc", cl);						\

Regards,
Boqun

#16 lore raw
In reply to Boqun Feng
Quoted 127 lines · Boqun Feng
Quoted 47 lines · Pan Xinhui

From: Pan Xinhui <[email protected]>

Implement xchg{u8,u16}{local,relaxed}, and
cmpxchg{u8,u16}{,local,acquire,relaxed}.

It works on all ppc.

remove volatile of first parameter in __cmpxchg_local and __cmpxchg

---
change from v3:
rewrite in asm for the LL/SC.
remove volatile in __cmpxchg_local and __cmpxchg.
change from v2:
in the do{}while(), we save one load and use corresponding cmpxchg suffix. Also add corresponding __cmpxchg_u32 function declaration in the __XCHG_GEN
change from V1:
rework totally.

---
 arch/powerpc/include/asm/cmpxchg.h | 109 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 106 insertions(+), 3 deletions(-)
arch/powerpc/include/asm/cmpxchg.h +15 -0
index 44efe73..8a3735f 100644
@@ -7,6 +7,71 @@
 #include <asm/asm-compat.h>
 #include <linux/bug.h>
 
+#ifdef __BIG_ENDIAN
+#define BITOFF_CAL(size, off)	((sizeof(u32) - size - off) * BITS_PER_BYTE)
+#else
+#define BITOFF_CAL(size, off)	(off * BITS_PER_BYTE)
+#endif
+
+#define XCHG_GEN(type, sfx, cl)				\
+static inline u32 __xchg_##type##sfx(void *p, u32 val)		\
+{								\
+	unsigned int prev, prev_mask, tmp, bitoff, off;		\
+								\
+	off = (unsigned long)p % sizeof(u32);			\
+	bitoff = BITOFF_CAL(sizeof(type), off);			\
+	p -= off;						\
+	val <<= bitoff;						\

arch/powerpc/include/asm/cmpxchg.h line 34

Quoted 10 lines
arch/powerpc/include/asm/cmpxchg.h +10 -0
+	prev_mask = (u32)(type)-1 << bitoff;			\
+								\
+	__asm__ __volatile__(					\
+"1:	lwarx   %0,0,%3\n"					\
+"	andc	%1,%0,%5\n"					\
+"	or	%1,%1,%4\n"					\
+	PPC405_ERR77(0,%3)					\
+"	stwcx.	%1,0,%3\n"					\
+"	bne-	1b\n"						\
+	: "=&r" (prev), "=&r" (tmp), "+m" (*(u32*)p)		\

I think we can save the "tmp" here by:

	__asm__ volatile__(
"1:	lwarx	%0,0,%2\n"
"	andc	%0,%0,%4\n"
"	or	%0,%0,%3\n"
	PPC405_ERR77(0,%2)
"	stwcx.	%0,0,%2\n"
"	bne-	1b\n"
	: "=&r" (prev), "+m" (*(u32*)p)
	: "r" (p), "r" (val), "r" (prev_mask)
	: "cc", cl);

right?

Quoted 24 lines
arch/powerpc/include/asm/cmpxchg.h +24 -0
+	: "r" (p), "r" (val), "r" (prev_mask)			\
+	: "cc", cl);						\
+								\
+	return prev >> bitoff;					\
+}
+
+#define CMPXCHG_GEN(type, sfx, br, br2, cl)			\
+static inline							\
+u32 __cmpxchg_##type##sfx(void *p, u32 old, u32 new)		\
+{								\
+	unsigned int prev, prev_mask, tmp, bitoff, off;		\
+								\
+	off = (unsigned long)p % sizeof(u32);			\
+	bitoff = BITOFF_CAL(sizeof(type), off);			\
+	p -= off;						\
+	old <<= bitoff;						\
+	new <<= bitoff;						\
+	prev_mask = (u32)(type)-1 << bitoff;			\
+								\
+	__asm__ __volatile__(					\
+	br							\
+"1:	lwarx   %0,0,%3\n"					\
+"	and	%1,%0,%6\n"					\
+"	cmpw	0,%1,%4\n"					\

arch/powerpc/include/asm/cmpxchg.h line 68

Quoted 10 lines
arch/powerpc/include/asm/cmpxchg.h +10 -0
+"	bne-	2f\n"						\
+"	andc	%1,%0,%6\n"					\
+"	or	%1,%1,%5\n"					\
+	PPC405_ERR77(0,%3)					\
+"	stwcx.  %1,0,%3\n"					\
+"	bne-    1b\n"						\
+	br2							\
+	"\n"							\
+"2:"								\
+	: "=&r" (prev), "=&r" (tmp), "+m" (*(u32*)p)		\

And "tmp" here could also be saved by:

"1: lwarx %0,0,%2\n" \
" xor %3,%0,%3\n" \
" and. %3,%3,%5\n" \
" bne- 2f\n" \
" andc %0,%0,%5\n" \

"	or	%0,%0,%4\n"					\
	PPC405_ERR77(0,%2)					\
"	stwcx.  %0,0,%2\n"					\
"	bne-    1b\n"						\
	br2							\
	"\n"							\
"2:"								\
	: "=&r" (prev), "+m" (*(u32*)p)		\
	: "r" (p), "r" (old), "r" (new), "r" (prev_mask)	\
	: "cc", cl);						\
Quoted 2 lines

right?

Sorry, my bad, we can't implement cmpxchg like this.. please ignore this, I should really go to bed soon...

But still, we can save the "tmp" for xchg() I think.

Regards,
Boqun

Quoted 8 lines

IIUC, saving the local variable "tmp" will result in saving a general register for the compilers to use for other variables.

So thoughts?

Regards,
Boqun

Acked-by
#17 lore raw
Quoted 6 lines · Boqun Feng

Sorry, my bad, we can't implement cmpxchg like this.. please ignore this, I should really go to bed soon...

But still, we can save the "tmp" for xchg() I think.

No.. we can't. Sorry for all the noise.

This patch looks good to me.

FWIW, you can add

Regards,
Boqun

author
#18 lore raw
Quoted 8 lines · Boqun Feng
Quoted 6 lines · Boqun Feng

Sorry, my bad, we can't implement cmpxchg like this.. please ignore this, I should really go to bed soon...

But still, we can save the "tmp" for xchg() I think.

Quoted 8 lines

No.. we can't. Sorry for all the noise.

This patch looks good to me.

FWIW, you can add

thanks!

Quoted 3 lines

Regards,
Boqun

Acked-by on v4 1/1
#19 lore raw
In reply to Pan Xinhui
Quoted 11 lines · Pan Xinhui

From: Pan Xinhui <[email protected]>

Implement xchg{u8,u16}{local,relaxed}, and
cmpxchg{u8,u16}{,local,acquire,relaxed}.

It works on all ppc.

remove volatile of first parameter in __cmpxchg_local and __cmpxchg

Generally has the right shape; and I trust others to double check the ppc-asm minutia.

author
#20 lore raw
Quoted 10 lines · Peter Zijlstra
Quoted 9 lines · Pan Xinhui

From: Pan Xinhui <[email protected]>

Implement xchg{u8,u16}{local,relaxed}, and
cmpxchg{u8,u16}{,local,acquire,relaxed}.

It works on all ppc.

remove volatile of first parameter in __cmpxchg_local and __cmpxchg

Quoted 9 lines
Quoted 2 lines

Generally has the right shape; and I trust others to double check the ppc-asm minutia.

thanks!

#21 lore raw
In reply to Pan Xinhui
Quoted 5 lines · xinhui

From: Pan Xinhui <[email protected]>

Implement xchg{u8,u16}{local,relaxed}, and
cmpxchg{u8,u16}{,local,acquire,relaxed}.

Quoted 8 lines

It works on all ppc.

remove volatile of first parameter in __cmpxchg_local and __cmpxchg

Applied to powerpc next, thanks.

https://git.kernel.org/powerpc/c/d0563a1297e234ed37f6b51c2e9321

cheers