Skip to contentSkip to conversation

HPPA Linuxthreads.

PATCH In review Carlos O'Donell 4 participants 13 messages May 21, 2003 – May 22, 2003

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 HPPA Linuxthreads.

libc-alpha,

This is a cleaner HPPA linuxthreads implementation that stems from the work that John David Anglin <[email protected]> and myself did to devise a self-aligning lock system that doesn't impose the 16-byte lock alignment restriction. Many thanks go to John for all his hard work!

This will also allow hppa to relax malloc alignment back to 8 (future patches).

libc/linuxthreads/*

 sysdeps/hppa/pt-machine.h                    |   81 ++++++++++++++++++++++-----
 sysdeps/hppa/pspinlock.c                     |   29 ++++-----
 sysdeps/unix/sysv/linux/hppa/bits/initspin.h |   22 +++++--
 sysdeps/pthread/bits/libc-lock.h             |    6 +-
 sysdeps/pthread/bits/pthreadtypes.h          |    8 +-
 descr.h                                      |    2 
 pt-machine.c                                 |    4 +
 pthread.c                                    |   14 ++--
 spinlock.c                                   |   24 ++++----
 spinlock.h                                   |   26 ++++++--
 10 files changed, 148 insertions(+), 68 deletions(-)

Comments welcome. Patches tested on HPPA, i686.

c.
---

2003-05-19 Carlos O'Donell <[email protected]>

* linuxthreads/sysdeps/hppa/pt-machine.h (THREAD_SELF): Define.
(INIT_THREAD_SELF): Define.
(testandset): Use __ldcw_align.
(lock_held): Define.
(__ldcw): Define.
(__ldcw_align): Define.
(__load_and_clear): Define.
* linuxthreads/sysdeps/hppa/pspinlock.c (__pthread_spin_lock): Use __ldcw_align. (__pthread_spin_trylock): Likewise. (__pthread_spin_unlock): Likewise. (__pthread_spin_init): Likewise.
* linuxthreads/sysdeps/unix/sysv/linux/hppa/bits/initspin.h (__LT_SPINLOCK_INIT): Define. (__LT_SPINLOCK_ALT_INIT): Define. (__LOCK_INITIALIZER): Define. (__LOCK_ALT_INITIALIZER): Define. (__ATOMIC_INITIALIZER): Define. (__LT_INITIALIZER_NOT_ZERO): Define.

* linuxthreads/sysdeps/pthread/bits/libc-lock.h:
Use __LT_INITIALIZER_NOT_ZERO instead of __LT_SPINLOCK_INIT.
* linuxthreads/sysdeps/pthread/bits/pthreadtypes.h:
Add global default definition for __atomic_lock_t. (pthread_fastlock): Change __spinlock to type __atomic_lock_t.

* linuxthreads/descr.h (pthread_atomic): change p_spinlock to type __atomic_lock_t from int *.
* linuxthreads/pt-machine.c: include pthread.h, change extern testandset prototype to use __atomic_lock_t * spinlock.
* linuxthreads/pthread.c (__pthread_initialize_minimal):
Use __LT_INITIALIZER_NOT_ZERO.
* linuxthreads/spinlock.c: use __pthread_lock_define_initialized macro to initialize wait_node_free_list_spinlock. (__pthread_acquire): change prototype to use __atomic_lock_t * spinlock.
(__pthread_release): Likewise. (__pthread_compare_and_swap): Likewise. (__pthread_acquire): Likewise.
(__pthread_alt_lock): Use __LT_SPINLOCK_INIT to initialize locks. (__pthread_alt_timedlock): Likewise.
* linuxthreads/spinlock.h: define default lock_held, __pthread_lock_define_initialized, and modify prototypes for __pthread_compare_and_swap, compare_and_swap, compare_and_swap_with_release_semantics, compare_and_swap, __pthread_compare_and_swap to use __atomic_lock_t * spinlock.

linuxthreads/descr.h +1 -1
diff -urN glibc-2.3.1.orig/linuxthreads/descr.h glibc-2.3.1/linuxthreads/descr.h
@@ -70,7 +70,7 @@
 /* Atomic counter made possible by compare_and_swap */
 struct pthread_atomic {
   long p_count;
-  int p_spinlock;
+  __atomic_lock_t p_spinlock;
 };
 
 
linuxthreads/pt-machine.c +3 -1
diff -urN glibc-2.3.1.orig/linuxthreads/pt-machine.c glibc-2.3.1/linuxthreads/pt-machine.c
@@ -19,7 +19,9 @@
 
 #define PT_EI
 
-extern long int testandset (int *spinlock);
+#include <pthread.h>
+
+extern long int testandset (__atomic_lock_t *spinlock);
 extern int __compare_and_swap (long int *p, long int oldval, long int newval);
 
 #include <pt-machine.h>
linuxthreads/pthread.c +7 -7
diff -urN glibc-2.3.1.orig/linuxthreads/pthread.c glibc-2.3.1/linuxthreads/pthread.c
@@ -296,9 +296,9 @@
   pthread_descr self;
 
   /* First of all init __pthread_handles[0] and [1] if needed.  */
-# if __LT_SPINLOCK_INIT != 0
-  __pthread_handles[0].h_lock = __LOCK_INITIALIZER;
-  __pthread_handles[1].h_lock = __LOCK_INITIALIZER;
+# ifdef __LT_INITIALIZER_NOT_ZERO
+  __pthread_handles[0].h_lock = __LOCK_ALT_INITIALIZER;
+  __pthread_handles[1].h_lock = __LOCK_ALT_INITIALIZER;
 # endif
 # ifndef SHARED
   /* Unlike in the dynamically linked case the dynamic linker has not
@@ -366,7 +366,7 @@
 # endif
   /* self->p_start_args need not be initialized, it's all zero.  */
   self->p_userstack = 1;
-# if __LT_SPINLOCK_INIT != 0
+# ifdef __LT_INITIALIZER_NOT_ZERO 
   self->p_resume_count = (struct pthread_atomic) __ATOMIC_INITIALIZER;
 # endif
   self->p_alloca_cutoff = __MAX_ALLOCA_CUTOFF;
@@ -380,9 +380,9 @@
 #else  /* USE_TLS */
 
   /* First of all init __pthread_handles[0] and [1].  */
-# if __LT_SPINLOCK_INIT != 0
-  __pthread_handles[0].h_lock = __LOCK_INITIALIZER;
-  __pthread_handles[1].h_lock = __LOCK_INITIALIZER;
+# ifdef __LT_INITIALIZER_NOT_ZERO
+  __pthread_handles[0].h_lock = __LOCK_ALT_INITIALIZER;
+  __pthread_handles[1].h_lock = __LOCK_ALT_INITIALIZER;
 # endif
   __pthread_handles[0].h_descr = &__pthread_initial_thread;
   __pthread_handles[1].h_descr = &__pthread_manager_thread;
linuxthreads/spinlock.c +11 -11
diff -urN glibc-2.3.1.orig/linuxthreads/spinlock.c glibc-2.3.1/linuxthreads/spinlock.c
@@ -24,9 +24,9 @@
 #include "spinlock.h"
 #include "restart.h"
 
-static void __pthread_acquire(int * spinlock);
+static void __pthread_acquire(__atomic_lock_t * spinlock);
 
-static inline void __pthread_release(int * spinlock)
+static inline void __pthread_release(__atomic_lock_t * spinlock)
 {
   WRITE_MEMORY_BARRIER();
   *spinlock = __LT_SPINLOCK_INIT;
@@ -269,11 +269,11 @@
 struct wait_node {
   struct wait_node *next;	/* Next node in null terminated linked list */
   pthread_descr thr;		/* The thread waiting with this node */
-  int abandoned;		/* Atomic flag */
+  __atomic_lock_t abandoned;	/* Atomic flag */
 };
 
 static long wait_node_free_list;
-static int wait_node_free_list_spinlock;
+__pthread_lock_define_initialized(static, wait_node_free_list_spinlock);
 
 /* Allocate a new node from the head of the free list using an atomic
    operation, or else using malloc if that list is empty.  A fundamental
@@ -376,7 +376,7 @@
       if (self == NULL)
 	self = thread_self();
 
-      wait_node.abandoned = 0;
+      wait_node.abandoned = __LT_SPINLOCK_INIT;
       wait_node.next = (struct wait_node *) lock->__status;
       wait_node.thr = self;
       lock->__status = (long) &wait_node;
@@ -402,7 +402,7 @@
       wait_node.thr = self;
       newstatus = (long) &wait_node;
     }
-    wait_node.abandoned = 0;
+    wait_node.abandoned = __LT_SPINLOCK_INIT;
     wait_node.next = (struct wait_node *) oldstatus;
     /* Make sure the store in wait_node.next completes before performing
        the compare-and-swap */
@@ -451,7 +451,7 @@
       if (self == NULL)
 	self = thread_self();
 
-      p_wait_node->abandoned = 0;
+      p_wait_node->abandoned = __LT_SPINLOCK_INIT;
       p_wait_node->next = (struct wait_node *) lock->__status;
       p_wait_node->thr = self;
       lock->__status = (long) p_wait_node;
@@ -474,7 +474,7 @@
       p_wait_node->thr = self;
       newstatus = (long) p_wait_node;
     }
-    p_wait_node->abandoned = 0;
+    p_wait_node->abandoned = __LT_SPINLOCK_INIT;
     p_wait_node->next = (struct wait_node *) oldstatus;
     /* Make sure the store in wait_node.next completes before performing
        the compare-and-swap */
@@ -574,7 +574,7 @@
     while (p_node != (struct wait_node *) 1) {
       int prio;
 
-      if (p_node->abandoned) {
+      if (lock_held(&p_node->abandoned)) {
 	/* Remove abandoned node. */
 #if defined TEST_FOR_COMPARE_AND_SWAP
 	if (!__pthread_has_cas)
@@ -662,7 +662,7 @@
 #if !defined HAS_COMPARE_AND_SWAP || defined TEST_FOR_COMPARE_AND_SWAP
 
 int __pthread_compare_and_swap(long * ptr, long oldval, long newval,
-                               int * spinlock)
+                               __atomic_lock_t * spinlock)
 {
   int res;
 
@@ -699,7 +699,7 @@
    - When nanosleep() returns, we try again, doing MAX_SPIN_COUNT
      sched_yield(), then sleeping again if needed. */
 
-static void __pthread_acquire(int * spinlock)
+static void __pthread_acquire(__atomic_lock_t * spinlock)	
 {
   int cnt = 0;
   struct timespec tm;
linuxthreads/spinlock.h +20 -6
diff -urN glibc-2.3.1.orig/linuxthreads/spinlock.h glibc-2.3.1/linuxthreads/spinlock.h
@@ -33,14 +33,28 @@
 #endif
 #endif
 
+/* Define lock_held for all arches that don't need a modified copy. */
+#ifndef __LT_INITIALIZER_NOT_ZERO
+# define lock_held(p) *(p)
+#endif
+
+/* Initliazers for possibly complex structures */
+#ifdef __LT_INITIALIZER_NOT_ZERO
+# define __pthread_lock_define_initialized(CLASS,NAME) \
+	CLASS __atomic_lock_t NAME = __LT_SPINLOCK_ALT_INIT
+#else
+# define __pthread_lock_define_initialized(CLASS,NAME) \
+	CLASS __atomic_lock_t NAME
+#endif
+
 #if defined(TEST_FOR_COMPARE_AND_SWAP)
 
 extern int __pthread_has_cas;
 extern int __pthread_compare_and_swap(long * ptr, long oldval, long newval,
-                                      int * spinlock);
+                                      __atomic_lock_t * spinlock);
 
 static inline int compare_and_swap(long * ptr, long oldval, long newval,
-                                   int * spinlock)
+                                   __atomic_lock_t * spinlock)
 {
   if (__builtin_expect (__pthread_has_cas, 1))
     return __compare_and_swap(ptr, oldval, newval);
@@ -58,7 +72,7 @@
 
 static inline int
 compare_and_swap_with_release_semantics (long * ptr, long oldval,
-					 long newval, int * spinlock)
+					 long newval, __atomic_lock_t * spinlock)
 {
   return __compare_and_swap_with_release_semantics (ptr, oldval,
 						    newval);
@@ -67,7 +81,7 @@
 #endif
 
 static inline int compare_and_swap(long * ptr, long oldval, long newval,
-                                   int * spinlock)
+                                   __atomic_lock_t * spinlock)
 {
   return __compare_and_swap(ptr, oldval, newval);
 }
@@ -75,10 +89,10 @@
 #else
 
 extern int __pthread_compare_and_swap(long * ptr, long oldval, long newval,
-                                      int * spinlock);
+                                      __atomic_lock_t * spinlock);
 
 static inline int compare_and_swap(long * ptr, long oldval, long newval,
-                                   int * spinlock)
+                                   __atomic_lock_t * spinlock)
 {
   return __pthread_compare_and_swap(ptr, oldval, newval, spinlock);
 }
linuxthreads/sysdeps/hppa/pspinlock.c +13 -16
diff -urN glibc-2.3.1.orig/linuxthreads/sysdeps/hppa/pspinlock.c glibc-2.3.1/linuxthreads/sysdeps/hppa/pspinlock.c
@@ -24,15 +24,12 @@
 int
 __pthread_spin_lock (pthread_spinlock_t *lock)
 {
-  unsigned int val;
+	unsigned int *addr = __ldcw_align (lock);
+	
+	while (__ldcw (addr) == 0)
+		while (*addr == 0) ;
 
-  do
-    asm volatile ("ldcw %1,%0"
-		  : "=r" (val), "=m" (*lock)
-		  : "m" (*lock));
-  while (!val);
-
-  return 0;
+	return 0;
 }
 weak_alias (__pthread_spin_lock, pthread_spin_lock)
 
@@ -40,13 +37,9 @@
 int
 __pthread_spin_trylock (pthread_spinlock_t *lock)
 {
-  unsigned int val;
-
-  asm volatile ("ldcw %1,%0"
-		: "=r" (val), "=m" (*lock)
-		: "m" (*lock));
+	unsigned int *a = __ldcw_align (lock);
 
-  return val ? 0 : EBUSY;
+	return __ldcw (a) ? 0 : EBUSY;
 }
 weak_alias (__pthread_spin_trylock, pthread_spin_trylock)
 
@@ -54,7 +47,9 @@
 int
 __pthread_spin_unlock (pthread_spinlock_t *lock)
 {
-  *lock = 1;
+  unsigned int *a = __ldcw_align (lock);
+
+  *a = 1;
   return 0;
 }
 weak_alias (__pthread_spin_unlock, pthread_spin_unlock)
@@ -66,7 +61,9 @@
   /* We can ignore the `pshared' parameter.  Since we are busy-waiting
      all processes which can access the memory location `lock' points
      to can use the spinlock.  */
-  *lock = 1;
+  unsigned int *a = __ldcw_align (lock);
+
+  *a = 1;
   return 0;
 }
 weak_alias (__pthread_spin_init, pthread_spin_init)
linuxthreads/sysdeps/hppa/pt-machine.h +67 -14

107 lines not on this page · raw message

linuxthreads/sysdeps/pthread/bits/libc-lock.h +3 -3
diff -urN glibc-2.3.1.orig/linuxthreads/sysdeps/pthread/bits/libc-lock.h glibc-2.3.1/linuxthreads/sysdeps/pthread/bits/libc-lock.h
@@ -71,12 +71,12 @@
    initialized locks must be set to one due to the lack of normal
    atomic operations.) */
 
-#if __LT_SPINLOCK_INIT == 0
+#ifdef __LT_INITIALIZER_NOT_ZERO
 #  define __libc_lock_define_initialized(CLASS,NAME) \
-  CLASS __libc_lock_t NAME;
+  CLASS __libc_lock_t NAME = PTHREAD_MUTEX_INITIALIZER;
 #else
 #  define __libc_lock_define_initialized(CLASS,NAME) \
-  CLASS __libc_lock_t NAME = PTHREAD_MUTEX_INITIALIZER;
+  CLASS __libc_lock_t NAME;
 #endif
 
 #define __libc_rwlock_define_initialized(CLASS,NAME) \
linuxthreads/sysdeps/pthread/bits/pthreadtypes.h +5 -3
diff -urN glibc-2.3.1.orig/linuxthreads/sysdeps/pthread/bits/pthreadtypes.h glibc-2.3.1/linuxthreads/sysdeps/pthread/bits/pthreadtypes.h
@@ -22,12 +22,14 @@
 #define __need_schedparam
 #include <bits/sched.h>
 
+typedef int __atomic_lock_t;
+
 /* Fast locks (not abstract because mutexes and conditions aren't abstract). */
 struct _pthread_fastlock
 {
-  long int __status;   /* "Free" or "taken" or head of waiting list */
-  int __spinlock;      /* Used by compare_and_swap emulation. Also,
-			  adaptive SMP lock stores spin count here. */
+  long int __status;		/* "Free" or "taken" or head of waiting list */
+  __atomic_lock_t __spinlock;	/* Used by compare_and_swap emulation. Also,
+				   adaptive SMP lock stores spin count here. */
 };
 
 #ifndef _PTHREAD_DESCR_DEFINED
linuxthreads/sysdeps/unix/sysv/linux/hppa/bits/initspin.h +17 -5
diff -urN glibc-2.3.1.orig/linuxthreads/sysdeps/unix/sysv/linux/hppa/bits/initspin.h glibc-2.3.1/linuxthreads/sysdeps/unix/sysv/linux/hppa/bits/initspin.h
@@ -17,11 +17,23 @@
    write to the Free Software Foundation, Inc., 59 Temple Place - Suite 330,
    Boston, MA 02111-1307, USA.  */
 
+/* Initialize global spinlocks without cast, generally macro wrapped */
+#define __LT_SPINLOCK_ALT_INIT { { 1, 1, 1, 1, } }
+
 /* Initial value of a spinlock.  PA-RISC only implements atomic load
    and clear so this must be non-zero. */
-#define __LT_SPINLOCK_INIT 1
+#define __LT_SPINLOCK_INIT ((__atomic_lock_t) __LT_SPINLOCK_ALT_INIT)
+
+/* Macros for lock initializers, not using the above definition.
+   The above definition is not used in the case that static initializers
+   use this value. */
+#define __LOCK_INITIALIZER { 0, __LT_SPINLOCK_ALT_INIT }
+#define __ATOMIC_INITIALIZER { 0, __LT_SPINLOCK_ALT_INIT }
+
+/* Used to initialize _pthread_fastlock's in non-static case */
+#define __LOCK_ALT_INITIALIZER ((struct _pthread_fastlock){ 0, __LT_SPINLOCK_INIT })
+
+/* Tell the rest of the code that the initializer is non-zero without
+   explaining it's internal structure */
+#define __LT_INITIALIZER_NOT_ZERO
 
-/* Macros for lock initializers, using the above definition. */
-#define __LOCK_INITIALIZER { 0, __LT_SPINLOCK_INIT }
-#define __ALT_LOCK_INITIALIZER { 0, __LT_SPINLOCK_INIT }
-#define __ATOMIC_INITIALIZER { 0, __LT_SPINLOCK_INIT }
#1 lore raw

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Quoted 4 lines · Carlos O'Donell

This is a cleaner HPPA linuxthreads implementation that stems from the work that John David Anglin <[email protected]> and myself did to devise a self-aligning lock system that doesn't impose the 16-byte lock alignment restriction.

The indentation and general style is wrong in many places.

linuxthreads/sysdeps/hppa/pspinlock.c line 30

Quoted 12 lines

diff -urN glibc-2.3.1.orig/linuxthreads/sysdeps/hppa/pspinlock.c glibc-2.3.1/linuxthreads/sysdeps/hppa/pspinlock.c

linuxthreads/sysdeps/hppa/pspinlock.c +4 -1
@@ -24,15 +24,12 @@
 int
 __pthread_spin_lock (pthread_spinlock_t *lock)
 {
-  unsigned int val;
+	unsigned int *addr = __ldcw_align (lock);
+	
+	while (__ldcw (addr) == 0)
+		while (*addr == 0) ;

This is plain wrong. addr at least must be volatile.

And I don't understand why you removed the asm code. These pieces of code are prime candidates for hand-coding.

linuxthreads/sysdeps/hppa/pspinlock.c

Quoted 6 lines
linuxthreads/sysdeps/hppa/pspinlock.c +6 -0
+static inline struct _pthread_descr_struct * __get_cr27(void)
+{
+	long cr27;
+	asm("mfctl %%cr27, %0" : "=r" (cr27) : );
+	return (struct _pthread_descr_struct *) cr27;
+}

Not a real problem, but you should get gcc to recognize this reqister and perform the loading.

- --
- --------------. ,-. 444 Castro Street
Ulrich Drepper \ ,-----------------' \ Mountain View, CA 94041 USA
Red Hat --' drepper at redhat.com ---------------------------
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.1 (GNU/Linux)

iD8DBQE+y74l2ijCOnn/RHQRAvZ1AKCkAhua5qA19EDylfBz2Zhp8dROIACfTrwO 9Slpj32aUy92PiggDNexZBE=
=6Ilf
-----END PGP SIGNATURE-----

#2 lore raw
In reply to Carlos O'Donell

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Quoted 4 lines · Carlos O'Donell

This is a cleaner HPPA linuxthreads implementation that stems from the work that John David Anglin <[email protected]> and myself did to devise a self-aligning lock system that doesn't impose the 16-byte lock alignment restriction.

The indentation and general style is wrong in many places.

linuxthreads/sysdeps/hppa/pspinlock.c line 30

Quoted 12 lines

diff -urN glibc-2.3.1.orig/linuxthreads/sysdeps/hppa/pspinlock.c glibc-2.3.1/linuxthreads/sysdeps/hppa/pspinlock.c

linuxthreads/sysdeps/hppa/pspinlock.c +4 -1
@@ -24,15 +24,12 @@
 int
 __pthread_spin_lock (pthread_spinlock_t *lock)
 {
-  unsigned int val;
+	unsigned int *addr = __ldcw_align (lock);
+	
+	while (__ldcw (addr) == 0)
+		while (*addr == 0) ;

This is plain wrong. addr at least must be volatile.

And I don't understand why you removed the asm code. These pieces of code are prime candidates for hand-coding.

linuxthreads/sysdeps/hppa/pspinlock.c

Quoted 6 lines
linuxthreads/sysdeps/hppa/pspinlock.c +6 -0
+static inline struct _pthread_descr_struct * __get_cr27(void)
+{
+	long cr27;
+	asm("mfctl %%cr27, %0" : "=r" (cr27) : );
+	return (struct _pthread_descr_struct *) cr27;
+}

Not a real problem, but you should get gcc to recognize this reqister and perform the loading.

- --
- --------------. ,-. 444 Castro Street
Ulrich Drepper \ ,-----------------' \ Mountain View, CA 94041 USA
Red Hat --' drepper at redhat.com ---------------------------
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.1 (GNU/Linux)

iD8DBQE+y74l2ijCOnn/RHQRAvZ1AKCkAhua5qA19EDylfBz2Zhp8dROIACfTrwO 9Slpj32aUy92PiggDNexZBE=
=6Ilf
-----END PGP SIGNATURE-----

#3 lore raw

Ulrich,

Quoted 1 line

The indentation and general style is wrong in many places.

I'll clean this up today and break it into three chunks for even easier digestion.

Quoted 9 lines

linuxthreads/sysdeps/hppa/pspinlock.c line 30

Quoted 7 lines

__pthread_spin_lock (pthread_spinlock_t *lock)
{

linuxthreads/sysdeps/hppa/pspinlock.c +4 -1
-  unsigned int val;
+	unsigned int *addr = __ldcw_align (lock);
+	
+	while (__ldcw (addr) == 0)
+		while (*addr == 0) ;

This is plain wrong. addr at least must be volatile.

__ldcw is volatile.

Are you suggesting that it is prehaps fragile based on gcc's choice of optimization?

Quoted 2 lines

And I don't understand why you removed the asm code. These pieces of code are prime candidates for hand-coding.

__ldcw is still assembly only. __ldcw_align is a macro. In truth I wanted a functioning implementation first, optimization is a close second. Though a full assembly version would fix gcc from reordering anything.

Quoted 9 lines

linuxthreads/sysdeps/hppa/pt-machine.h line 49

Quoted 6 lines
linuxthreads/sysdeps/hppa/pt-machine.h +6 -0
+static inline struct _pthread_descr_struct * __get_cr27(void)
+{
+	long cr27;
+	asm("mfctl %%cr27, %0" : "=r" (cr27) : );
+	return (struct _pthread_descr_struct *) cr27;
+}

Not a real problem, but you should get gcc to recognize this reqister and perform the loading.

This should get addressed as TLS is implemented for HPPA. I will be attending gcc-summit in hopes to discuss this with my colleagues.

c.

#4 lore raw
Quoted 6 lines · Carlos O'Donell
Quoted 4 lines

linuxthreads/sysdeps/hppa/pspinlock.c line 30

Quoted 2 lines
linuxthreads/sysdeps/hppa/pspinlock.c +2 -0
+	while (__ldcw (addr) == 0)
+		while (*addr == 0) ;

This is plain wrong. addr at least must be volatile.

__ldcw is volatile.

yes, but *addr can be optimized out of the while loop.

Quoted 2 lines

Are you suggesting that it is prehaps fragile based on gcc's choice of optimization?

That's another issue. OTOH, I doubt gcc will ever mangle this fairly short chunk of code. Maybe post the gcc asm output and see if anyone can improve it.

grant

#5 lore raw
Quoted 9 lines · Carlos O'Donell
Quoted 6 lines
Quoted 4 lines

linuxthreads/sysdeps/hppa/pspinlock.c line 30

Quoted 2 lines
linuxthreads/sysdeps/hppa/pspinlock.c +2 -0
+	while (__ldcw (addr) == 0)
+		while (*addr == 0) ;

This is plain wrong. addr at least must be volatile.

__ldcw is volatile.

yes, but *addr can be optimized out of the while loop.

Yes, you are correct. This is the code from the inner loop:

  1c:   0f 40 10 93     ldw 0(,r26),r19
  20:   86 60 3f f5     cmpib,= 0,r19,20 <__pthread_spin_lock+0x20>
  24:   08 00 02 40     nop

Don't know how I missied that :(

Dave

Signature · 2 lines
J. David Anglin                                  [email protected]
National Research Council of Canada              (613) 990-0752 (FAX: 952-6602)
#6 lore raw
In reply to Grant Grundler
Quoted 9 lines · Carlos O'Donell
Quoted 6 lines
Quoted 4 lines

linuxthreads/sysdeps/hppa/pspinlock.c line 30

Quoted 2 lines
linuxthreads/sysdeps/hppa/pspinlock.c +2 -0
+	while (__ldcw (addr) == 0)
+		while (*addr == 0) ;

This is plain wrong. addr at least must be volatile.

__ldcw is volatile.

yes, but *addr can be optimized out of the while loop.

Yes, you are correct. This is the code from the inner loop:

  1c:   0f 40 10 93     ldw 0(,r26),r19
  20:   86 60 3f f5     cmpib,= 0,r19,20 <__pthread_spin_lock+0x20>
  24:   08 00 02 40     nop

Don't know how I missied that :(

Dave

Signature · 2 lines
J. David Anglin                                  [email protected]
National Research Council of Canada              (613) 990-0752 (FAX: 952-6602)
#7 lore raw
Quoted 1 line · John David Anglin

Don't know how I missied that :(

If you have a test for this, it's not contending for the lock. Writing a test for this would probably be a good thing if there isn't already one. Note there is no fairness and a test would need to be sensitive to that.

grant

#8 lore raw
Quoted 4 lines

If you have a test for this, it's not contending for the lock. Writing a test for this would probably be a good thing if there isn't already one. Note there is no fairness and a test would need to be sensitive to that.

Agreed. The libc suite would probably be the best place for such a test ;-)

Dave

Signature · 2 lines
J. David Anglin                                  [email protected]
National Research Council of Canada              (613) 990-0752 (FAX: 952-6602)
#9 lore raw
In reply to Grant Grundler
Quoted 4 lines

If you have a test for this, it's not contending for the lock. Writing a test for this would probably be a good thing if there isn't already one. Note there is no fairness and a test would need to be sensitive to that.

Agreed. The libc suite would probably be the best place for such a test ;-)

Dave

Signature · 2 lines
J. David Anglin                                  [email protected]
National Research Council of Canada              (613) 990-0752 (FAX: 952-6602)
#10 lore raw
In reply to John David Anglin
Quoted 1 line · John David Anglin

Don't know how I missied that :(

If you have a test for this, it's not contending for the lock. Writing a test for this would probably be a good thing if there isn't already one. Note there is no fairness and a test would need to be sensitive to that.

grant

#11 lore raw
In reply to Carlos O'Donell
Quoted 6 lines · Carlos O'Donell
Quoted 4 lines

linuxthreads/sysdeps/hppa/pspinlock.c line 30

Quoted 2 lines
linuxthreads/sysdeps/hppa/pspinlock.c +2 -0
+	while (__ldcw (addr) == 0)
+		while (*addr == 0) ;

This is plain wrong. addr at least must be volatile.

__ldcw is volatile.

yes, but *addr can be optimized out of the while loop.

Quoted 2 lines

Are you suggesting that it is prehaps fragile based on gcc's choice of optimization?

That's another issue. OTOH, I doubt gcc will ever mangle this fairly short chunk of code. Maybe post the gcc asm output and see if anyone can improve it.

grant

#12 lore raw
In reply to Ulrich Drepper

Ulrich,

Quoted 1 line

The indentation and general style is wrong in many places.

I'll clean this up today and break it into three chunks for even easier digestion.

Quoted 9 lines

linuxthreads/sysdeps/hppa/pspinlock.c line 30

Quoted 7 lines

__pthread_spin_lock (pthread_spinlock_t *lock)
{

linuxthreads/sysdeps/hppa/pspinlock.c +4 -1
-  unsigned int val;
+	unsigned int *addr = __ldcw_align (lock);
+	
+	while (__ldcw (addr) == 0)
+		while (*addr == 0) ;

This is plain wrong. addr at least must be volatile.

__ldcw is volatile.

Are you suggesting that it is prehaps fragile based on gcc's choice of optimization?

Quoted 2 lines

And I don't understand why you removed the asm code. These pieces of code are prime candidates for hand-coding.

__ldcw is still assembly only. __ldcw_align is a macro. In truth I wanted a functioning implementation first, optimization is a close second. Though a full assembly version would fix gcc from reordering anything.

Quoted 9 lines

linuxthreads/sysdeps/hppa/pt-machine.h line 49

Quoted 6 lines
linuxthreads/sysdeps/hppa/pt-machine.h +6 -0
+static inline struct _pthread_descr_struct * __get_cr27(void)
+{
+	long cr27;
+	asm("mfctl %%cr27, %0" : "=r" (cr27) : );
+	return (struct _pthread_descr_struct *) cr27;
+}

Not a real problem, but you should get gcc to recognize this reqister and perform the loading.

This should get addressed as TLS is implemented for HPPA. I will be attending gcc-summit in hopes to discuss this with my colleagues.

c.