Skip to content

Commit 042e152

Browse files
codonellcodonell
committed
Fix semaphore destruction (bug 12674).
This commit fixes semaphore destruction by either using 64b atomic operations (where available), or by using two separate fields when only 32b atomic operations are available. In the latter case, we keep a conservative estimate of whether there are any waiting threads in one bit of the field that counts the number of available tokens, thus allowing sem_post to atomically both add a token and determine whether it needs to call futex_wake. See: https://sourceware.org/ml/libc-alpha/2014-12/msg00155.html
1 parent a8db092 commit 042e152

34 files changed

+732
-2103
lines changed

‎ChangeLog

+52
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,55 @@
1+
2015-01-21 Torvald Riegel <[email protected]>
2+
Carlos O'Donell <[email protected]>
3+
4+
[BZ #12674]
5+
* nptl/sem_waitcommon.c: New file.
6+
* nptl/sem_wait.c: Include sem_waitcommon.c.
7+
(__sem_wait_cleanup, do_futex_wait): Remove.
8+
(__new_sem_wait): Adapt.
9+
(__new_sem_trywait): New function.
10+
(__old_sem_trywait): Moved here from nptl/sem_trywait.c.
11+
* nptl/sem_timedwait.c: Include sem_waitcommon.c.
12+
(__sem_wait_cleanup, do_futex_timed_wait): Remove.
13+
(sem_timedwait): Adapt.
14+
* nptl/sem_post.c (__new_sem_post): Adapt.
15+
(futex_wake): New function.
16+
(__old_sem_post): Add release MO fence.
17+
* nptl/sem_open.c (sem_open): Adapt.
18+
* nptl/sem_init.c (__new_sem_init): Adapt.
19+
(futex_private_if_supported): New function.
20+
* nptl/sem_getvalue.c (__new_sem_getvalue): Adapt.
21+
(__old_sem_getvalue): Add using previous code.
22+
* sysdeps/nptl/internaltypes.h: Adapt.
23+
* nptl/tst-sem13.c (do_test): Adapt.
24+
* nptl/tst-sem11.c (main): Adapt.
25+
* nptl/sem_trywait.c: Remove.
26+
* nptl/DESIGN-sem.txt: Remove.
27+
* nptl/Makefile (libpthread-routines): Remove sem_trywait.
28+
(gen-as-const-headers): Remove structsem.sym.
29+
* nptl/structsem.sym: Remove.
30+
* sysdeps/unix/sysv/linux/alpha/sem_post.c: Remove.
31+
* sysdeps/unix/sysv/linux/i386/i486/sem_post.S: Remove.
32+
* sysdeps/unix/sysv/linux/i386/i486/sem_timedwait.S: Remove.
33+
* sysdeps/unix/sysv/linux/i386/i486/sem_trywait.S: Remove.
34+
* sysdeps/unix/sysv/linux/i386/i486/sem_wait.S: Remove.
35+
* sysdeps/unix/sysv/linux/i386/i586/sem_post.S: Remove.
36+
* sysdeps/unix/sysv/linux/i386/i586/sem_timedwait.S: Remove.
37+
* sysdeps/unix/sysv/linux/i386/i586/sem_trywait.S: Remove.
38+
* sysdeps/unix/sysv/linux/i386/i586/sem_wait.S: Remove.
39+
* sysdeps/unix/sysv/linux/i386/i686/sem_post.S: Remove.
40+
* sysdeps/unix/sysv/linux/i386/i686/sem_timedwait.S: Remove.
41+
* sysdeps/unix/sysv/linux/i386/i686/sem_trywait.S: Remove.
42+
* sysdeps/unix/sysv/linux/i386/i686/sem_wait.S: Remove.
43+
* sysdeps/unix/sysv/linux/powerpc/sem_post.c: Remove.
44+
* sysdeps/unix/sysv/linux/sh/sem_post.S: Remove.
45+
* sysdeps/unix/sysv/linux/sh/sem_timedwait.S: Remove.
46+
* sysdeps/unix/sysv/linux/sh/sem_trywait.S: Remove.
47+
* sysdeps/unix/sysv/linux/sh/sem_wait.S: Remove.
48+
* sysdeps/unix/sysv/linux/x86_64/sem_post.S: Remove.
49+
* sysdeps/unix/sysv/linux/x86_64/sem_timedwait.S: Remove.
50+
* sysdeps/unix/sysv/linux/x86_64/sem_trywait.S: Remove.
51+
* sysdeps/unix/sysv/linux/x86_64/sem_wait.S: Remove.
52+
153
2015-01-20 Carlos O'Donell <[email protected]>
254

355
* INSTALL: Regenerated.

‎NEWS

+16-9
Original file line numberDiff line numberDiff line change
@@ -9,15 +9,22 @@ Version 2.21
99

1010
* The following bugs are resolved with this release:
1111

12-
6652, 10672, 12847, 12926, 13862, 14132, 14138, 14171, 14498, 15215,
13-
15884, 16009, 16191, 16469, 16617, 16619, 16657, 16740, 16857, 17192,
14-
17266, 17273, 17344, 17363, 17370, 17371, 17411, 17460, 17475, 17485,
15-
17501, 17506, 17508, 17522, 17555, 17570, 17571, 17572, 17573, 17574,
16-
17582, 17583, 17584, 17585, 17589, 17594, 17601, 17608, 17616, 17625,
17-
17630, 17633, 17634, 17635, 17647, 17653, 17657, 17658, 17664, 17665,
18-
17668, 17682, 17717, 17719, 17722, 17723, 17724, 17725, 17732, 17733,
19-
17744, 17745, 17746, 17747, 17748, 17775, 17777, 17780, 17781, 17782,
20-
17791, 17793, 17796, 17797, 17803, 17806, 17834, 17844, 17848
12+
6652, 10672, 12674, 12847, 12926, 13862, 14132, 14138, 14171, 14498,
13+
15215, 15884, 16009, 16191, 16469, 16617, 16619, 16657, 16740, 16857,
14+
17192, 17266, 17273, 17344, 17363, 17370, 17371, 17411, 17460, 17475,
15+
17485, 17501, 17506, 17508, 17522, 17555, 17570, 17571, 17572, 17573,
16+
17574, 17582, 17583, 17584, 17585, 17589, 17594, 17601, 17608, 17616,
17+
17625, 17630, 17633, 17634, 17635, 17647, 17653, 17657, 17658, 17664,
18+
17665, 17668, 17682, 17717, 17719, 17722, 17723, 17724, 17725, 17732,
19+
17733, 17744, 17745, 17746, 17747, 17748, 17775, 17777, 17780, 17781,
20+
17782, 17791, 17793, 17796, 17797, 17803, 17806, 17834, 17844, 17848
21+
22+
* A new semaphore algorithm has been implemented in generic C code for all
23+
machines. Previous custom assembly implementations of semaphore were
24+
difficult to reason about or ensure that they were safe. The new version
25+
of semaphore supports machines with 64-bit or 32-bit atomic operations.
26+
The new semaphore algorithm is used by sem_init, sem_open, sem_post,
27+
sem_wait, sem_timedwait, sem_trywait, and sem_getvalue.
2128

2229
* Port to Altera Nios II has been contributed by Mentor Graphics.
2330

‎nptl/DESIGN-sem.txt

-46
This file was deleted.

‎nptl/Makefile

+2-3
Original file line numberDiff line numberDiff line change
@@ -100,7 +100,7 @@ libpthread-routines = nptl-init vars events version \
100100
sem_init sem_destroy \
101101
sem_open sem_close sem_unlink \
102102
sem_getvalue \
103-
sem_wait sem_trywait sem_timedwait sem_post \
103+
sem_wait sem_timedwait sem_post \
104104
cleanup cleanup_defer cleanup_compat \
105105
cleanup_defer_compat unwind \
106106
pt-longjmp pt-cleanup\
@@ -283,8 +283,7 @@ tests-nolibpthread = tst-unload
283283
gen-as-const-headers = pthread-errnos.sym \
284284
lowlevelcond.sym lowlevelrwlock.sym \
285285
lowlevelbarrier.sym unwindbuf.sym \
286-
lowlevelrobustlock.sym pthread-pi-defines.sym \
287-
structsem.sym
286+
lowlevelrobustlock.sym pthread-pi-defines.sym
288287

289288

290289
LDFLAGS-pthread.so = -Wl,--enable-new-dtags,-z,nodelete,-z,initfirst

‎nptl/sem_getvalue.c

+20-6
Original file line numberDiff line numberDiff line change
@@ -19,23 +19,37 @@
1919
#include <semaphore.h>
2020
#include <shlib-compat.h>
2121
#include "semaphoreP.h"
22+
#include <atomic.h>
2223

2324

2425
int
25-
__new_sem_getvalue (sem, sval)
26-
sem_t *sem;
27-
int *sval;
26+
__new_sem_getvalue (sem_t *sem, int *sval)
2827
{
2928
struct new_sem *isem = (struct new_sem *) sem;
3029

3130
/* XXX Check for valid SEM parameter. */
32-
33-
*sval = isem->value;
31+
/* FIXME This uses relaxed MO, even though POSIX specifies that this function
32+
should be linearizable. However, its debatable whether linearizability
33+
is the right requirement. We need to follow up with POSIX and, if
34+
necessary, use a stronger MO here and elsewhere (e.g., potentially
35+
release MO in all places where we consume a token). */
36+
37+
#if __HAVE_64B_ATOMICS
38+
*sval = atomic_load_relaxed (&isem->data) & SEM_VALUE_MASK;
39+
#else
40+
*sval = atomic_load_relaxed (&isem->value) >> SEM_VALUE_SHIFT;
41+
#endif
3442

3543
return 0;
3644
}
3745
versioned_symbol (libpthread, __new_sem_getvalue, sem_getvalue, GLIBC_2_1);
3846
#if SHLIB_COMPAT(libpthread, GLIBC_2_0, GLIBC_2_1)
39-
strong_alias (__new_sem_getvalue, __old_sem_getvalue)
47+
int
48+
__old_sem_getvalue (sem_t *sem, int *sval)
49+
{
50+
struct old_sem *isem = (struct old_sem *) sem;
51+
*sval = isem->value;
52+
return 0;
53+
}
4054
compat_symbol (libpthread, __old_sem_getvalue, sem_getvalue, GLIBC_2_0);
4155
#endif

‎nptl/sem_init.c

+23-12
Original file line numberDiff line numberDiff line change
@@ -18,17 +18,29 @@
1818

1919
#include <errno.h>
2020
#include <semaphore.h>
21-
#include <lowlevellock.h>
2221
#include <shlib-compat.h>
2322
#include "semaphoreP.h"
2423
#include <kernel-features.h>
2524

25+
/* Returns FUTEX_PRIVATE if pshared is zero and private futexes are supported;
26+
returns FUTEX_SHARED otherwise.
27+
TODO Remove when cleaning up the futex API throughout glibc. */
28+
static __always_inline int
29+
futex_private_if_supported (int pshared)
30+
{
31+
if (pshared != 0)
32+
return LLL_SHARED;
33+
#ifdef __ASSUME_PRIVATE_FUTEX
34+
return LLL_PRIVATE;
35+
#else
36+
return THREAD_GETMEM (THREAD_SELF, header.private_futex)
37+
^ FUTEX_PRIVATE_FLAG;
38+
#endif
39+
}
40+
2641

2742
int
28-
__new_sem_init (sem, pshared, value)
29-
sem_t *sem;
30-
int pshared;
31-
unsigned int value;
43+
__new_sem_init (sem_t *sem, int pshared, unsigned int value)
3244
{
3345
/* Parameter sanity check. */
3446
if (__glibc_unlikely (value > SEM_VALUE_MAX))
@@ -40,16 +52,15 @@ __new_sem_init (sem, pshared, value)
4052
/* Map to the internal type. */
4153
struct new_sem *isem = (struct new_sem *) sem;
4254

43-
/* Use the values the user provided. */
44-
isem->value = value;
45-
#ifdef __ASSUME_PRIVATE_FUTEX
46-
isem->private = pshared ? 0 : FUTEX_PRIVATE_FLAG;
55+
/* Use the values the caller provided. */
56+
#if __HAVE_64B_ATOMICS
57+
isem->data = value;
4758
#else
48-
isem->private = pshared ? 0 : THREAD_GETMEM (THREAD_SELF,
49-
header.private_futex);
59+
isem->value = value << SEM_VALUE_SHIFT;
60+
isem->nwaiters = 0;
5061
#endif
5162

52-
isem->nwaiters = 0;
63+
isem->private = futex_private_if_supported (pshared);
5364

5465
return 0;
5566
}

‎nptl/sem_open.c

+7-2
Original file line numberDiff line numberDiff line change
@@ -193,9 +193,14 @@ sem_open (const char *name, int oflag, ...)
193193
struct new_sem newsem;
194194
} sem;
195195

196-
sem.newsem.value = value;
197-
sem.newsem.private = 0;
196+
#if __HAVE_64B_ATOMICS
197+
sem.newsem.data = value;
198+
#else
199+
sem.newsem.value = value << SEM_VALUE_SHIFT;
198200
sem.newsem.nwaiters = 0;
201+
#endif
202+
/* This always is a shared semaphore. */
203+
sem.newsem.private = LLL_SHARED;
199204

200205
/* Initialize the remaining bytes as well. */
201206
memset ((char *) &sem.initsem + sizeof (struct new_sem), '\0',

‎nptl/sem_post.c

+57-10
Original file line numberDiff line numberDiff line change
@@ -26,34 +26,78 @@
2626

2727
#include <shlib-compat.h>
2828

29+
/* Wrapper for lll_futex_wake, with error checking.
30+
TODO Remove when cleaning up the futex API throughout glibc. */
31+
static __always_inline void
32+
futex_wake (unsigned int* futex, int processes_to_wake, int private)
33+
{
34+
int res = lll_futex_wake (futex, processes_to_wake, private);
35+
/* No error. Ignore the number of woken processes. */
36+
if (res >= 0)
37+
return;
38+
switch (res)
39+
{
40+
case -EFAULT: /* Could have happened due to memory reuse. */
41+
case -EINVAL: /* Could be either due to incorrect alignment (a bug in
42+
glibc or in the application) or due to memory being
43+
reused for a PI futex. We cannot distinguish between the
44+
two causes, and one of them is correct use, so we do not
45+
act in this case. */
46+
return;
47+
case -ENOSYS: /* Must have been caused by a glibc bug. */
48+
/* No other errors are documented at this time. */
49+
default:
50+
abort ();
51+
}
52+
}
53+
54+
55+
/* See sem_wait for an explanation of the algorithm. */
2956
int
3057
__new_sem_post (sem_t *sem)
3158
{
3259
struct new_sem *isem = (struct new_sem *) sem;
60+
int private = isem->private;
3361

34-
__typeof (isem->value) cur;
62+
#if __HAVE_64B_ATOMICS
63+
/* Add a token to the semaphore. We use release MO to make sure that a
64+
thread acquiring this token synchronizes with us and other threads that
65+
added tokens before (the release sequence includes atomic RMW operations
66+
by other threads). */
67+
/* TODO Use atomic_fetch_add to make it scale better than a CAS loop? */
68+
unsigned long int d = atomic_load_relaxed (&isem->data);
3569
do
3670
{
37-
cur = isem->value;
38-
if (isem->value == SEM_VALUE_MAX)
71+
if ((d & SEM_VALUE_MASK) == SEM_VALUE_MAX)
3972
{
4073
__set_errno (EOVERFLOW);
4174
return -1;
4275
}
4376
}
44-
while (atomic_compare_and_exchange_bool_rel (&isem->value, cur + 1, cur));
77+
while (!atomic_compare_exchange_weak_release (&isem->data, &d, d + 1));
4578

46-
atomic_full_barrier ();
47-
if (isem->nwaiters > 0)
79+
/* If there is any potentially blocked waiter, wake one of them. */
80+
if ((d >> SEM_NWAITERS_SHIFT) > 0)
81+
futex_wake (((unsigned int *) &isem->data) + SEM_VALUE_OFFSET, 1, private);
82+
#else
83+
/* Add a token to the semaphore. Similar to 64b version. */
84+
unsigned int v = atomic_load_relaxed (&isem->value);
85+
do
4886
{
49-
int err = lll_futex_wake (&isem->value, 1,
50-
isem->private ^ FUTEX_PRIVATE_FLAG);
51-
if (__builtin_expect (err, 0) < 0)
87+
if ((v << SEM_VALUE_SHIFT) == SEM_VALUE_MAX)
5288
{
53-
__set_errno (-err);
89+
__set_errno (EOVERFLOW);
5490
return -1;
5591
}
5692
}
93+
while (!atomic_compare_exchange_weak_release (&isem->value,
94+
&v, v + (1 << SEM_VALUE_SHIFT)));
95+
96+
/* If there is any potentially blocked waiter, wake one of them. */
97+
if ((v & SEM_NWAITERS_MASK) != 0)
98+
futex_wake (&isem->value, 1, private);
99+
#endif
100+
57101
return 0;
58102
}
59103
versioned_symbol (libpthread, __new_sem_post, sem_post, GLIBC_2_1);
@@ -66,6 +110,9 @@ __old_sem_post (sem_t *sem)
66110
{
67111
int *futex = (int *) sem;
68112

113+
/* We must need to synchronize with consumers of this token, so the atomic
114+
increment must have release MO semantics. */
115+
atomic_write_barrier ();
69116
(void) atomic_increment_val (futex);
70117
/* We always have to assume it is a shared semaphore. */
71118
int err = lll_futex_wake (futex, 1, LLL_SHARED);

0 commit comments

Comments
 (0)