Message ID | 20201130213753.1921430-1-lamm@linux.ibm.com |
---|---|
State | New |
Headers | show |
Series | [v3] nptl: Fix __futex_clocklock64 return error check [BZ #26964] | expand |
On 30/11/2020 18:37, Lucas A. M. Magalhaes wrote: > The earlier implementation of this, __lll_clocklock, calls lll_clockwait > that doesn't return the futex syscall error codes. It always tries again > if that fails. > > However in the current implementation, when the futex returns EAGAIN, > __futex_clocklock64 will also return EGAIN, even if the futex is taken. > > This patch fixes the EAGAIN issue and also adds a check for EINTR. As > futex syscall can return EINTR if the thread is interrupted by a signal. > In this case I'm assuming the function should continue trying to lock as > there is no mention to about it on POSIX. Also add a test for both > scenarios. LGTM with the change in comment below (there is no need to send a v4). Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org> > --- > nptl/Makefile | 2 +- > nptl/tst-pthread-timedlock-lockloop.c | 138 ++++++++++++++++++++++++++ > sysdeps/nptl/futex-internal.h | 7 +- > 3 files changed, 142 insertions(+), 5 deletions(-) > create mode 100644 nptl/tst-pthread-timedlock-lockloop.c > > diff --git a/nptl/Makefile b/nptl/Makefile > index a48426a396..42e30a4ce1 100644 > --- a/nptl/Makefile > +++ b/nptl/Makefile > @@ -298,7 +298,7 @@ tests = tst-attr2 tst-attr3 tst-default-attr \ > tst-thread-affinity-sched \ > tst-pthread-defaultattr-free \ > tst-pthread-attr-sigmask \ > - > + tst-pthread-timedlock-lockloop > > tests-container = tst-pthread-getattr > > diff --git a/nptl/tst-pthread-timedlock-lockloop.c b/nptl/tst-pthread-timedlock-lockloop.c > new file mode 100644 > index 0000000000..91635b27d4 > --- /dev/null > +++ b/nptl/tst-pthread-timedlock-lockloop.c > @@ -0,0 +1,138 @@ > +/* Make sure pthread_mutex_timedlock doesn't return spurious error codes. > + > + Copyright (C) 2020 Free Software Foundation, Inc. > + This file is part of the GNU C Library. > + > + The GNU C Library is free software; you can redistribute it and/or > + modify it under the terms of the GNU Lesser General Public > + License as published by the Free Software Foundation; either > + version 2.1 of the License, or (at your option) any later version. > + > + The GNU C Library is distributed in the hope that it will be useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + Lesser General Public License for more details. > + > + You should have received a copy of the GNU Lesser General Public > + License along with the GNU C Library; if not, see > + <https://www.gnu.org/licenses/>. */ > + > +#include <errno.h> > +#include <pthread.h> > +#include <signal.h> > +#include <stdio.h> > +#include <stdlib.h> > +#include <support/check.h> > +#include <support/timespec.h> > +#include <support/xsignal.h> > +#include <support/xthread.h> > +#include <support/xtime.h> > +#include <time.h> > +#include <unistd.h> > + > +#define NANO_PER_SEC 1000000000LL > +#define TIMEOUT (NANO_PER_SEC / 1000LL) > +#define NUM_THREADS 50 > +#define RETEST_TIMES 100 > + > +static pthread_mutex_t mutex; > +static int runs; > +static clockid_t clockid; > + > +static void > +signal_handler (int sig_num) > +{ > + TEST_COMPARE (sig_num, SIGUSR1); > +} > + > +/* Call pthread_mutex_timedlock()/pthread_mutex_unlock() repetitively, hoping > + that one of them returns EAGAIN or EINTR unexpectedly. */ > +static void * > +worker_timedlock (void *arg) > +{ > + for (unsigned int run = 0; run < runs; run++) > + { > + struct timespec abs_time = timespec_add (xclock_now (CLOCK_REALTIME), > + make_timespec (0, 1000000)); > + > + int ret = pthread_mutex_timedlock (&mutex, &abs_time); > + > + if (ret == 0) > + xpthread_mutex_unlock (&mutex); > + > + TEST_VERIFY_EXIT (ret == 0 || ret == ETIMEDOUT); > + } > + return NULL; > +} > + > +static void * > +worker_clocklock (void *arg) > +{ > + for (unsigned int run = 0; run < runs; run++) > + { > + struct timespec time = > + timespec_add (xclock_now (clockid), make_timespec (0, 1000000)); > + > + int ret = pthread_mutex_clocklock (&mutex, clockid, &time); > + > + if (ret == 0) > + xpthread_mutex_unlock (&mutex); > + > + TEST_VERIFY_EXIT (ret == 0 || ret == ETIMEDOUT); > + } > + return NULL; > +} > + > +static int > +run_test_set (void *(*worker) (void *)) > +{ > + pthread_t workers[NUM_THREADS]; > + > + /* Run the checks to catch an EAGAIN. */ > + /* As there is no way to ensure the error condition, just run the test many > + times hoping to catch the error. */ I think the comment should be more clear, since we are not really expecting an error here. I suggest: /* Check if default pthread_mutex_{timed,clock}lock with valid arguments returns either 0 or ETIMEDOUT. Since there is no easy way to force the error condition, the test creates multiple threads which in turn issues pthread_mutex_timedlock multiple times. */ > + runs = 100; > + for (int run = 0; run < RETEST_TIMES; run++) > + { > + for (int i = 0; i < NUM_THREADS; i++) > + workers[i] = xpthread_create (NULL, worker, NULL); > + for (int i = 0; i < NUM_THREADS; i++) > + xpthread_join (workers[i]); > + } > + > + /* Run the test to check if we catch an EINTR. */ > + /* As there is no way to ensure the error condition, just run the test many > + times hoping to catch the error. */ /* The idea is similar to previous tests, but we check if pthread_mutex_{timed,clock}lock does not return EINTR. */ > + pthread_t thread; > + runs = 1; > + for (int i = 0; i < RETEST_TIMES * 1000; i++) > + { > + xpthread_mutex_lock (&mutex); > + thread = xpthread_create (NULL, worker, NULL); > + /* Sleep just a little bit to reach the lock on the worker thread. */ > + usleep (10); > + pthread_kill (thread, SIGUSR1); > + xpthread_mutex_unlock (&mutex); > + xpthread_join (thread); > + } > + > + return 0; > +} > + > +static int > +do_test (void) > +{ > + > + xsignal (SIGUSR1, signal_handler); > + > + xpthread_mutex_init (&mutex, NULL); > + > + run_test_set (worker_timedlock); > + clockid = CLOCK_REALTIME; > + run_test_set (worker_clocklock); > + clockid = CLOCK_MONOTONIC; > + run_test_set (worker_clocklock); > + return 0; > +} > + > +#include <support/test-driver.c> > diff --git a/sysdeps/nptl/futex-internal.h b/sysdeps/nptl/futex-internal.h > index e67803952f..428621bf70 100644 > --- a/sysdeps/nptl/futex-internal.h > +++ b/sysdeps/nptl/futex-internal.h > @@ -418,19 +418,18 @@ static __always_inline int > __futex_clocklock64 (int *futex, clockid_t clockid, > const struct __timespec64 *abstime, int private) > { > - int err = 0; > - > if (__glibc_unlikely (atomic_compare_and_exchange_bool_acq (futex, 1, 0))) > { > while (atomic_exchange_acq (futex, 2) != 0) > { > + int err = 0; > err = __futex_abstimed_wait64 ((unsigned int *) futex, 2, clockid, > abstime, private); > if (err == EINVAL || err == ETIMEDOUT || err == EOVERFLOW) > - break; > + return err; > } > } > - return err; > + return 0; > } > > #endif /* futex-internal.h */ >
Adhemerval Zanella via Libc-alpha <libc-alpha@sourceware.org> writes: > On 30/11/2020 18:37, Lucas A. M. Magalhaes wrote: >> The earlier implementation of this, __lll_clocklock, calls lll_clockwait >> that doesn't return the futex syscall error codes. It always tries again >> if that fails. >> >> However in the current implementation, when the futex returns EAGAIN, >> __futex_clocklock64 will also return EGAIN, even if the futex is taken. >> >> This patch fixes the EAGAIN issue and also adds a check for EINTR. As >> futex syscall can return EINTR if the thread is interrupted by a signal. >> In this case I'm assuming the function should continue trying to lock as >> there is no mention to about it on POSIX. Also add a test for both >> scenarios. > > LGTM with the change in comment below (there is no need to send a v4). > > Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org> Changed the comments below and pushed as 61855081017dff30c577855cda882740356b5d98. Thanks!
diff --git a/nptl/Makefile b/nptl/Makefile index a48426a396..42e30a4ce1 100644 --- a/nptl/Makefile +++ b/nptl/Makefile @@ -298,7 +298,7 @@ tests = tst-attr2 tst-attr3 tst-default-attr \ tst-thread-affinity-sched \ tst-pthread-defaultattr-free \ tst-pthread-attr-sigmask \ - + tst-pthread-timedlock-lockloop tests-container = tst-pthread-getattr diff --git a/nptl/tst-pthread-timedlock-lockloop.c b/nptl/tst-pthread-timedlock-lockloop.c new file mode 100644 index 0000000000..91635b27d4 --- /dev/null +++ b/nptl/tst-pthread-timedlock-lockloop.c @@ -0,0 +1,138 @@ +/* Make sure pthread_mutex_timedlock doesn't return spurious error codes. + + Copyright (C) 2020 Free Software Foundation, Inc. + This file is part of the GNU C Library. + + The GNU C Library is free software; you can redistribute it and/or + modify it under the terms of the GNU Lesser General Public + License as published by the Free Software Foundation; either + version 2.1 of the License, or (at your option) any later version. + + The GNU C Library is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + Lesser General Public License for more details. + + You should have received a copy of the GNU Lesser General Public + License along with the GNU C Library; if not, see + <https://www.gnu.org/licenses/>. */ + +#include <errno.h> +#include <pthread.h> +#include <signal.h> +#include <stdio.h> +#include <stdlib.h> +#include <support/check.h> +#include <support/timespec.h> +#include <support/xsignal.h> +#include <support/xthread.h> +#include <support/xtime.h> +#include <time.h> +#include <unistd.h> + +#define NANO_PER_SEC 1000000000LL +#define TIMEOUT (NANO_PER_SEC / 1000LL) +#define NUM_THREADS 50 +#define RETEST_TIMES 100 + +static pthread_mutex_t mutex; +static int runs; +static clockid_t clockid; + +static void +signal_handler (int sig_num) +{ + TEST_COMPARE (sig_num, SIGUSR1); +} + +/* Call pthread_mutex_timedlock()/pthread_mutex_unlock() repetitively, hoping + that one of them returns EAGAIN or EINTR unexpectedly. */ +static void * +worker_timedlock (void *arg) +{ + for (unsigned int run = 0; run < runs; run++) + { + struct timespec abs_time = timespec_add (xclock_now (CLOCK_REALTIME), + make_timespec (0, 1000000)); + + int ret = pthread_mutex_timedlock (&mutex, &abs_time); + + if (ret == 0) + xpthread_mutex_unlock (&mutex); + + TEST_VERIFY_EXIT (ret == 0 || ret == ETIMEDOUT); + } + return NULL; +} + +static void * +worker_clocklock (void *arg) +{ + for (unsigned int run = 0; run < runs; run++) + { + struct timespec time = + timespec_add (xclock_now (clockid), make_timespec (0, 1000000)); + + int ret = pthread_mutex_clocklock (&mutex, clockid, &time); + + if (ret == 0) + xpthread_mutex_unlock (&mutex); + + TEST_VERIFY_EXIT (ret == 0 || ret == ETIMEDOUT); + } + return NULL; +} + +static int +run_test_set (void *(*worker) (void *)) +{ + pthread_t workers[NUM_THREADS]; + + /* Run the checks to catch an EAGAIN. */ + /* As there is no way to ensure the error condition, just run the test many + times hoping to catch the error. */ + runs = 100; + for (int run = 0; run < RETEST_TIMES; run++) + { + for (int i = 0; i < NUM_THREADS; i++) + workers[i] = xpthread_create (NULL, worker, NULL); + for (int i = 0; i < NUM_THREADS; i++) + xpthread_join (workers[i]); + } + + /* Run the test to check if we catch an EINTR. */ + /* As there is no way to ensure the error condition, just run the test many + times hoping to catch the error. */ + pthread_t thread; + runs = 1; + for (int i = 0; i < RETEST_TIMES * 1000; i++) + { + xpthread_mutex_lock (&mutex); + thread = xpthread_create (NULL, worker, NULL); + /* Sleep just a little bit to reach the lock on the worker thread. */ + usleep (10); + pthread_kill (thread, SIGUSR1); + xpthread_mutex_unlock (&mutex); + xpthread_join (thread); + } + + return 0; +} + +static int +do_test (void) +{ + + xsignal (SIGUSR1, signal_handler); + + xpthread_mutex_init (&mutex, NULL); + + run_test_set (worker_timedlock); + clockid = CLOCK_REALTIME; + run_test_set (worker_clocklock); + clockid = CLOCK_MONOTONIC; + run_test_set (worker_clocklock); + return 0; +} + +#include <support/test-driver.c> diff --git a/sysdeps/nptl/futex-internal.h b/sysdeps/nptl/futex-internal.h index e67803952f..428621bf70 100644 --- a/sysdeps/nptl/futex-internal.h +++ b/sysdeps/nptl/futex-internal.h @@ -418,19 +418,18 @@ static __always_inline int __futex_clocklock64 (int *futex, clockid_t clockid, const struct __timespec64 *abstime, int private) { - int err = 0; - if (__glibc_unlikely (atomic_compare_and_exchange_bool_acq (futex, 1, 0))) { while (atomic_exchange_acq (futex, 2) != 0) { + int err = 0; err = __futex_abstimed_wait64 ((unsigned int *) futex, 2, clockid, abstime, private); if (err == EINVAL || err == ETIMEDOUT || err == EOVERFLOW) - break; + return err; } } - return err; + return 0; } #endif /* futex-internal.h */