rewrite kernel spinlock code to work better with gcc

RFC In review Randolph Chung 4 participants 15 messages Nov 26, 2003 – Dec 2, 2003 Atomic infrastructure

lore

Share & tools

lore

Randolph Chung

author

2003-11-26 07:03 UTC #0 lore raw

RFC 1/1 rewrite kernel spinlock code to work better with gcc

This has been discussed in bits and pieces several times on the list, let me summarize:

- On at least some PA processors, addresses passed to ldcw() must be 16-byte aligned
- gcc doesn't guarantee alignment of automatic variables even if the structure is marked with aligned(16). In gcc-3.0.x, this worked most of the time because stack alignment was set to 128 bits, but this caused various problems so the change was reverted in later revisions of gcc.

In glibc, Carlos and Dave implemented "auto-aligning" locks by using an array of 4 ints and doing ldcw on the 16-byte aligned word inside that array. This makes the code work all the time irregardless of how it is placed in memory. Here's a patch that implements similar locking mechanisms for the kernel. It compiles, but as SMP still doesn't boot on 2.6, i haven't really tried to run it.

there is some concern this will make structures bigger, but at least in some situations this actually makes them smaller. e.g. if you have:

struct { int x; spinlock_t lock; };

with the current scheme (using the aligned(16) attribute) the structure is 32 bytes. with the new scheme it is only 20 bytes. actually i don't think there are any cases where it will make any structures bigger than they are now.....

Any comments?

Index: include/asm-parisc/spinlock.h
===================================================================
RCS file: /var/cvs/linux-2.6/include/asm-parisc/spinlock.h,v retrieving revision 1.1

asm-parisc/spinlock.h +31 ~~-24~~

diff -u -p -r1.1 spinlock.h
@@ -4,35 +4,42 @@
 #include <asm/system.h>
 
 /* Note that PA-RISC has to use `1' to mean unlocked and `0' to mean locked
- * since it only has load-and-zero.
+ * since it only has load-and-zero. Moreover, at least on some PA processors,
+ * the semaphore address has to be 16-byte aligned.
  */
 
 #undef SPIN_LOCK_UNLOCKED
-#define SPIN_LOCK_UNLOCKED (spinlock_t) { 1 }
+#define SPIN_LOCK_UNLOCKED (spinlock_t) { { 1, 1, 1, 1 } }
 
-#define spin_lock_init(x)	do { (x)->lock = 1; } while(0)
+#define spin_lock_init(x)	do { *(x) = SPIN_LOCK_UNLOCKED; } while(0)
 
-#define spin_is_locked(x) ((x)->lock == 0)
-
-#define spin_unlock_wait(x)	do { barrier(); } while(((volatile spinlock_t *)(x))->lock == 0)
-
-#if 1
-#define _raw_spin_lock(x) do { \
-	while (__ldcw (&(x)->lock) == 0) \
-		while (((x)->lock) == 0) ; } while (0)
-
-#else
-#define _raw_spin_lock(x) \
-	do { while(__ldcw(&(x)->lock) == 0); } while(0)
-#endif
+static inline int spin_is_locked(spinlock_t *x)
+{
+	volatile unsigned int *a = __ldcw_align(x);
+	return *a == 0;
+}
+
+#define spin_unlock_wait(x)	do { barrier(); } while(spin_is_locked(x))
+
+static inline void _raw_spin_lock(spinlock_t *x)
+{
+	volatile unsigned int *a = __ldcw_align(x);
+	while (__ldcw(a) == 0)
+		while (*a == 0);
+}
+
+static inline void _raw_spin_unlock(spinlock_t *x)
+{
+	volatile unsigned int *a = __ldcw_align(x);
+	*a = 1;
+}
+
+static inline int _raw_spin_trylock(spinlock_t *x)
+{
+	volatile unsigned int *a = __ldcw_align(x);
+	return __ldcw(a) != 0;
+}
 	
-#define _raw_spin_unlock(x) \
-	do { (x)->lock = 1; } while(0)
-
-#define _raw_spin_trylock(x) (__ldcw(&(x)->lock) != 0)
-
-
-
 /*
  * Read-write spinlocks, allowing multiple readers
  * but only one writer.

@@ -42,7 +49,7 @@ typedef struct {
 	volatile int counter;
 } rwlock_t;
 
-#define RW_LOCK_UNLOCKED (rwlock_t) { {1}, 0 }
+#define RW_LOCK_UNLOCKED (rwlock_t) { { { 1, 1, 1, 1 } }, 0 }
 
 #define rwlock_init(lp)	do { *(lp) = RW_LOCK_UNLOCKED; } while (0)

Index: include/asm-parisc/system.h
===================================================================
RCS file: /var/cvs/linux-2.6/include/asm-parisc/system.h,v retrieving revision 1.1

asm-parisc/system.h +14 -1

diff -u -p -r1.1 system.h
@@ -145,6 +145,19 @@ static inline void set_eiem(unsigned lon
 	__ret; \
 })
 
+/* Because kmalloc only guarantees 8-byte alignment for kmalloc'd data,
+   and GCC only guarantees 8-byte alignment for stack locals, we can't
+   be assured of 16-byte alignment for atomic lock data even if we
+   specify "__attribute ((aligned(16)))" in the type declaration.  So,
+   we use a struct containing an array of four ints for the atomic lock
+   type and dynamically select the 16-byte aligned int from the array
+   for the semaphore.  */
+#define __PA_LDCW_ALIGNMENT 16
+#define __ldcw_align(a) ({ \
+  unsigned long __ret = (unsigned long) a;                     		\
+  __ret = (__ret + __PA_LDCW_ALIGNMENT - 1) & ~(__PA_LDCW_ALIGNMENT - 1); \
+  (unsigned int *) __ret;                                               \
+})
 
 #ifdef CONFIG_SMP
 /*

@@ -152,7 +165,7 @@ static inline void set_eiem(unsigned lon
  */
 
 typedef struct {
-	volatile unsigned int __attribute__((aligned(16))) lock;
+	volatile unsigned int lock[4];
 } spinlock_t;
 #endif

Index: include/asm-parisc/atomic.h
===================================================================
RCS file: /var/cvs/linux-2.6/include/asm-parisc/atomic.h,v retrieving revision 1.5

asm-parisc/atomic.h +12 -5

diff -u -p -r1.5 atomic.h
@@ -24,11 +24,18 @@
 
 extern spinlock_t __atomic_hash[ATOMIC_HASH_SIZE];
 /* copied from <asm/spinlock.h> and modified */
-#  define SPIN_LOCK(x) \
-	do { while(__ldcw(&(x)->lock) == 0); } while(0)
-	
-#  define SPIN_UNLOCK(x) \
-	do { (x)->lock = 1; } while(0)
+static inline void SPIN_LOCK(spinlock_t *x)
+{
+	volatile unsigned int *a = __ldcw_align(x);
+	while (__ldcw(a) == 0)
+		while (*a == 0);
+}
+
+static inline void SPIN_UNLOCK(spinlock_t *x)
+{
+	volatile unsigned int *a = __ldcw_align(x);
+	*a = 1;
+}
 #else
 #  define ATOMIC_HASH_SIZE 1
 #  define ATOMIC_HASH(a)	(0)

randolph

Signature · 3 lines

Randolph Chung
Debian GNU/Linux Developer, hppa/ia64 ports
http://www.tausq.org/

John David Anglin

2003-11-26 16:54 UTC #1 lore raw

asm-parisc/system.h static inline void set_eiem(unsigned lon · line 160

Quoted 13 lines

asm-parisc/system.h +13 -0

+/* Because kmalloc only guarantees 8-byte alignment for kmalloc'd data,
+   and GCC only guarantees 8-byte alignment for stack locals, we can't
+   be assured of 16-byte alignment for atomic lock data even if we
+   specify "__attribute ((aligned(16)))" in the type declaration.  So,
+   we use a struct containing an array of four ints for the atomic lock
+   type and dynamically select the 16-byte aligned int from the array
+   for the semaphore.  */
+#define __PA_LDCW_ALIGNMENT 16
+#define __ldcw_align(a) ({ \
+  unsigned long __ret = (unsigned long) a;                     		\
+  __ret = (__ret + __PA_LDCW_ALIGNMENT - 1) & ~(__PA_LDCW_ALIGNMENT - 1); \
+  (unsigned int *) __ret;                                               \
+})

Change cast to "volatile unsigned int *".

asm-parisc/system.h static inline void set_eiem(unsigned lon · line 170

Quoted 5 lines

typedef struct {

asm-parisc/system.h +1 -1

-	volatile unsigned int __attribute__((aligned(16))) lock;
+	volatile unsigned int lock[4];
 } spinlock_t;
 #endif

Is the struct necessary? For example,

  typedef volatile unsigned int spinlock_t[4];

I believe that there are situations where the rest of the cache line holding the active lock word should not be used. The PA 1.x document says

When using semaphores to synchonize with I/O, care must be taken in placing other information in the same cache line as the semaphore. Data which is writeable can only be placed in the same cache line as a semaphore if access to write the data is controlled by the semaphore.

I think this restriction only applies to semaphores used both by I/O and CPU processors, and it's not a concern for semaphores used solely by CPU processors. If we have locks used by I/O processors and they are stack allocated, then some extra padding would appear to be needed for these locks.

Dave

Signature · 2 lines

J. David Anglin                                  [email protected]
National Research Council of Canada              (613) 990-0752 (FAX: 952-6602)

Joel Soete

2003-11-29 23:50 UTC #2 lore raw

Quoted 18 lines · John David Anglin

asm-parisc/system.h static inline void set_eiem(unsigned lon · line 160

Quoted 13 lines

asm-parisc/system.h +13 -0

+/* Because kmalloc only guarantees 8-byte alignment for kmalloc'd data,
+   and GCC only guarantees 8-byte alignment for stack locals, we can't
+   be assured of 16-byte alignment for atomic lock data even if we
+   specify "__attribute ((aligned(16)))" in the type declaration.  So,
+   we use a struct containing an array of four ints for the atomic lock
+   type and dynamically select the 16-byte aligned int from the array
+   for the semaphore.  */
+#define __PA_LDCW_ALIGNMENT 16
+#define __ldcw_align(a) ({ \
+  unsigned long __ret = (unsigned long) a;                     		\
+  __ret = (__ret + __PA_LDCW_ALIGNMENT - 1) & ~(__PA_LDCW_ALIGNMENT - 1); \
+  (unsigned int *) __ret;                                               \
+})

Change cast to "volatile unsigned int *".

Quoted 9 lines

Quoted 5 lines
typedef struct {
asm-parisc/system.h +1 -1
-	volatile unsigned int __attribute__((aligned(16))) lock;
+	volatile unsigned int lock[4];
} spinlock_t;
#endif
Is the struct necessary? For example,

hmm i supposed that came from 2.4 struct:
typedef struct {

#ifdef CONFIG_PA20
	volatile unsigned int lock;
#else
	volatile unsigned int __attribute__((aligned(16))) lock;
#endif
#ifdef CONFIG_DEBUG_SPINLOCK
	volatile unsigned long owner_pc;
	volatile unsigned long owner_cpu;

#endif
} spinlock_t;

But I don't know yet if CONFIG_DEBUG_SPINLOCK is still foreseen for 2.6

Thanks,
Joel

Joel Soete

2003-11-29 23:35 UTC #3 lore raw

In reply to Randolph Chung

Hi Randolph,

may be this alignement with 2.4 is still relevant:
---------><---------

system.h 2003-11-30 00:26:17.000000000 +0100 renamed from system.h-rc 2003-11-30 00:21:55.000000000 +0100 +0 -0

@@ -138,12 +138,36 @@
 #define set_wmb(var, value)            do { var = value; wmb(); }

while (0)

-/* LDCW, the only atomic read-write operation PA-RISC has. *sigh*. */
+/* LDCW, the only atomic read-write operation PA-RISC has. *sigh*.
+ *
+ * Note that PA-RISC has to use 1' to mean unlocked and 0' to mean locked
+ * since it only has load-and-zero. Moreover, at least on some PA processors,+ * the semaphore address has to be 16-byte aligned.
+ */
+#ifdef CONFIG_PA20
+/*
+> From: "Jim Hull" <jim.hull of hp.com>
+> Delivery-date: Wed, 29 Jan 2003 13:57:05 -0500
+> I've attached a summary of the change, but basically, for PA 2.0, as
+> long as the ",CO" (coherent operation) completer is specified, then the
+> 16-byte alignment requirement for ldcw and ldcd is relaxed, and instead
+> they only require "natural" alignment (4-byte for ldcw, 8-byte for
+> ldcd).
+*/
+
+#define __ldcw(a) ({ \

+       unsigned __ret; \
+       __asm__ __volatile__("ldcw,co 0(%1),%0" : "=r" (__ret) : "r" (a)); \
+       __ret; \
+})
+#else
 #define __ldcw(a) ({ \
        unsigned __ret; \
        __asm__ __volatile__("ldcw 0(%1),%0" : "=r" (__ret) : "r" (a)); \
        __ret; \

})
+#endif
+

/* Because kmalloc only guarantees 8-byte alignment for kmalloc'd data,
and GCC only guarantees 8-byte alignment for stack locals, we can't
---------><---------

hth,
Joel

ps: also join as attachment (in case of bad wraping)

Quoted 178 lines · Randolph Chung

This has been discussed in bits and pieces several times on the list, let me summarize:
- On at least some PA processors, addresses passed to ldcw() must be 16-byte aligned
- gcc doesn't guarantee alignment of automatic variables even if the structure is marked with aligned(16). In gcc-3.0.x, this worked most of the time because stack alignment was set to 128 bits, but this caused various problems so the change was reverted in later revisions of gcc.
In glibc, Carlos and Dave implemented "auto-aligning" locks by using an array of 4 ints and doing ldcw on the 16-byte aligned word inside that array. This makes the code work all the time irregardless of how it is placed in memory. Here's a patch that implements similar locking mechanisms for the kernel. It compiles, but as SMP still doesn't boot on 2.6, i haven't really tried to run it.
there is some concern this will make structures bigger, but at least in some situations this actually makes them smaller. e.g. if you have:
struct { int x; spinlock_t lock; };
with the current scheme (using the aligned(16) attribute) the structure
is 32 bytes. with the new scheme it is only 20 bytes. actually i don't think there are any cases where it will make any structures bigger than they are now.....
Any comments?
Index: include/asm-parisc/spinlock.h
===================================================================
RCS file: /var/cvs/linux-2.6/include/asm-parisc/spinlock.h,v retrieving revision 1.1
diff -u -p -r1.1 spinlock.h
asm-parisc/spinlock.h +31 ~~-24~~
@@ -4,35 +4,42 @@
 #include <asm/system.h>
 
 /* Note that PA-RISC has to use `1' to mean unlocked and `0' to mean locked
- * since it only has load-and-zero.
+ * since it only has load-and-zero. Moreover, at least on some PA processors,
+ * the semaphore address has to be 16-byte aligned.
  */
 
 #undef SPIN_LOCK_UNLOCKED
-#define SPIN_LOCK_UNLOCKED (spinlock_t) { 1 }
+#define SPIN_LOCK_UNLOCKED (spinlock_t) { { 1, 1, 1, 1 } }
 
-#define spin_lock_init(x)	do { (x)->lock = 1; } while(0)
+#define spin_lock_init(x)	do { *(x) = SPIN_LOCK_UNLOCKED; } while(0)
 
-#define spin_is_locked(x) ((x)->lock == 0)
-
-#define spin_unlock_wait(x)	do { barrier(); } while(((volatile spinlock_t *)(x))->lock == 0)
-
-#if 1
-#define _raw_spin_lock(x) do { \
-	while (__ldcw (&(x)->lock) == 0) \
-		while (((x)->lock) == 0) ; } while (0)
-
-#else
-#define _raw_spin_lock(x) \
-	do { while(__ldcw(&(x)->lock) == 0); } while(0)
-#endif
+static inline int spin_is_locked(spinlock_t *x)
+{
+	volatile unsigned int *a = __ldcw_align(x);
+	return *a == 0;
+}
+
+#define spin_unlock_wait(x)	do { barrier(); } while(spin_is_locked(x))
+
+static inline void _raw_spin_lock(spinlock_t *x)
+{
+	volatile unsigned int *a = __ldcw_align(x);
+	while (__ldcw(a) == 0)
+		while (*a == 0);
+}
+
+static inline void _raw_spin_unlock(spinlock_t *x)
+{
+	volatile unsigned int *a = __ldcw_align(x);
+	*a = 1;
+}
+
+static inline int _raw_spin_trylock(spinlock_t *x)
+{
+	volatile unsigned int *a = __ldcw_align(x);
+	return __ldcw(a) != 0;
+}
 
-#define _raw_spin_unlock(x) \
-	do { (x)->lock = 1; } while(0)
-
-#define _raw_spin_trylock(x) (__ldcw(&(x)->lock) != 0)
-
-
-
 /*
  * Read-write spinlocks, allowing multiple readers
  * but only one writer.
@@ -42,7 +49,7 @@ typedef struct {
 	volatile int counter;
 } rwlock_t;
 
-#define RW_LOCK_UNLOCKED (rwlock_t) { {1}, 0 }
+#define RW_LOCK_UNLOCKED (rwlock_t) { { { 1, 1, 1, 1 } }, 0 }
 
 #define rwlock_init(lp)	do { *(lp) = RW_LOCK_UNLOCKED; } while (0)
 
Index: include/asm-parisc/system.h
===================================================================
RCS file: /var/cvs/linux-2.6/include/asm-parisc/system.h,v retrieving revision 1.1
diff -u -p -r1.1 system.h
asm-parisc/system.h +14 -1
@@ -145,6 +145,19 @@ static inline void set_eiem(unsigned lon
 	__ret; \
 })
 
+/* Because kmalloc only guarantees 8-byte alignment for kmalloc'd data,
+   and GCC only guarantees 8-byte alignment for stack locals, we can't
+   be assured of 16-byte alignment for atomic lock data even if we
+   specify "__attribute ((aligned(16)))" in the type declaration.  So,
+   we use a struct containing an array of four ints for the atomic lock
+   type and dynamically select the 16-byte aligned int from the array
+   for the semaphore.  */
+#define __PA_LDCW_ALIGNMENT 16
+#define __ldcw_align(a) ({ \
+  unsigned long __ret = (unsigned long) a;                     		\
+  __ret = (__ret + __PA_LDCW_ALIGNMENT - 1) & ~(__PA_LDCW_ALIGNMENT - 1); \
+  (unsigned int *) __ret;                                               \
+})
 
 #ifdef CONFIG_SMP
 /*
@@ -152,7 +165,7 @@ static inline void set_eiem(unsigned lon
  */
 
 typedef struct {
-	volatile unsigned int __attribute__((aligned(16))) lock;
+	volatile unsigned int lock[4];
 } spinlock_t;
 #endif
Index: include/asm-parisc/atomic.h
===================================================================
RCS file: /var/cvs/linux-2.6/include/asm-parisc/atomic.h,v retrieving revision 1.5
diff -u -p -r1.5 atomic.h
asm-parisc/atomic.h +12 -5
@@ -24,11 +24,18 @@
 
 extern spinlock_t __atomic_hash[ATOMIC_HASH_SIZE];
 /* copied from <asm/spinlock.h> and modified */
-#  define SPIN_LOCK(x) \
-	do { while(__ldcw(&(x)->lock) == 0); } while(0)
-	
-#  define SPIN_UNLOCK(x) \
-	do { (x)->lock = 1; } while(0)
+static inline void SPIN_LOCK(spinlock_t *x)
+{
+	volatile unsigned int *a = __ldcw_align(x);
+	while (__ldcw(a) == 0)
+		while (*a == 0);
+}
+
+static inline void SPIN_UNLOCK(spinlock_t *x)
+{
+	volatile unsigned int *a = __ldcw_align(x);
+	*a = 1;
+}
 #else
 #  define ATOMIC_HASH_SIZE 1
 #  define ATOMIC_HASH(a)	(0)

Quoted 1 line

randolph

system.h renamed from system.h-rc +25 -1

@@ -138,12 +138,36 @@
 #define set_wmb(var, value)		do { var = value; wmb(); } while (0)
 
 
-/* LDCW, the only atomic read-write operation PA-RISC has. *sigh*.  */
+/* LDCW, the only atomic read-write operation PA-RISC has. *sigh*.
+ *
+ * Note that PA-RISC has to use `1' to mean unlocked and `0' to mean locked
+ * since it only has load-and-zero. Moreover, at least on some PA processors,
+ * the semaphore address has to be 16-byte aligned.
+ */
+#ifdef CONFIG_PA20
+/* 
+> From: "Jim Hull" <jim.hull of hp.com>
+> Delivery-date: Wed, 29 Jan 2003 13:57:05 -0500
+> I've attached a summary of the change, but basically, for PA 2.0, as
+> long as the ",CO" (coherent operation) completer is specified, then the
+> 16-byte alignment requirement for ldcw and ldcd is relaxed, and instead
+> they only require "natural" alignment (4-byte for ldcw, 8-byte for
+> ldcd).
+*/
+
+#define __ldcw(a) ({ \
+	unsigned __ret; \
+	__asm__ __volatile__("ldcw,co 0(%1),%0" : "=r" (__ret) : "r" (a)); \
+	__ret; \
+})
+#else
 #define __ldcw(a) ({ \
 	unsigned __ret; \
 	__asm__ __volatile__("ldcw 0(%1),%0" : "=r" (__ret) : "r" (a)); \
 	__ret; \
 })
+#endif
+
 
 /* Because kmalloc only guarantees 8-byte alignment for kmalloc'd data,
    and GCC only guarantees 8-byte alignment for stack locals, we can't

Joel Soete

2003-11-30 00:43 UTC #4 lore raw

[parisc-linux] spinlock 2.4 re-organise a la 2.6 [was: [RFC] rewrite kernel spinlock code to work better with gcc]

In reply to Randolph Chung

Hello,

btw here is attached a patch to re-organize spinlock stuff in 2.4 as in 2.6.

Can somebody help me to apply and test it before ci (not yet time to test and no access to cvs ci)

thanks in advance,
Joel

include/asm-parisc/atomic.h +1 -2

diff -Naur linux-2.4.23-rc5-pa17/include/asm-parisc/atomic.h linux-2.4.23-rc5-pa17-bp/include/asm-parisc/atomic.h
@@ -1,7 +1,6 @@
 #ifndef _ASM_PARISC_ATOMIC_H_
 #define _ASM_PARISC_ATOMIC_H_
 
-#include <linux/config.h>
 #include <asm/system.h>
 
 /* Copyright (C) 2000 Philipp Rumpf <[email protected]>.  */

@@ -193,4 +192,4 @@
 #define smp_mb__before_atomic_inc()	smp_mb()
 #define smp_mb__after_atomic_inc()	smp_mb()
 
-#endif
+#endif /* _ASM_PARISC_ATOMIC_H_ */

include/asm-parisc/spinlock.h +49 -3

58 lines not on this page · raw message

include/asm-parisc/spinlock_t.h +0 ~~-97~~

97 lines not on this page · raw message

include/asm-parisc/system.h +47 -6

75 lines not on this page · raw message

Grant Grundler

2003-11-30 03:37 UTC #5 lore raw

Re: [parisc-linux] spinlock 2.4 re-organise a la 2.6 [was: [RFC] rewrite kernel spinlock code to work better with gcc]

On Sun, Nov 30, 2003 at 12:43:51AM +0000, Joel Soete wrote:
...

include/asm-parisc/atomic.h line 4

Quoted 7 lines

+++ linux-2.4.23-rc5-pa17-bp/include/asm-parisc/atomic.h 2003-11-30 01:23:57.000000000 +0100
include/asm-parisc/atomic.h +0 -1
@@ -1,7 +1,6 @@
 #ifndef _ASM_PARISC_ATOMIC_H_
 #define _ASM_PARISC_ATOMIC_H_
 
-#include <linux/config.h>
 #include <asm/system.h>

Joel,
This is wrong - atomic.h uses CONFIG_SMP and thus is required to include config.h.

grant

Joel Soete

2003-11-30 10:39 UTC #6 lore raw

Re: [parisc-linux] spinlock 2.4 re-organise a la 2.6 [was: [RFC] rewrite kernel spinlock code to work better with gcc]

Quoted 8 lines · Grant Grundler

On Sun, Nov 30, 2003 at 12:43:51AM +0000, Joel Soete wrote:
...
Quoted 5 lines
+++ linux-2.4.23-rc5-pa17-bp/include/asm-parisc/atomic.h 2003-11-30 01:23:57.000000000 +0100
include/asm-parisc/atomic.h +0 -0
@@ -1,7 +1,6 @@
 #ifndef _ASM_PARISC_ATOMIC_H_
 #define _ASM_PARISC_ATOMIC_H_
 

Quoted 8 lines

include/asm-parisc/atomic.h line 4
Quoted 2 lines
include/asm-parisc/atomic.h +0 -1
-#include <linux/config.h>
 #include <asm/system.h>
Joel,
This is wrong - atomic.h uses CONFIG_SMP and thus is required to include config.h.

I trust you.
Just a small explanation: i was confused by the begining of asm/system.h which is:
---------><---------
#ifndef __PARISC_SYSTEM_H
#define __PARISC_SYSTEM_H

#include <linux/config.h>
#include <asm/psw.h>

[...]
---------><---------

so I trusted that asm/system.h firstly include <linux/config.h>. So there should be something wrong elsewhere but don't yet find.

Sorry for my bad understand,

Joel

Joel Soete

2003-11-30 10:57 UTC #7 lore raw

Re: [parisc-linux] spinlock 2.4 re-organise a la 2.6 [was: [RFC] rewrite kernel spinlock code to work better with gcc]

In reply to Grant Grundler

Yes forgive that try, it breaks something in 'make dep' I presume because make vmlinux failled oribly:

gcc -D__KERNEL__ -I/Debian-apt/SRC/Test/linux-2.4.23-rc5-pa17/include -Wall -Wstrict-prototypes -Wno-trigraphs -O2 -fno-strict-aliasing -fno-common -D__linux__ -pipe -fno-strength-reduce -mno-space-regs -mfast-indirect-calls -mdisable-fpregs -ffunction-sections -march=1.1 -mschedule=7100 -DKBUILD_BASENAME=main -c -o init/main.o init/main.c
In file included from /Debian-apt/SRC/Test/linux-2.4.23-rc5-pa17/include/linux/wait.h:16,
from
/Debian-apt/SRC/Test/linux-2.4.23-rc5-pa17/include/linux/fs.h:12,
from
/Debian-apt/SRC/Test/linux-2.4.23-rc5-pa17/include/linux/capability.h:17,
from
/Debian-apt/SRC/Test/linux-2.4.23-rc5-pa17/include/linux/binfmts.h:5,
from
/Debian-apt/SRC/Test/linux-2.4.23-rc5-pa17/include/linux/sched.h:9,
from
/Debian-apt/SRC/Test/linux-2.4.23-rc5-pa17/include/linux/mm.h:4,
from
/Debian-apt/SRC/Test/linux-2.4.23-rc5-pa17/include/linux/slab.h:14,
from
/Debian-apt/SRC/Test/linux-2.4.23-rc5-pa17/include/linux/proc_fs.h:5,

                 from init/main.c:15:

/Debian-apt/SRC/Test/linux-2.4.23-rc5-pa17/include/linux/spinlock.h:75: error: conflicting types for spinlock_t' /Debian-apt/SRC/Test/linux-2.4.23-rc5-pa17/include/asm/system.h:189: error: previous declaration of spinlock_t'
make: *** [init/main.o] Error 1

My bad, sorry,
Joel

Quoted 22 lines · Grant Grundler

On Sun, Nov 30, 2003 at 12:43:51AM +0000, Joel Soete wrote:
...
include/asm-parisc/atomic.h line 4
Quoted 7 lines
+++ linux-2.4.23-rc5-pa17-bp/include/asm-parisc/atomic.h 2003-11-30 01:23:57.000000000 +0100
include/asm-parisc/atomic.h +0 -1
@@ -1,7 +1,6 @@
 #ifndef _ASM_PARISC_ATOMIC_H_
 #define _ASM_PARISC_ATOMIC_H_
 
-#include <linux/config.h>
 #include <asm/system.h>
Joel,
This is wrong - atomic.h uses CONFIG_SMP and thus is required to include config.h.
grant
_______________________________________________
parisc-linux mailing list
[email protected]
http://lists.parisc-linux.org/mailman/listinfo/parisc-linux

Joel Soete

2003-11-30 16:31 UTC #8 lore raw

Re: [parisc-linux] spinlock 2.4 re-organise a la 2.6 [was: [RFC] rewrite kernel spinlock code to work better with gcc]

In reply to Grant Grundler

Quoted 8 lines · Grant Grundler

On Sun, Nov 30, 2003 at 12:43:51AM +0000, Joel Soete wrote:
...
Quoted 5 lines
+++ linux-2.4.23-rc5-pa17-bp/include/asm-parisc/atomic.h 2003-11-30 01:23:57.000000000 +0100
(quoted diff) +0 -0
@@ -1,7 +1,6 @@
 #ifndef _ASM_PARISC_ATOMIC_H_
 #define _ASM_PARISC_ATOMIC_H_
 

Quoted 8 lines

Review line 4
Quoted 2 lines
(quoted diff) +0 -1
-#include <linux/config.h>
 #include <asm/system.h>
Joel,
This is wrong - atomic.h uses CONFIG_SMP and thus is required to include config.h.

No, i think it is right but I forgot to embrace spinlock_t declaration with "#ifdef CONFIG_SMP ...#endif" in system.h. Done in the new attched patch (tested and run fine on c100 (32bit up) with kernel up and smp) My bad in previous test: forgot make distclean ; make mrproper (to be sure).

Still have to test in 64bit up (no means to test in smp neither 32 or 64 bit :( ).

If you find some interest can you ci (I would like to test Randolph patch on n4k just to be sure).

Thanks for help,
Joel

include/asm-parisc/atomic.h +1 -3

diff -Naur linux-2.4.23-rc5-pa17/include/asm-parisc/atomic.h linux-2.4.23-rc5-pa17-bp/include/asm-parisc/atomic.h
@@ -1,7 +1,6 @@
 #ifndef _ASM_PARISC_ATOMIC_H_
 #define _ASM_PARISC_ATOMIC_H_
 
-#include <linux/config.h>
 #include <asm/system.h>
 
 /* Copyright (C) 2000 Philipp Rumpf <[email protected]>.  */

@@ -14,7 +13,6 @@
  * have to write any serious assembly.   prumpf
  */
 #ifdef CONFIG_SMP
-#include <asm/spinlock_t.h>
 
 /* Use an array of spinlocks for our atomic_ts.
 ** Hash function to index into a different SPINLOCK.

@@ -193,4 +191,4 @@
 #define smp_mb__before_atomic_inc()	smp_mb()
 #define smp_mb__after_atomic_inc()	smp_mb()
 
-#endif
+#endif /* _ASM_PARISC_ATOMIC_H_ */

include/asm-parisc/processor.h +0 -3

diff -Naur linux-2.4.23-rc5-pa17/include/asm-parisc/processor.h linux-2.4.23-rc5-pa17-bp/include/asm-parisc/processor.h
@@ -18,9 +18,6 @@
 #include <asm/ptrace.h>
 #include <asm/types.h>
 #include <asm/system.h>
-#ifdef CONFIG_SMP
-#include <asm/spinlock_t.h>
-#endif
 #endif /* __ASSEMBLY__ */
 
 /*

include/asm-parisc/spinlock.h +49 -3

58 lines not on this page · raw message

include/asm-parisc/spinlock_t.h +0 ~~-97~~

97 lines not on this page · raw message

include/asm-parisc/system.h +49 -6

77 lines not on this page · raw message

Grant Grundler

2003-11-30 21:10 UTC #9 lore raw

Re: [parisc-linux] spinlock 2.4 re-organise a la 2.6 [was: [RFC] rewrite kernel spinlock code to work better with gcc]

Quoted 4 lines · Joel Soete

No, i think it is right but I forgot to embrace spinlock_t declaration with "#ifdef CONFIG_SMP ...#endif" in system.h. Done in the new attched patch (tested and run fine on c100 (32bit up) with kernel up and smp) My bad in previous test: forgot make distclean ; make mrproper (to be sure).

Use of "make distclean" means you don't have the dependencies correct. ie whereever "ifdef CONFIG_*" is used, linux/config.h needs to be included.

Quoted 2 lines

If you find some interest can you ci (I would like to test Randolph patch on n4k just to be sure).

I don't. I have a basic problem with this patch.

...

Quoted 1 line

-#include <asm/spinlock_t.h>

...

include/asm-parisc/spinlock_t.h line 2

Quoted 6 lines

diff -Naur linux-2.4.23-rc5-pa17/include/asm-parisc/spinlock_t.h linux-2.4.23-rc5-pa17-bp/include/asm-parisc/spinlock_t.h
include/asm-parisc/spinlock_t.h +0 -2
@@ -1,97 +0,0 @@
-#ifndef __PARISC_SPINLOCK_T_H
-#define __PARISC_SPINLOCK_T_H

...

Why delete spinlock_t.h?

I added spinlock_t.h to resolve the circular inter-dependency between asm/system.h, asm/bitops.h, and asm/spinlock.h. spinlock_t.h depends on nothing and defines spinlock primitives used by the others.

grant

Joel Soete

2003-12-01 07:00 UTC #10 lore raw

Re: [parisc-linux] spinlock 2.4 re-organise a la 2.6 [was: [RFC] rewrite kernel spinlock code to work better with gcc]

Quoted 3 lines

I added spinlock_t.h to resolve the circular inter-dependency between asm/system.h, asm/bitops.h, and asm/spinlock.h. spinlock_t.h depends on nothing and defines spinlock primitives used by the others.

I see (forgive)

Joel

-------------------------------------------------------------------------
Tiscali ADSL: 12 mois à 29,50 €/mois! L'Internet rapide, c'est pour tout le monde.
http://reg.tiscali.be/default.asp?lg=fr

Joel Soete

2003-11-30 00:51 UTC #11 lore raw

In reply to Randolph Chung

a trivial atomic.h patch:
---------><---------

atomic.h 2003-11-30 01:48:01.000000000 +0100 renamed from atomic.h.orig 2003-11-30 01:47:49.000000000 +0100 +0 -1

@@ -1,7 +1,6 @@
 #ifndef _ASM_PARISC_ATOMIC_H_
 #define _ASM_PARISC_ATOMIC_H_
 
-#include <linux/config.h>
 #include <asm/system.h>
 
 /* Copyright (C) 2000 Philipp Rumpf <[email protected]>.  */

---------><---------

hth,

J.

Joel Soete

2003-12-01 15:14 UTC #12 lore raw

In reply to Randolph Chung

Hi Randolph,

(Just in case it would help) I backport your patch in 2.4 (just because the only system on which I can test it is a b2k on which 2.6 doesn't boot yet) and build a smp 64bit kernel which boot and run fine (rebuild another kernel) on this b2k:)

Joel

ps: I don't yet investigate Dave idea :(

Quoted 10 lines

-- Original Message --
From: Randolph Chung <[email protected]>
To: [email protected]
Reply-To: Randolph Chung <[email protected]>
Date: Tue, 25 Nov 2003 23:07:14 -0800
Subject: [parisc-linux] [RFC] rewrite kernel spinlock code to work better with gcc
This has been discussed in bits and pieces several times on the list,

let me summarize:

- On at least some PA processors, addresses passed to ldcw() must be 16-byte aligned
- gcc doesn't guarantee alignment of automatic variables even if the str

Quoted 1 line

cture is marked with aligned(16). In gcc-3.0.x, this worked most of

the time because stack alignment was set to 128 bits, but this caused various problems so the change was reverted in later revisions of gcc.

In glibc, Carlos and Dave implemented

Quoted 1 line

auto-aligning" locks by using an

array of 4 ints and doing ldcw on the 16-byte aligned word inside that array. This makes the code work all the time irregardless of how it is placed in memory. Here's a patch that implements similar locking mechanis

Quoted 1 line

s for the kernel. It compiles, but as SMP still doesn't boot

on 2.6, i haven't really tried to run it.

there is some concern this will make structures bigger, but at least in some situations this actually makes them smaller. e.g. if you have:

Quoted 1 line

struct { int x; spinlock_t lock; };

Quoted 1 line

than they are now.....

Any comments?

Index: include/asm-parisc/spinlock.h
===================================================================
RCS file: /var/cvs/linux-2.6/include/asm-parisc/spinlock.h,v retrieving revision 1.1
diff -u -p -r

Quoted 1 line

.1 spinlock.h

asm-parisc/spinlock.h +0 -0

@@ -4,35 +4,42 @@
 #include <asm/system.h>
 
 /* Note that PA-RISC has to use `1' to mean unlocked and `0'

Quoted 1 line

to mean locked

- * since it only has load-and-zero.
+ * since it only has load-and-zero. Moreover, at least on some PA processors,
+ * the semaphore address has to be 16-byte aligned.
*/

#undef SPIN_LOCK_UNLOCKED
-#define SPIN_LOCK_UNLOCKED

Quoted 1 line

(spinlock_t) { 1 }

+#define SPIN_LOCK_UNLOCKED (spinlock_t) { { 1, 1, 1, 1 } }

-#define spin_lock_init(x) do { (x)->lock = 1; } while(0)
+#define spin_lock_init(x) do { *(x) = SPIN_LOCK_UNLOCKED; } while(0)

-#define spin_is_locked(x) ((x)->lo

Quoted 1 line

k == 0)

-
-#define spin_unlock_wait(x) do { barrier(); } while(((volatile spinlock_t
*)(x))->lock == 0)
-
-#if 1
-#define _raw_spin_lock(x) do { \
- while (__ldcw (&(x)->lock) == 0) \
- while (((x)->lock) == 0) ; } while (0)
-
-#else
-#defin

Quoted 1 line

_raw_spin_lock(x) \

- do { while(__ldcw(&(x)->lock) == 0); } while(0)
-#endif
+static inline int spin_is_locked(spinlock_t *x)
+{
+ volatile unsigned int *a = __ldcw_align(x);
+ return *a == 0;
+}
+
+#define spin_unlock_wait(x) do { barrier()

Quoted 1 line

} while(spin_is_locked(x))

+
+static inline void _raw_spin_lock(spinlock_t *x) +{
+ volatile unsigned int *a = __ldcw_align(x);
+ while (__ldcw(a) == 0)
+ while (*a == 0);
+}
+
+static inline void _raw_spin_unlock(spinlock_t *x) +{
+ volat

Quoted 1 line

le unsigned int *a = __ldcw_align(x);

+ *a = 1;
+}
+
+static inline int _raw_spin_trylock(spinlock_t *x) +{
+ volatile unsigned int *a = __ldcw_align(x);
+ return __ldcw(a) != 0;
+}

-#define _raw_spin_unlock(x) \
- do { (x)->lock = 1; } w

Quoted 1 line

ile(0)

-
-#define _raw_spin_trylock(x) (__ldcw(&(x)->lock) != 0) -
-
-
/*
* Read-write spinlocks, allowing multiple readers
* but only one writer.
@@ -42,7 +49,7 @@ typedef struct {
volatile int counter;
} rwlock_t;

-#define RW_L

Quoted 1 line

CK_UNLOCKED (rwlock_t) { {1}, 0 }

+#define RW_LOCK_UNLOCKED (rwlock_t) { { { 1, 1, 1, 1 } }, 0 }

#define rwlock_init(lp) do { *(lp) = RW_LOCK_UNLOCKED; } while (0)

Index: include/asm-parisc/system.h
==========================================

Quoted 1 line

========================

RCS file: /var/cvs/linux-2.6/include/asm-parisc/system.h,v retrieving revision 1.1

asm-parisc/system.h +0 -0

diff -u -p -r1.1 system.h

Quoted 1 line

2 -0000

@@ -145,6 +145,19 @@ static inline void set_eiem(unsigned lon
__ret; \
})

+/* Because kmalloc only guarantees 8-byte alignment for kmalloc'd data,
+ and GCC only guarantees 8-byte alignment for stack locals, we can't
+ be assure

Quoted 1 line

of 16-byte alignment for atomic lock data even if we

+   specify "__attribute ((aligned(16)))" in the type declaration.  So,
+   we use a struct containing an array of four ints for the atomic lock
+   type and dynamically select the 16-byte aligne

Quoted 1 line

int from the array

+ for the semaphore. */
+#define __PA_LDCW_ALIGNMENT 16
+#define __ldcw_align(a) ({ \
+ unsigned long __ret = (unsigned long) a; \
+ __ret = (__ret + __PA_LDCW_ALIGNMENT - 1) & ~(__PA_LDCW_ALIGNMENT -

Quoted 1 line

1); \

+ (unsigned int *) __ret;
\
+})

#ifdef CONFIG_SMP
/*
@@ -152,7 +165,7 @@ static inline void set_eiem(unsigned lon
*/

typedef struct {
- volatile unsigned int __attribute__((aligned

Quoted 1 line

16))) lock;

+ volatile unsigned int lock[4];
} spinlock_t;
#endif
Index: include/asm-parisc/atomic.h
===================================================================
RCS file: /var/cvs/linux-2.6/include/asm-parisc/atomic.h,v retrieving revi

Quoted 1 line

ion 1.5

asm-parisc/atomic.h +0 -0

diff -u -p -r1.5 atomic.h
@@ -24,11 +24,18 @@
 
 extern spinlock_t __atomic_hash[ATOMIC_HASH_SIZE];
 /* copied from

Quoted 1 line

<asm/spinlock.h> and modified */

-# define SPIN_LOCK(x) \
- do { while(__ldcw(&(x)->lock) == 0); } while(0)
-
-# define SPIN_UNLOCK(x) \
- do { (x)->lock = 1; } while(0)
+static inline void SPIN_LOCK(spinlock_t *x)
+{
+ volatile unsigned int

Quoted 1 line

*a = __ldcw_align(x);

+ while (__ldcw(a) == 0)
+ while (*a == 0);
+}
+
+static inline void SPIN_UNLOCK(spinlock_t *x)
+{
+ volatile unsigned int *a = __ldcw_align(x);
+ *a = 1;
+}
#else
# define ATOMIC_HASH_SIZE 1
# define ATOMIC_HASH

Quoted 1 line

a) (0)

randolph

Signature · 7 lines

Randolph Chung
Debian GNU/Linux Developer, hppa/ia64 ports
http://www.tausq.org/
_______________________________________________
parisc-linux mailing list
[email protected]
http://lists.parisc-linux.or

Quoted 1 line

/mailman/listinfo/parisc-linux

-------------------------------------------------------------------------
Tiscali ADSL: 12 mois à 29,50 €/mois! L'Internet rapide, c'est pour tout le monde.
http://reg.tiscali.be/default.asp?lg=fr

John David Anglin

2003-12-01 18:30 UTC #13 lore raw

Quoted 4 lines

(Just in case it would help) I backport your patch in 2.4 (just because the only system on which I can test it is a b2k on which 2.6 doesn't boot yet) and build a smp 64bit kernel which boot and run fine (rebuild another kernel) on this b2k:)

I'd forgotten about the "ldcw,co" errata for PA 2.0. You only want/need self-aligning locks for PA 1.x. You should be able to test this on a PA 2.0 machine if you don't define CONFIG_PA20.

Dave

Signature · 2 lines

J. David Anglin                                  [email protected]
National Research Council of Canada              (613) 990-0752 (FAX: 952-6602)

Joel Soete

2003-12-02 16:42 UTC #14 lore raw

Hi Dave,

I trust that I have to restart my test (32b smp, 64b smp; all seems to works fine in 32b but some worries in 64b). With the new 2.4.23-pa1, I discover that I let badly commented a small chunk a code (just involved toc) and I wrongly merge Randolph's patch (just about a small #ifdef CONFIG_PA20 in 2.4: thanks for your comments :) ).

I will advise.

Thanks again,
Joel

Quoted 10 lines

-- Original Message --
To: [email protected] (Joel Soete)
Date: Mon, 1 Dec 2003 13:30:07 -0500 (EST)
From: "John David Anglin" <[email protected]>
Cc: [email protected], [email protected]
Subject: Re: [parisc-linux] [RFC] rewrite kernel spinlock code to work better with gcc
Quoted 1 line
(Just in case it would help) I backport your patch in 2.4 (just because

the

Quoted 4 lines

only system on which I can test it is a b2k on which 2.6 doesn't boot yet) and build a smp 64bit kernel which boot and run fine (rebuild another kernel) on this b2k:

I'd forgotten about the "ldcw,co" errata for PA 2.0. You only want/need self-aligning locks for PA 1.x. You should be able to test this on a PA 2.0 machine if you don't define CONFIG_PA20.

Dave

Signature · 1 line

J. David Anglin

Quoted 1 line

[email protected]

National Research Council of Canada (613) 990-0752 (FAX: 952-6602)

-------------------------------------------------------------------------
Tiscali ADSL: 12 mois à 29,50 €/mois! L'Internet rapide, c'est pour tout le monde.
http://reg.tiscali.be/default.asp?lg=fr