This is the mail archive of the libc-hacker@sourceware.org mailing list for the glibc project.

Note that libc-hacker is a closed list. You may look at the archives of this list, but subscription and posting are not open.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] clean up PPC for private futex changes.


On Mon, Jul 23, 2007 at 09:10:27AM -0700, Ulrich Drepper wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> Applied.

Unfortunately that patch contains several important bugs:
1) lll_futex_wait etc. had only correct definition when
   __ASSUME_PRIVATE_FUTEX, when that is not true, it would
   call futex syscall with bit 7 set even when libpthread.so
   init determined it is not supported
2) lll_private_futex_wait etc. macros were shared when
   not __ASSUME_PRIVATE_FUTEX
3) FUTEX_WAKE_OP had the wake operation argument also ored
   with 128 in some cases

Here is how it IMHO should look like (built and tested on ppc64-linux,
unfortunately not wiht a 2.6.23ish kernel).
The x86_64 changes are untested.

2007-07-23  Jakub Jelinek  <jakub@redhat.com>

	* sysdeps/unix/sysv/linux/powerpc/lowlevellock.h
	(__lll_private_flag): Define.
	(lll_futex_wait): Define as a wrapper around lll_futex_timed_wait.
	(lll_futex_timed_wait, lll_futex_wake, lll_futex_wake_unlock): Use
	__lll_private_flag.
	(lll_private_futex_wait, lll_private_futex_timedwait,
	lll_private_futex_wake): Define as wrapper around non-_private
	macros.
	* sysdeps/unix/sysv/linux/x86_64/lowlevellock.h
	(__lll_private_flag): Define.
	(lll_futex_timed_wait, lll_futex_wake): Use __lll_private_flag.
	(lll_private_futex_wait, lll_private_futex_timedwait,
	lll_private_futex_wake): Define as wrapper around non-_private
	macros.

--- libc/nptl/sysdeps/unix/sysv/linux/powerpc/lowlevellock.h.jj	2007-07-23 19:36:30.000000000 +0200
+++ libc/nptl/sysdeps/unix/sysv/linux/powerpc/lowlevellock.h	2007-07-23 21:36:34.000000000 +0200
@@ -45,40 +45,55 @@
 #define LLL_PRIVATE	0
 #define LLL_SHARED	FUTEX_PRIVATE_FLAG
 
+#if !defined NOT_IN_libc || defined IS_IN_rtld
+/* In libc.so or ld.so all futexes are private.  */
+# ifdef __ASSUME_PRIVATE_FUTEX
+#  define __lll_private_flag(fl, private) \
+  ((fl) | FUTEX_PRIVATE_FLAG)
+# else
+#  define __lll_private_flag(fl, private) \
+  ((fl) | THREAD_GETMEM (THREAD_SELF, header.private_futex))
+# endif
+#else
+# ifdef __ASSUME_PRIVATE_FUTEX
+#  define __lll_private_flag(fl, private) \
+  (((fl) | FUTEX_PRIVATE_FLAG) ^ (private))
+# else
+#  define __lll_private_flag(fl, private) \
+  (__builtin_constant_p (private)					      \
+   ? ((private) == 0							      \
+      ? ((fl) | THREAD_GETMEM (THREAD_SELF, header.private_futex))	      \
+      : (fl))								      \
+   : ((fl) | (((private) ^ FUTEX_PRIVATE_FLAG)				      \
+	      & THREAD_GETMEM (THREAD_SELF, header.private_futex))))
+# endif	      
+#endif
 
 /* Initializer for compatibility lock.	*/
 #define LLL_MUTEX_LOCK_INITIALIZER (0)
 
 #define lll_futex_wait(futexp, val, private) \
-  ({									      \
-    INTERNAL_SYSCALL_DECL (__err);					      \
-    long int opt_flags = (FUTEX_WAIT | LLL_SHARED) ^ private;		      \
-    long int __ret;							      \
-									      \
-    __ret = INTERNAL_SYSCALL (futex, __err, 4,				      \
-			      (futexp), opt_flags, (val), 0);		      \
-    INTERNAL_SYSCALL_ERROR_P (__ret, __err) ? -__ret : __ret;		      \
-  })
+  lll_futex_timed_wait (futexp, val, NULL, private)
 
 #define lll_futex_timed_wait(futexp, val, timespec, private) \
   ({									      \
     INTERNAL_SYSCALL_DECL (__err);					      \
-    long int opt_flags = (FUTEX_WAIT | LLL_SHARED) ^ private;		      \
     long int __ret;							      \
 									      \
-    __ret = INTERNAL_SYSCALL (futex, __err, 4,				      \
-			      (futexp), opt_flags, (val), (timespec));	      \
+    __ret = INTERNAL_SYSCALL (futex, __err, 4, (futexp),		      \
+			      __lll_private_flag (FUTEX_WAIT, private),	      \
+			      (val), (timespec));			      \
     INTERNAL_SYSCALL_ERROR_P (__ret, __err) ? -__ret : __ret;		      \
   })
 
 #define lll_futex_wake(futexp, nr, private) \
   ({									      \
     INTERNAL_SYSCALL_DECL (__err);					      \
-    long int opt_flags = (FUTEX_WAKE | LLL_SHARED) ^ private;		      \
     long int __ret;							      \
 									      \
-    __ret = INTERNAL_SYSCALL (futex, __err, 4,				      \
-			      (futexp), opt_flags, (nr), 0);		      \
+    __ret = INTERNAL_SYSCALL (futex, __err, 4, (futexp),		      \
+			      __lll_private_flag (FUTEX_WAKE, private),	      \
+			      (nr), 0);					      \
     INTERNAL_SYSCALL_ERROR_P (__ret, __err) ? -__ret : __ret;		      \
   })
 
@@ -109,66 +124,24 @@
 #define lll_futex_wake_unlock(futexp, nr_wake, nr_wake2, futexp2, private) \
   ({									      \
     INTERNAL_SYSCALL_DECL (__err);					      \
-    long int opt_flags = (FUTEX_WAKE_OP | LLL_SHARED) ^ private;	      \
-    long int opt_flag2 = (FUTEX_OP_CLEAR_WAKE_IF_GT_ONE | LLL_SHARED)	      \
-                          ^ private;					      \
     long int __ret;							      \
 									      \
-    __ret = INTERNAL_SYSCALL (futex, __err, 6,				      \
-			      (futexp), opt_flags, (nr_wake),		      \
-			      (nr_wake2), (futexp2),			      \
-			      opt_flag2);				      \
+    __ret = INTERNAL_SYSCALL (futex, __err, 6, (futexp),		      \
+			      __lll_private_flag (FUTEX_WAKE_OP, private),    \
+			      (nr_wake), (nr_wake2), (futexp2),		      \
+			      FUTEX_OP_CLEAR_WAKE_IF_GT_ONE);		      \
     INTERNAL_SYSCALL_ERROR_P (__ret, __err);				      \
   })
   
   
 #define lll_private_futex_wait(futexp, val) \
-  lll_private_futex_timed_wait (futexp, val, NULL)
-
-
-#ifdef __ASSUME_PRIVATE_FUTEX  
-# define lll_private_futex_timed_wait(futexp, val, timeout) \
-  ({									      \
-    INTERNAL_SYSCALL_DECL (__err);					      \
-    long int __ret;							      \
-									      \
-    __ret = INTERNAL_SYSCALL (futex, __err, 4,				      \
-			      (futexp), (FUTEX_WAIT | FUTEX_PRIVATE_FLAG),    \
-			      (val), (timeout));			      \
-    INTERNAL_SYSCALL_ERROR_P (__ret, __err) ? -__ret : __ret;		      \
-  })
+  lll_futex_timed_wait (futexp, val, NULL, LLL_PRIVATE)
 
-# define lll_private_futex_wake(futexp, val) \
-  ({									      \
-    INTERNAL_SYSCALL_DECL (__err);					      \
-    long int __ret;							      \
-									      \
-    __ret = INTERNAL_SYSCALL (futex, __err, 4,				      \
-			      (futexp), (FUTEX_WAKE | FUTEX_PRIVATE_FLAG),    \
-			      (val), 0);				      \
-    INTERNAL_SYSCALL_ERROR_P (__ret, __err) ? -__ret : __ret;		      \
-  })
-#else
-# define lll_private_futex_timed_wait(futexp, val, timeout) \
-  ({									      \
-    INTERNAL_SYSCALL_DECL (__err);					      \
-    long int __ret;							      \
-									      \
-    __ret = INTERNAL_SYSCALL (futex, __err, 4,				      \
-			      (futexp), FUTEX_WAIT, (val), (timeout));	      \
-    INTERNAL_SYSCALL_ERROR_P (__ret, __err) ? -__ret : __ret;		      \
-  })
+#define lll_private_futex_timed_wait(futexp, val, timeout) \
+  lll_futex_timed_wait (futexp, val, timeout, LLL_PRIVATE)
 
-# define lll_private_futex_wake(futexp, val) \
-  ({									      \
-    INTERNAL_SYSCALL_DECL (__err);					      \
-    long int __ret;							      \
-									      \
-    __ret = INTERNAL_SYSCALL (futex, __err, 4,				      \
-			      (futexp), FUTEX_WAKE, (val), 0);		      \
-    INTERNAL_SYSCALL_ERROR_P (__ret, __err) ? -__ret : __ret;		      \
-  })
-#endif
+#define lll_private_futex_wake(futexp, val) \
+  lll_futex_wake (futexp, val, LLL_PRIVATE)
 
 #ifdef UP
 # define __lll_acq_instr	""
--- libc/nptl/sysdeps/unix/sysv/linux/x86_64/lowlevellock.h.jj	2007-06-19 13:10:21.000000000 +0200
+++ libc/nptl/sysdeps/unix/sysv/linux/x86_64/lowlevellock.h	2007-07-23 21:56:00.000000000 +0200
@@ -50,6 +50,31 @@
 #define LLL_PRIVATE	0
 #define LLL_SHARED	FUTEX_PRIVATE_FLAG
 
+#if !defined NOT_IN_libc || defined IS_IN_rtld
+/* In libc.so or ld.so all futexes are private.  */
+# ifdef __ASSUME_PRIVATE_FUTEX
+#  define __lll_private_flag(fl, private) \
+  ((fl) | FUTEX_PRIVATE_FLAG)
+# else
+#  define __lll_private_flag(fl, private) \
+  ((fl) | THREAD_GETMEM (THREAD_SELF, header.private_futex))
+# endif
+#else
+# ifdef __ASSUME_PRIVATE_FUTEX
+#  define __lll_private_flag(fl, private) \
+  (((fl) | FUTEX_PRIVATE_FLAG) ^ (private))
+# else
+#  define __lll_private_flag(fl, private) \
+  (__builtin_constant_p (private)					      \
+   ? ((private) == 0							      \
+      ? ((fl) | THREAD_GETMEM (THREAD_SELF, header.private_futex))	      \
+      : (fl))								      \
+   : ({ unsigned int __fl = ((private) ^ FUTEX_PRIVATE_FLAG);		      \
+	asm ("andl %%fs:%P1, %0" : "+r" (__fl)				      \
+	     : "i" offsetof (struct pthread, header.private_futex));	      \
+	__fl | (fl); })
+# endif	      
+#endif
 
 /* Initializer for compatibility lock.  */
 #define LLL_MUTEX_LOCK_INITIALIZER		(0)
@@ -169,7 +194,8 @@ LLL_STUB_UNWIND_INFO_END
     register __typeof (val) _val __asm ("edx") = (val);			      \
     __asm __volatile ("syscall"						      \
 		      : "=a" (__status)					      \
-		      : "0" (SYS_futex), "D" (futex), "S" (FUTEX_WAIT),	      \
+		      : "0" (SYS_futex), "D" (futex),			      \
+			"S" (__lll_private_flag (FUTEX_WAIT, private)),	      \
 		        "d" (_val), "r" (__to)				      \
 		      : "memory", "cc", "r11", "cx");			      \
     __status;								      \
@@ -182,73 +208,21 @@ LLL_STUB_UNWIND_INFO_END
     register __typeof (nr) _nr __asm ("edx") = (nr);			      \
     __asm __volatile ("syscall"						      \
 		      : "=a" (__ignore)					      \
-		      : "0" (SYS_futex), "D" (futex), "S" (FUTEX_WAKE),	      \
+		      : "0" (SYS_futex), "D" (futex),			      \
+			"S" (__lll_private_flag (FUTEX_WAKE, private)),	      \
 			"d" (_nr)					      \
 		      : "memory", "cc", "r10", "r11", "cx");		      \
   } while (0)
 
 
 #define lll_private_futex_wait(futex, val) \
-  lll_private_futex_timed_wait (futex, val, NULL)
-
-
-#ifdef __ASSUME_PRIVATE_FUTEX
-# define lll_private_futex_timed_wait(futex, val, timeout) \
-  ({									      \
-    register const struct timespec *__to __asm ("r10") = timeout;	      \
-    int __status;							      \
-    register __typeof (val) _val __asm ("edx") = (val);			      \
-    __asm __volatile ("syscall"						      \
-		      : "=a" (__status)					      \
-		      : "0" (SYS_futex), "D" (futex),			      \
-			"S" (FUTEX_WAIT | FUTEX_PRIVATE_FLAG),		      \
-		        "d" (_val), "r" (__to)				      \
-		      : "memory", "cc", "r11", "cx");			      \
-    __status;								      \
-  })
-
-
-# define lll_private_futex_wake(futex, nr) \
-  do {									      \
-    int __ignore;							      \
-    register __typeof (nr) _nr __asm ("edx") = (nr);			      \
-    __asm __volatile ("syscall"						      \
-		      : "=a" (__ignore)					      \
-		      : "0" (SYS_futex), "D" (futex),			      \
-			"S" (FUTEX_WAKE | FUTEX_PRIVATE_FLAG),		      \
-			"d" (_nr)					      \
-		      : "memory", "cc", "r10", "r11", "cx");		      \
-  } while (0)
-#else
-# define lll_private_futex_timed_wait(futex, val, timeout) \
-  ({									      \
-    register const struct timespec *__to __asm ("r10") = timeout;	      \
-    int __status;							      \
-    int __ignore;							      \
-    register __typeof (val) _val __asm ("edx") = (val);			      \
-    __asm __volatile ("movl %%fs:%P3, %%esi\n\t"			      \
-		      "syscall"						      \
-		      : "=a" (__status), "=S" (__ignore)		      \
-		      : "0" (SYS_futex), "i" (PRIVATE_FUTEX), "D" (futex),    \
-		        "d" (_val), "r" (__to)				      \
-		      : "memory", "cc", "r11", "cx");			      \
-    __status;								      \
-  })
+  lll_futex_timed_wait (futex, val, NULL, LLL_PRIVATE)
 
+#define lll_private_futex_timed_wait(futex, val, timeout) \
+  lll_futex_timed_wait (futex, val, timeout, LLL_PRIVATE)
 
-# define lll_private_futex_wake(futex, nr) \
-  do {									      \
-    int __ignore;							      \
-    int __ignore2;							      \
-    register __typeof (nr) _nr __asm ("edx") = (nr);			      \
-    __asm __volatile ("orl %%fs:%P3, %%esi\n\t"				      \
-		      "syscall"						      \
-		      : "=a" (__ignore), "=S" (__ignore2)		      \
-		      : "0" (SYS_futex), "i" (PRIVATE_FUTEX), "D" (futex),    \
-			"1" (FUTEX_WAKE), "d" (_nr)			      \
-		      : "memory", "cc", "r10", "r11", "cx");		      \
-  } while (0)
-#endif
+#define lll_private_futex_wake(futex, nr) \
+  lll_futex_wake (futex, nr, LLL_PRIVATE)
 
 
 /* Does not preserve %eax and %ecx.  */

	Jakub


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]