Re: [RFC][BZ #13690] Always read private before lll_unlock.

["Ondřej Bílka" <neleai@seznam.cz>]

On Fri, Dec 06, 2013 at 04:11:58PM -0500, Mike Frysinger wrote:
> On Friday 06 December 2013 14:43:05 Ondřej Bílka wrote:
> > --- /dev/null
> > +++ b/include/futex_unlock.h
> 
> probably should live at nptl/lowlevellock_unlock
> 
or better name, see below.
> > @@ -0,0 +1,15 @@
> > +#define lll_unlock(lock, private) \
> 
> all new files need a proper comment header block
> 
> > +  ((void)) ({								      \
> > +    int __private = private;						      \
> > +    __lll_unlock (lock, __private);					      \
> > +  })
> > +
> > +#define lll_unlock(lock, private) \
> 
> did i misread, or are both of these macros named "lll_unlock" ?  should one 
> have a __ prefix ?
>
yes, i missed that, thanks.


Now when I looked to implementations we need more radical refactoring,

These implementatation were created by copying so headers are mostly
identical except of cosmetic changes like macro/inline function, added
expect and similar.

Some architectures need different primitives than atomic_compare_and_exchange_val_acq/rel but 
it looks most do not need that.

As example a diff -uw between arm and tile is following:

--- ports/sysdeps/unix/sysv/linux/tile/nptl/lowlevellock.h	2013-12-06 20:17:57.092514754 +0100
+++ ports/sysdeps/unix/sysv/linux/arm/nptl/lowlevellock.h	2013-12-06 20:19:28.855381949 +0100
@@ -1,6 +1,5 @@
-/* Copyright (C) 2011-2013 Free Software Foundation, Inc.
+/* Copyright (C) 2005-2013 Free Software Foundation, Inc.
    This file is part of the GNU C Library.
-   Contributed by Chris Metcalf <cmetcalf@tilera.com>, 2011.
 
    The GNU C Library is free software; you can redistribute it and/or
    modify it under the terms of the GNU Lesser General Public
@@ -26,7 +25,6 @@
 #include <sysdep.h>
 #include <kernel-features.h>
 
-
 #define FUTEX_WAIT		0
 #define FUTEX_WAKE		1
 #define FUTEX_REQUEUE		3
@@ -83,9 +81,11 @@
 #define lll_futex_timed_wait(futexp, val, timespec, private) \
   ({									      \
     INTERNAL_SYSCALL_DECL (__err);					      \
-    INTERNAL_SYSCALL (futex, __err, 4, (futexp),			      \
+    long int __ret;							      \
+    __ret = INTERNAL_SYSCALL (futex, __err, 4, (futexp),		      \
 			      __lll_private_flag (FUTEX_WAIT, private),	      \
 			      (val), (timespec));			      \
+    __ret;								      \
   })
 
 #define lll_futex_timed_wait_bitset(futexp, val, timespec, clockbit, private) \
@@ -93,7 +93,6 @@
     INTERNAL_SYSCALL_DECL (__err);					      \
     long int __ret;							      \
     int __op = FUTEX_WAIT_BITSET | clockbit;				      \
-									      \
     __ret = INTERNAL_SYSCALL (futex, __err, 6, (futexp),		      \
 			      __lll_private_flag (__op, private),	      \
 			      (val), (timespec), NULL /* Unused.  */, 	      \
@@ -104,9 +103,11 @@
 #define lll_futex_wake(futexp, nr, private) \
   ({									      \
     INTERNAL_SYSCALL_DECL (__err);					      \
-    INTERNAL_SYSCALL (futex, __err, 4, (futexp),			      \
+    long int __ret;							      \
+    __ret = INTERNAL_SYSCALL (futex, __err, 4, (futexp),		      \
 			      __lll_private_flag (FUTEX_WAKE, private),	      \
 			      (nr), 0);					      \
+    __ret;								      \
   })
 
 #define lll_robust_dead(futexv, private) \
@@ -129,6 +130,7 @@
     INTERNAL_SYSCALL_ERROR_P (__ret, __err);				      \
   })
 
+
 /* Returns non-zero if error happened, zero if success.  */
 #define lll_futex_wake_unlock(futexp, nr_wake, nr_wake2, futexp2, private) \
   ({									      \
@@ -149,13 +151,11 @@
 					mutex, private)			      \
   ({									      \
     INTERNAL_SYSCALL_DECL (__err);					      \
-    long int __ret;							      \
     int __op = FUTEX_WAIT_REQUEUE_PI | clockbit;			      \
 									      \
-    __ret = INTERNAL_SYSCALL (futex, __err, 5, (futexp),		      \
+    INTERNAL_SYSCALL (futex, __err, 5, (futexp),			      \
 			      __lll_private_flag (__op, private),	      \
 			      (val), (timespec), mutex); 		      \
-    INTERNAL_SYSCALL_ERROR_P (__ret, __err);				      \
   })
 
 #define lll_futex_cmp_requeue_pi(futexp, nr_wake, nr_move, mutex, val, priv)  \
@@ -170,27 +170,14 @@
   })
 
 
-static inline int __attribute__ ((always_inline))
-__lll_trylock (int *futex)
-{
-  return atomic_compare_and_exchange_val_acq (futex, 1, 0) != 0;
-}
-#define lll_trylock(lock)	__lll_trylock (&(lock))
-
-
-static inline int __attribute__ ((always_inline))
-__lll_cond_trylock (int *futex)
-{
-  return atomic_compare_and_exchange_val_acq (futex, 2, 0) != 0;
-}
-#define lll_cond_trylock(lock)	__lll_cond_trylock (&(lock))
-
-
-static inline int __attribute__ ((always_inline))
-__lll_robust_trylock (int *futex, int id)
-{
-  return atomic_compare_and_exchange_val_acq (futex, id, 0) != 0;
-}
+#define lll_trylock(lock)	\
+  atomic_compare_and_exchange_val_acq(&(lock), 1, 0)
+
+#define lll_cond_trylock(lock)	\
+  atomic_compare_and_exchange_val_acq(&(lock), 2, 0)
+
+#define __lll_robust_trylock(futex, id) \
+  (atomic_compare_and_exchange_val_acq (futex, id, 0) != 0)
 #define lll_robust_trylock(lock, id) \
   __lll_robust_trylock (&(lock), id)
 
@@ -198,38 +185,41 @@
 extern void __lll_lock_wait (int *futex, int private) attribute_hidden;
 extern int __lll_robust_lock_wait (int *futex, int private) attribute_hidden;
 
-static inline void __attribute__ ((always_inline))
-__lll_lock (int *futex, int private)
-{
-  if (atomic_compare_and_exchange_bool_acq (futex, 1, 0) != 0)
-    {
-      if (__builtin_constant_p (private) && private == LLL_PRIVATE)
-	__lll_lock_wait_private (futex);
-      else
-	__lll_lock_wait (futex, private);
-    }
-}
+#define __lll_lock(futex, private)					      \
+  ((void) ({								      \
+    int *__futex = (futex);						      \
+    if (__builtin_expect (atomic_compare_and_exchange_val_acq (__futex,       \
+								1, 0), 0))    \
+      {									      \
+	if (__builtin_constant_p (private) && (private) == LLL_PRIVATE)	      \
+	  __lll_lock_wait_private (__futex);				      \
+	else								      \
+	  __lll_lock_wait (__futex, private);				      \
+      }									      \
+  }))
 #define lll_lock(futex, private) __lll_lock (&(futex), private)
 
 
-static inline int __attribute__ ((always_inline))
-__lll_robust_lock (int *futex, int id, int private)
-{
-  int result = 0;
-  if (atomic_compare_and_exchange_bool_acq (futex, id, 0) != 0)
-    result = __lll_robust_lock_wait (futex, private);
-  return result;
-}
+#define __lll_robust_lock(futex, id, private)				      \
+  ({									      \
+    int *__futex = (futex);						      \
+    int __val = 0;							      \
+									      \
+    if (__builtin_expect (atomic_compare_and_exchange_bool_acq (__futex, id,  \
+								0), 0))	      \
+      __val = __lll_robust_lock_wait (__futex, private);		      \
+    __val;								      \
+  })
 #define lll_robust_lock(futex, id, private) \
   __lll_robust_lock (&(futex), id, private)
 
 
-static inline void __attribute__ ((always_inline))
-__lll_cond_lock (int *futex, int private)
-{
-  if (atomic_compare_and_exchange_bool_acq (futex, 2, 0) != 0)
-    __lll_lock_wait (futex, private);
-}
+#define __lll_cond_lock(futex, private)					      \
+  ((void) ({								      \
+    int *__futex = (futex);						      \
+    if (__builtin_expect (atomic_exchange_acq (__futex, 2), 0))		      \
+      __lll_lock_wait (__futex, private);				      \
+  }))
 #define lll_cond_lock(futex, private) __lll_cond_lock (&(futex), private)
 
 
@@ -242,27 +232,29 @@
 extern int __lll_robust_timedlock_wait (int *futex, const struct timespec *,
 					int private) attribute_hidden;
 
-static inline int __attribute__ ((always_inline))
-__lll_timedlock (int *futex, const struct timespec *abstime, int private)
-{
-  int result = 0;
-  if (atomic_compare_and_exchange_bool_acq (futex, 1, 0) != 0)
-    result = __lll_timedlock_wait (futex, abstime, private);
-  return result;
-}
+#define __lll_timedlock(futex, abstime, private)			      \
+  ({									      \
+     int *__futex = (futex);						      \
+     int __val = 0;							      \
+									      \
+     if (__builtin_expect (atomic_exchange_acq (__futex, 1), 0))	      \
+       __val = __lll_timedlock_wait (__futex, abstime, private);	      \
+     __val;								      \
+  })
 #define lll_timedlock(futex, abstime, private) \
   __lll_timedlock (&(futex), abstime, private)
 
 
-static inline int __attribute__ ((always_inline))
-__lll_robust_timedlock (int *futex, const struct timespec *abstime,
-			int id, int private)
-{
-  int result = 0;
-  if (atomic_compare_and_exchange_bool_acq (futex, id, 0) != 0)
-    result = __lll_robust_timedlock_wait (futex, abstime, private);
-  return result;
-}
+#define __lll_robust_timedlock(futex, abstime, id, private)		      \
+  ({									      \
+    int *__futex = (futex);						      \
+    int __val = 0;							      \
+									      \
+    if (__builtin_expect (atomic_compare_and_exchange_bool_acq (__futex, id,  \
+								0), 0))	      \
+      __val = __lll_robust_timedlock_wait (__futex, abstime, private);	      \
+    __val;								      \
+  })
 #define lll_robust_timedlock(futex, abstime, id, private) \
   __lll_robust_timedlock (&(futex), abstime, id, private)
 
@@ -282,10 +274,18 @@
 #define lll_islocked(futex) \
   (futex != 0)
 
+
+/* Our internal lock implementation is identical to the binary-compatible
+   mutex implementation. */
+
 /* Initializers for lock.  */
 #define LLL_LOCK_INITIALIZER		(0)
 #define LLL_LOCK_INITIALIZER_LOCKED	(1)
 
+/* The states of a lock are:
+    0  -  untaken
+    1  -  taken by one user
+   >1  -  taken by more users */
 
 /* The kernel notifies a process which uses CLONE_CHILD_CLEARTID via futex
    wakeup when the clone terminates.  The memory location contains the