* [RFC][BZ #13690] Always read private before lll_unlock.
[not found] <20131206190159.GA25502@domone.podge>
@ 2013-12-06 19:43 ` Ondřej Bílka
2013-12-06 21:12 ` Mike Frysinger
2013-12-18 20:43 ` Torvald Riegel
0 siblings, 2 replies; 4+ messages in thread
From: Ondřej Bílka @ 2013-12-06 19:43 UTC (permalink / raw)
To: Atsushi Nemoto; +Cc: libc-alpha, libc-ports
On Fri, Dec 06, 2013 at 08:01:59PM +0100, OndÅ™ej BÃlka wrote:
> Hi, a related issue to semaphore races is a race in mutex unlocking.
>
from bugzilla:
>
> On most platforms, lll_unlock() is defined as a macro like this:
> #define lll_unlock(lock, private) \
> ((void) ({ \
> int *__futex = &(lock); \
> int __val = atomic_exchange_rel (__futex, 0); \
> if (__builtin_expect (__val > 1, 0)) \
> lll_futex_wake (__futex, 1, private); \
> }))
>
Which causes this problem that could be avoided by changing macro to
#define lll_unlock(lock, private) \
((void) ({ \
int *__futex = &(lock); \
int __private = private \
I wrote a prelimitary patch for that, most of lll_unlock macros are
duplicates so I added a file include/futex_unlock.h with common
implementation. We should check these for more duplicates and if
other functions need similar wrapper.
Comments?
---
include/futex_unlock.h | 15 +++++++++++++++
nptl/sysdeps/unix/sysv/linux/powerpc/lowlevellock.h | 8 +-------
nptl/sysdeps/unix/sysv/linux/s390/lowlevellock.h | 6 +++++-
nptl/sysdeps/unix/sysv/linux/sh/lowlevellock.h | 8 +++++++-
nptl/sysdeps/unix/sysv/linux/sparc/lowlevellock.h | 6 ++++++
ports/sysdeps/unix/sysv/linux/aarch64/nptl/lowlevellock.h | 11 +----------
ports/sysdeps/unix/sysv/linux/alpha/nptl/lowlevellock.h | 10 +---------
ports/sysdeps/unix/sysv/linux/arm/nptl/lowlevellock.h | 11 +----------
ports/sysdeps/unix/sysv/linux/hppa/nptl/lowlevellock.h | 8 +-------
ports/sysdeps/unix/sysv/linux/ia64/nptl/lowlevellock.h | 12 +-----------
ports/sysdeps/unix/sysv/linux/m68k/nptl/lowlevellock.h | 11 +----------
.../unix/sysv/linux/microblaze/nptl/lowlevellock.h | 10 +---------
ports/sysdeps/unix/sysv/linux/mips/nptl/lowlevellock.h | 11 +----------
ports/sysdeps/unix/sysv/linux/tile/nptl/lowlevellock.h | 11 +----------
14 files changed, 43 insertions(+), 95 deletions(-)
create mode 100644 include/futex_unlock.h
diff --git a/include/futex_unlock.h b/include/futex_unlock.h
new file mode 100644
index 0000000..cd497c2
--- /dev/null
+++ b/include/futex_unlock.h
@@ -0,0 +1,15 @@
+#define lll_unlock(lock, private) \
+ ((void)) ({ \
+ int __private = private; \
+ __lll_unlock (lock, __private); \
+ })
+
+#define lll_unlock(lock, private) \
+ ((void) ({ \
+ int *__futex = &(lock); \
+ int __val = atomic_exchange_rel (__futex, 0); \
+ if (__builtin_expect (__val > 1, 0)) \
+ lll_futex_wake (__futex, 1, private); \
+ }))
+
+
diff --git a/nptl/sysdeps/unix/sysv/linux/powerpc/lowlevellock.h b/nptl/sysdeps/unix/sysv/linux/powerpc/lowlevellock.h
index f33f703..84dea3d 100644
--- a/nptl/sysdeps/unix/sysv/linux/powerpc/lowlevellock.h
+++ b/nptl/sysdeps/unix/sysv/linux/powerpc/lowlevellock.h
@@ -302,13 +302,7 @@ extern int __lll_robust_timedlock_wait
__val; \
})
-#define lll_unlock(lock, private) \
- ((void) ({ \
- int *__futex = &(lock); \
- int __val = atomic_exchange_rel (__futex, 0); \
- if (__builtin_expect (__val > 1, 0)) \
- lll_futex_wake (__futex, 1, private); \
- }))
+#include <futex_unlock.h>
#define lll_robust_unlock(lock, private) \
((void) ({ \
diff --git a/nptl/sysdeps/unix/sysv/linux/s390/lowlevellock.h b/nptl/sysdeps/unix/sysv/linux/s390/lowlevellock.h
index 3dab05e..911ff74 100644
--- a/nptl/sysdeps/unix/sysv/linux/s390/lowlevellock.h
+++ b/nptl/sysdeps/unix/sysv/linux/s390/lowlevellock.h
@@ -299,6 +299,11 @@ __lll_robust_timedlock (int *futex, const struct timespec *abstime,
#define lll_robust_timedlock(futex, abstime, id, private) \
__lll_robust_timedlock (&(futex), abstime, id, private)
+#define lll_unlock(futex, private) \
+ ((void)) ({ \
+ int __private = private; \
+ __lll_unlock (&(futex), __private); \
+ })
#define __lll_unlock(futex, private) \
(void) \
@@ -310,7 +315,6 @@ __lll_robust_timedlock (int *futex, const struct timespec *abstime,
if (__builtin_expect (__oldval > 1, 0)) \
lll_futex_wake (__futexp, 1, private); \
})
-#define lll_unlock(futex, private) __lll_unlock(&(futex), private)
#define __lll_robust_unlock(futex, private) \
diff --git a/nptl/sysdeps/unix/sysv/linux/sh/lowlevellock.h b/nptl/sysdeps/unix/sysv/linux/sh/lowlevellock.h
index 486e02c..e24cdf2 100644
--- a/nptl/sysdeps/unix/sysv/linux/sh/lowlevellock.h
+++ b/nptl/sysdeps/unix/sysv/linux/sh/lowlevellock.h
@@ -284,7 +284,13 @@ extern int __lll_unlock_wake (int *__futex, int private) attribute_hidden;
timeout, private); \
__result; })
-#define lll_unlock(futex, private) \
+#define lll_unlock(lock, private) \
+ ((void)) ({ \
+ int __private = private; \
+ __lll_unlock (lock, __private); \
+ })
+
+#define __lll_unlock(futex, private) \
(void) ({ int __result, *__futex = &(futex); \
__asm __volatile ("\
.align 2\n\
diff --git a/nptl/sysdeps/unix/sysv/linux/sparc/lowlevellock.h b/nptl/sysdeps/unix/sysv/linux/sparc/lowlevellock.h
index 5ee8f6d..e435f4b 100644
--- a/nptl/sysdeps/unix/sysv/linux/sparc/lowlevellock.h
+++ b/nptl/sysdeps/unix/sysv/linux/sparc/lowlevellock.h
@@ -293,6 +293,12 @@ __lll_robust_timedlock (int *futex, const struct timespec *abstime,
__lll_robust_timedlock (&(futex), abstime, id, private)
#define lll_unlock(lock, private) \
+ ((void)) ({ \
+ int __private = private; \
+ __lll_unlock (lock, __private); \
+ })
+
+#define __lll_unlock(lock, private) \
((void) ({ \
int *__futex = &(lock); \
int __val = atomic_exchange_24_rel (__futex, 0); \
diff --git a/ports/sysdeps/unix/sysv/linux/aarch64/nptl/lowlevellock.h b/ports/sysdeps/unix/sysv/linux/aarch64/nptl/lowlevellock.h
index 52f8a7a..0256f81 100644
--- a/ports/sysdeps/unix/sysv/linux/aarch64/nptl/lowlevellock.h
+++ b/ports/sysdeps/unix/sysv/linux/aarch64/nptl/lowlevellock.h
@@ -232,16 +232,7 @@ extern int __lll_robust_timedlock_wait (int *futex, const struct timespec *,
__lll_robust_timedlock (&(futex), abstime, id, private)
-#define __lll_unlock(futex, private) \
- (void) \
- ({ int *__futex = (futex); \
- int __oldval = atomic_exchange_rel (__futex, 0); \
- if (__builtin_expect (__oldval > 1, 0)) \
- lll_futex_wake (__futex, 1, private); \
- })
-
-#define lll_unlock(futex, private) __lll_unlock(&(futex), private)
-
+#include <futex_unlock.h>
#define __lll_robust_unlock(futex, private) \
(void) \
diff --git a/ports/sysdeps/unix/sysv/linux/alpha/nptl/lowlevellock.h b/ports/sysdeps/unix/sysv/linux/alpha/nptl/lowlevellock.h
index 567f8ab..a1f1143 100644
--- a/ports/sysdeps/unix/sysv/linux/alpha/nptl/lowlevellock.h
+++ b/ports/sysdeps/unix/sysv/linux/alpha/nptl/lowlevellock.h
@@ -268,15 +268,7 @@ __lll_robust_timedlock (int *futex, const struct timespec *abstime,
__lll_robust_timedlock (&(futex), abstime, id, private)
-#define __lll_unlock(futex, private) \
- (void) \
- ({ int *__futex = (futex); \
- int __oldval = atomic_exchange_rel (__futex, 0); \
- if (__builtin_expect (__oldval > 1, 0)) \
- lll_futex_wake (__futex, 1, private); \
- })
-#define lll_unlock(futex, private) __lll_unlock(&(futex), private)
-
+#include <futex_unlock.h>
#define __lll_robust_unlock(futex, private) \
(void) \
diff --git a/ports/sysdeps/unix/sysv/linux/arm/nptl/lowlevellock.h b/ports/sysdeps/unix/sysv/linux/arm/nptl/lowlevellock.h
index a29593a..c9b3335 100644
--- a/ports/sysdeps/unix/sysv/linux/arm/nptl/lowlevellock.h
+++ b/ports/sysdeps/unix/sysv/linux/arm/nptl/lowlevellock.h
@@ -258,16 +258,7 @@ extern int __lll_robust_timedlock_wait (int *futex, const struct timespec *,
#define lll_robust_timedlock(futex, abstime, id, private) \
__lll_robust_timedlock (&(futex), abstime, id, private)
-
-#define __lll_unlock(futex, private) \
- (void) \
- ({ int *__futex = (futex); \
- int __oldval = atomic_exchange_rel (__futex, 0); \
- if (__builtin_expect (__oldval > 1, 0)) \
- lll_futex_wake (__futex, 1, private); \
- })
-#define lll_unlock(futex, private) __lll_unlock(&(futex), private)
-
+#include <futex_unlock.h>
#define __lll_robust_unlock(futex, private) \
(void) \
diff --git a/ports/sysdeps/unix/sysv/linux/hppa/nptl/lowlevellock.h b/ports/sysdeps/unix/sysv/linux/hppa/nptl/lowlevellock.h
index 4cf8468..d76ef90 100644
--- a/ports/sysdeps/unix/sysv/linux/hppa/nptl/lowlevellock.h
+++ b/ports/sysdeps/unix/sysv/linux/hppa/nptl/lowlevellock.h
@@ -288,13 +288,7 @@ __lll_robust_timedlock (int *futex, const struct timespec *abstime,
#define lll_robust_timedlock(futex, abstime, id, private) \
__lll_robust_timedlock (&(futex), abstime, id, private)
-#define __lll_unlock(futex, private) \
- (void) \
- ({ int val = atomic_exchange_rel (futex, 0); \
- if (__builtin_expect (val > 1, 0)) \
- lll_futex_wake (futex, 1, private); \
- })
-#define lll_unlock(futex, private) __lll_unlock(&(futex), private)
+#include <futex_unlock.h>
#define __lll_robust_unlock(futex,private) \
(void) \
diff --git a/ports/sysdeps/unix/sysv/linux/ia64/nptl/lowlevellock.h b/ports/sysdeps/unix/sysv/linux/ia64/nptl/lowlevellock.h
index cd36f95..3140447 100644
--- a/ports/sysdeps/unix/sysv/linux/ia64/nptl/lowlevellock.h
+++ b/ports/sysdeps/unix/sysv/linux/ia64/nptl/lowlevellock.h
@@ -236,17 +236,7 @@ extern int __lll_robust_timedlock_wait (int *futex, const struct timespec *,
#define lll_robust_timedlock(futex, abstime, id, private) \
__lll_robust_timedlock (&(futex), abstime, id, private)
-
-#define __lll_unlock(futex, private) \
- ((void) ({ \
- int *__futex = (futex); \
- int __val = atomic_exchange_rel (__futex, 0); \
- \
- if (__builtin_expect (__val > 1, 0)) \
- lll_futex_wake (__futex, 1, private); \
- }))
-#define lll_unlock(futex, private) __lll_unlock(&(futex), private)
-
+#include <futex_unlock.h>
#define __lll_robust_unlock(futex, private) \
((void) ({ \
diff --git a/ports/sysdeps/unix/sysv/linux/m68k/nptl/lowlevellock.h b/ports/sysdeps/unix/sysv/linux/m68k/nptl/lowlevellock.h
index 0df6604..5f683c2 100644
--- a/ports/sysdeps/unix/sysv/linux/m68k/nptl/lowlevellock.h
+++ b/ports/sysdeps/unix/sysv/linux/m68k/nptl/lowlevellock.h
@@ -228,16 +228,7 @@ extern int __lll_robust_timedlock_wait (int *futex, const struct timespec *,
#define lll_robust_timedlock(futex, abstime, id, private) \
__lll_robust_timedlock (&(futex), abstime, id, private)
-
-#define __lll_unlock(futex, private) \
- (void) \
- ({ int *__futex = (futex); \
- int __oldval = atomic_exchange_rel (__futex, 0); \
- if (__builtin_expect (__oldval > 1, 0)) \
- lll_futex_wake (__futex, 1, private); \
- })
-#define lll_unlock(futex, private) __lll_unlock(&(futex), private)
-
+#include <futex_unlock.h>
#define __lll_robust_unlock(futex, private) \
(void) \
diff --git a/ports/sysdeps/unix/sysv/linux/microblaze/nptl/lowlevellock.h b/ports/sysdeps/unix/sysv/linux/microblaze/nptl/lowlevellock.h
index 70f5537..a601233 100644
--- a/ports/sysdeps/unix/sysv/linux/microblaze/nptl/lowlevellock.h
+++ b/ports/sysdeps/unix/sysv/linux/microblaze/nptl/lowlevellock.h
@@ -256,15 +256,7 @@ __lll_robust_timedlock (int *futex, const struct timespec *abstime,
#define lll_robust_timedlock(futex, abstime, id, private) \
__lll_robust_timedlock (&(futex), abstime, id, private)
-#define __lll_unlock(futex, private) \
- ((void) ({ \
- int *__futex = (futex); \
- int __val = atomic_exchange_rel (__futex, 0); \
- \
- if (__builtin_expect (__val > 1, 0)) \
- lll_futex_wake (__futex, 1, private); \
- }))
-#define lll_unlock(futex, private) __lll_unlock(&(futex), private)
+#include <futex_unlock.h>
#define __lll_robust_unlock(futex, private) \
((void) ({ \
diff --git a/ports/sysdeps/unix/sysv/linux/mips/nptl/lowlevellock.h b/ports/sysdeps/unix/sysv/linux/mips/nptl/lowlevellock.h
index 208df8d..001f5f4 100644
--- a/ports/sysdeps/unix/sysv/linux/mips/nptl/lowlevellock.h
+++ b/ports/sysdeps/unix/sysv/linux/mips/nptl/lowlevellock.h
@@ -271,16 +271,7 @@ __lll_robust_timedlock (int *futex, const struct timespec *abstime,
__lll_robust_timedlock (&(futex), abstime, id, private)
-#define __lll_unlock(futex, private) \
- ((void) ({ \
- int *__futex = (futex); \
- int __val = atomic_exchange_rel (__futex, 0); \
- \
- if (__builtin_expect (__val > 1, 0)) \
- lll_futex_wake (__futex, 1, private); \
- }))
-#define lll_unlock(futex, private) __lll_unlock(&(futex), private)
-
+#include <futex_unlock.h>
#define __lll_robust_unlock(futex, private) \
((void) ({ \
diff --git a/ports/sysdeps/unix/sysv/linux/tile/nptl/lowlevellock.h b/ports/sysdeps/unix/sysv/linux/tile/nptl/lowlevellock.h
index a9822ec..a8844f3 100644
--- a/ports/sysdeps/unix/sysv/linux/tile/nptl/lowlevellock.h
+++ b/ports/sysdeps/unix/sysv/linux/tile/nptl/lowlevellock.h
@@ -266,16 +266,7 @@ __lll_robust_timedlock (int *futex, const struct timespec *abstime,
#define lll_robust_timedlock(futex, abstime, id, private) \
__lll_robust_timedlock (&(futex), abstime, id, private)
-
-#define __lll_unlock(futex, private) \
- (void) \
- ({ int *__futex = (futex); \
- int __oldval = atomic_exchange_rel (__futex, 0); \
- if (__builtin_expect (__oldval > 1, 0)) \
- lll_futex_wake (__futex, 1, private); \
- })
-#define lll_unlock(futex, private) __lll_unlock(&(futex), private)
-
+#include <futex_unlock.h>
#define __lll_robust_unlock(futex, private) \
(void) \
--
1.8.4.rc3
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [RFC][BZ #13690] Always read private before lll_unlock.
2013-12-06 19:43 ` [RFC][BZ #13690] Always read private before lll_unlock Ondřej Bílka
@ 2013-12-06 21:12 ` Mike Frysinger
2013-12-06 21:52 ` Ondřej Bílka
2013-12-18 20:43 ` Torvald Riegel
1 sibling, 1 reply; 4+ messages in thread
From: Mike Frysinger @ 2013-12-06 21:12 UTC (permalink / raw)
To: libc-ports; +Cc: Ondřej Bílka, Atsushi Nemoto, libc-alpha
[-- Attachment #1: Type: Text/Plain, Size: 877 bytes --]
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
> @@ -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 ?
> + ((void) ({ \
> + int *__futex = &(lock); \
> + int __val = atomic_exchange_rel (__futex, 0); \
> + if (__builtin_expect (__val > 1, 0)) \
> + lll_futex_wake (__futex, 1, private); \
> + }))
> +
> +
no trailing newlines please
-mike
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [RFC][BZ #13690] Always read private before lll_unlock.
2013-12-06 21:12 ` Mike Frysinger
@ 2013-12-06 21:52 ` Ondřej Bílka
0 siblings, 0 replies; 4+ messages in thread
From: Ondřej Bílka @ 2013-12-06 21:52 UTC (permalink / raw)
To: Mike Frysinger; +Cc: libc-ports, Atsushi Nemoto, libc-alpha
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [RFC][BZ #13690] Always read private before lll_unlock.
2013-12-06 19:43 ` [RFC][BZ #13690] Always read private before lll_unlock Ondřej Bílka
2013-12-06 21:12 ` Mike Frysinger
@ 2013-12-18 20:43 ` Torvald Riegel
1 sibling, 0 replies; 4+ messages in thread
From: Torvald Riegel @ 2013-12-18 20:43 UTC (permalink / raw)
To: Ondřej Bílka; +Cc: Atsushi Nemoto, libc-alpha, libc-ports
On Fri, 2013-12-06 at 20:43 +0100, OndÅ™ej BÃlka wrote:
> On Fri, Dec 06, 2013 at 08:01:59PM +0100, OndÅ™ej BÃlka wrote:
> > Hi, a related issue to semaphore races is a race in mutex unlocking.
> >
> from bugzilla:
> >
> > On most platforms, lll_unlock() is defined as a macro like this:
> > #define lll_unlock(lock, private) \
> > ((void) ({ \
> > int *__futex = &(lock); \
> > int __val = atomic_exchange_rel (__futex, 0); \
> > if (__builtin_expect (__val > 1, 0)) \
> > lll_futex_wake (__futex, 1, private); \
> > }))
> >
> Which causes this problem that could be avoided by changing macro to
>
> #define lll_unlock(lock, private) \
> ((void) ({ \
> int *__futex = &(lock); \
> int __private = private \
>
> I wrote a prelimitary patch for that, most of lll_unlock macros are
> duplicates so I added a file include/futex_unlock.h with common
> implementation. We should check these for more duplicates and if
> other functions need similar wrapper.
>
> Comments?
This needs clarification in POSIX; see my comment on #13690
(https://sourceware.org/bugzilla/show_bug.cgi?id=13690#c24). Depending
on how the Austin Group decides, this is either not a bug, or we have to
fix the pending load AND investigate whether the pending futex_wake call
is harmless. The latter might be the case for normal mutexes, but I
wouldn't be surprised to find out that PI or robust mutexes aren't as
simple and need a more complex fix.
Therefore, I think we should wait for the POSIX clarification and then
decide what the next steps should be.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2013-12-18 20:43 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <20131206190159.GA25502@domone.podge>
2013-12-06 19:43 ` [RFC][BZ #13690] Always read private before lll_unlock Ondřej Bílka
2013-12-06 21:12 ` Mike Frysinger
2013-12-06 21:52 ` Ondřej Bílka
2013-12-18 20:43 ` Torvald Riegel
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).