[PATCH v2 0/5] selftests/futex: fix the issue of abnormal test results caused by thread timing
On the Android arm32 platform, when performing the futex_requeue test, it will most likely return a failure. The specific reason is detailed in a commit[1] previously submitted by Edward Liaw. However, this commit cannot perfectly solve the problem. This is because using a barrier does not guarantee that the child thread will wait on futex_wait.
This series of patches attempts to solve this problem by checking whether the child thread is in a sleeping state. This is because when the child thread goes to sleep, it indicates that it is waiting for the futex lock.
v1->v2: - Solve the compilation problems found by the kernel test robot - Cleanup the atomic library code for futex test
Link: https://lore.kernel.org/all/20240918231102.234253-1-edliaw@google.com/
In the requeue_single function, the variable "waits" only uses one element. There is no need to use an array.
Signed-off-by: Yuwen Chen ywen.chen@foxmail.com --- tools/testing/selftests/futex/functional/futex_requeue.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tools/testing/selftests/futex/functional/futex_requeue.c b/tools/testing/selftests/futex/functional/futex_requeue.c index 69e2555b60399..1807465de2144 100644 --- a/tools/testing/selftests/futex/functional/futex_requeue.c +++ b/tools/testing/selftests/futex/functional/futex_requeue.c @@ -33,7 +33,7 @@ TEST(requeue_single) { volatile futex_t _f1 = 0; volatile futex_t f2 = 0; - pthread_t waiter[10]; + pthread_t waiter; int res;
f1 = &_f1; @@ -41,7 +41,7 @@ TEST(requeue_single) /* * Requeue a waiter from f1 to f2, and wake f2. */ - if (pthread_create(&waiter[0], NULL, waiterfn, NULL)) + if (pthread_create(&waiter, NULL, waiterfn, NULL)) ksft_exit_fail_msg("pthread_create failed\n");
usleep(WAKE_WAIT_US);
When creating a thread using pthread_create, you should use pthread_join to free its resources.
Signed-off-by: Yuwen Chen ywen.chen@foxmail.com --- tools/testing/selftests/futex/functional/futex_requeue.c | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/tools/testing/selftests/futex/functional/futex_requeue.c b/tools/testing/selftests/futex/functional/futex_requeue.c index 1807465de2144..7a22458c7fc96 100644 --- a/tools/testing/selftests/futex/functional/futex_requeue.c +++ b/tools/testing/selftests/futex/functional/futex_requeue.c @@ -62,6 +62,8 @@ TEST(requeue_single) } else { ksft_test_result_pass("futex_requeue simple succeeds\n"); } + + pthread_join(waiter, NULL); }
TEST(requeue_multiple) @@ -101,6 +103,9 @@ TEST(requeue_multiple) } else { ksft_test_result_pass("futex_requeue many succeeds\n"); } + + for (i = 0; i < 10; i++) + pthread_join(waiter[i], NULL); }
TEST_HARNESS_MAIN
Directly reading the internal value of atomic_t externally does not meet the encapsulation requirements. A new function atomic_read is added to obtain the value of an atomic variable.
Signed-off-by: Yuwen Chen ywen.chen@foxmail.com --- .../selftests/futex/functional/futex_requeue_pi.c | 12 ++++++------ .../functional/futex_requeue_pi_signal_restart.c | 4 ++-- tools/testing/selftests/futex/include/atomic.h | 12 ++++++++++++ 3 files changed, 20 insertions(+), 8 deletions(-)
diff --git a/tools/testing/selftests/futex/functional/futex_requeue_pi.c b/tools/testing/selftests/futex/functional/futex_requeue_pi.c index f299d75848cd4..5d845b052891b 100644 --- a/tools/testing/selftests/futex/functional/futex_requeue_pi.c +++ b/tools/testing/selftests/futex/functional/futex_requeue_pi.c @@ -201,7 +201,7 @@ void *broadcast_wakerfn(void *arg) int i = 0;
ksft_print_dbg_msg("Waker: waiting for waiters to block\n"); - while (waiters_blocked.val < THREAD_MAX) + while (atomic_read(&waiters_blocked) < THREAD_MAX) usleep(1000); usleep(1000);
@@ -218,7 +218,7 @@ void *broadcast_wakerfn(void *arg) ksft_exit_fail_msg("FUTEX_CMP_REQUEUE_PI failed\n"); } else if (++i < MAX_WAKE_ITERS) { task_count += args->ret; - if (task_count < THREAD_MAX - waiters_woken.val) + if (task_count < THREAD_MAX - atomic_read(&waiters_woken)) goto continue_requeue; } else { ksft_exit_fail_msg("max broadcast iterations (%d) reached with %d/%d tasks woken or requeued\n", @@ -247,13 +247,13 @@ void *signal_wakerfn(void *arg) int i = 0;
ksft_print_dbg_msg("Waker: waiting for waiters to block\n"); - while (waiters_blocked.val < THREAD_MAX) + while (atomic_read(&waiters_blocked) < THREAD_MAX) usleep(1000); usleep(1000);
- while (task_count < THREAD_MAX && waiters_woken.val < THREAD_MAX) { + while (task_count < THREAD_MAX && atomic_read(&waiters_woken) < THREAD_MAX) { ksft_print_dbg_msg("task_count: %d, waiters_woken: %d\n", - task_count, waiters_woken.val); + task_count, atomic_read(&waiters_woken)); if (args->lock) { ksft_print_dbg_msg("Calling FUTEX_LOCK_PI on mutex=%x @ %p\n", f2, &f2); @@ -293,7 +293,7 @@ void *signal_wakerfn(void *arg) args->ret = task_count;
ksft_print_dbg_msg("Waker: exiting with %d\n", args->ret); - ksft_print_dbg_msg("Waker: waiters_woken: %d\n", waiters_woken.val); + ksft_print_dbg_msg("Waker: waiters_woken: %d\n", atomic_read(&waiters_woken)); pthread_exit((void *)&args->ret); }
diff --git a/tools/testing/selftests/futex/functional/futex_requeue_pi_signal_restart.c b/tools/testing/selftests/futex/functional/futex_requeue_pi_signal_restart.c index e34ee0f9ebccd..29f93916b46da 100644 --- a/tools/testing/selftests/futex/functional/futex_requeue_pi_signal_restart.c +++ b/tools/testing/selftests/futex/functional/futex_requeue_pi_signal_restart.c @@ -70,7 +70,7 @@ int create_rt_thread(pthread_t *pth, void*(*func)(void *), void *arg, void handle_signal(int signo) { ksft_print_dbg_msg("signal received %s requeue\n", - requeued.val ? "after" : "prior to"); + atomic_read(&requeued) ? "after" : "prior to"); }
void *waiterfn(void *arg) @@ -83,7 +83,7 @@ void *waiterfn(void *arg) old_val = f1; res = futex_wait_requeue_pi(&f1, old_val, &(f2), NULL, FUTEX_PRIVATE_FLAG); - if (!requeued.val || errno != EWOULDBLOCK) { + if (!atomic_read(&requeued) || errno != EWOULDBLOCK) { ksft_test_result_fail("unexpected return from futex_wait_requeue_pi: %d (%s)\n", res, strerror(errno)); ksft_print_dbg_msg("w2:futex: %x\n", f2); diff --git a/tools/testing/selftests/futex/include/atomic.h b/tools/testing/selftests/futex/include/atomic.h index 428bcd921bb53..c0dccb1b966ba 100644 --- a/tools/testing/selftests/futex/include/atomic.h +++ b/tools/testing/selftests/futex/include/atomic.h @@ -76,4 +76,16 @@ atomic_set(atomic_t *addr, int newval) return newval; }
+/** + * atomic_read() - Atomic read + * @addr: Address of the variable to read + * + * Return the value of addr->val. + */ +static inline int +atomic_read(atomic_t *addr) +{ + return addr->val; +} + #endif
Introduce the header files specifically prepared for test programs from the tools/include directory. And solve the problem that atomic_read and atomic_set may not be safe in a multi - threaded environment.
Signed-off-by: Yuwen Chen ywen.chen@foxmail.com Reported-by: kernel test robot lkp@intel.com Closes: https://lore.kernel.org/oe-kbuild-all/202512112344.gsyPl2ag-lkp@intel.com/ --- .../selftests/futex/functional/Makefile | 2 +- .../testing/selftests/futex/include/atomic.h | 25 +++++++++---------- 2 files changed, 13 insertions(+), 14 deletions(-)
diff --git a/tools/testing/selftests/futex/functional/Makefile b/tools/testing/selftests/futex/functional/Makefile index 490ace1f017e8..8589917a4b126 100644 --- a/tools/testing/selftests/futex/functional/Makefile +++ b/tools/testing/selftests/futex/functional/Makefile @@ -2,7 +2,7 @@ PKG_CONFIG ?= pkg-config LIBNUMA_TEST = $(shell sh -c "$(PKG_CONFIG) numa --atleast-version 2.0.16 > /dev/null 2>&1 && echo SUFFICIENT || echo NO")
-INCLUDES := -I../include -I../../ $(KHDR_INCLUDES) +INCLUDES := -I../include -I../../ $(KHDR_INCLUDES) -I../../../../include CFLAGS := $(CFLAGS) -g -O2 -Wall -pthread -D_FILE_OFFSET_BITS=64 -D_TIME_BITS=64 $(INCLUDES) $(KHDR_INCLUDES) -DLIBNUMA_VER_$(LIBNUMA_TEST)=1 LDLIBS := -lpthread -lrt -lnuma
diff --git a/tools/testing/selftests/futex/include/atomic.h b/tools/testing/selftests/futex/include/atomic.h index c0dccb1b966ba..b23e1a50949a7 100644 --- a/tools/testing/selftests/futex/include/atomic.h +++ b/tools/testing/selftests/futex/include/atomic.h @@ -18,9 +18,8 @@ #ifndef _ATOMIC_H #define _ATOMIC_H
-typedef struct { - volatile int val; -} atomic_t; +#include <linux/types.h> +#include <linux/compiler.h>
#define ATOMIC_INITIALIZER { 0 }
@@ -30,36 +29,36 @@ typedef struct { * @oldval: The expected value of the futex * @newval: The new value to try and assign the futex * - * Return the old value of addr->val. + * Return the old value of addr->counter. */ static inline int atomic_cmpxchg(atomic_t *addr, int oldval, int newval) { - return __sync_val_compare_and_swap(&addr->val, oldval, newval); + return __sync_val_compare_and_swap(&addr->counter, oldval, newval); }
/** * atomic_inc() - Atomic incrememnt * @addr: Address of the variable to increment * - * Return the new value of addr->val. + * Return the new value of addr->counter. */ static inline int atomic_inc(atomic_t *addr) { - return __sync_add_and_fetch(&addr->val, 1); + return __sync_add_and_fetch(&addr->counter, 1); }
/** * atomic_dec() - Atomic decrement * @addr: Address of the variable to decrement * - * Return the new value of addr-val. + * Return the new value of addr-counter. */ static inline int atomic_dec(atomic_t *addr) { - return __sync_sub_and_fetch(&addr->val, 1); + return __sync_sub_and_fetch(&addr->counter, 1); }
/** @@ -67,12 +66,12 @@ atomic_dec(atomic_t *addr) * @addr: Address of the variable to set * @newval: New value for the atomic_t * - * Return the new value of addr->val. + * Return the new value of addr->counter. */ static inline int atomic_set(atomic_t *addr, int newval) { - addr->val = newval; + WRITE_ONCE(addr->counter, newval); return newval; }
@@ -80,12 +79,12 @@ atomic_set(atomic_t *addr, int newval) * atomic_read() - Atomic read * @addr: Address of the variable to read * - * Return the value of addr->val. + * Return the value of addr->counter. */ static inline int atomic_read(atomic_t *addr) { - return addr->val; + return READ_ONCE(addr->counter); }
#endif
Fixes a race between parent and child threads in futex_requeue.
Similar to commit fbf4dec70277 ("selftests/futex: Order calls to futex_lock_pi"), which fixed a flake in futex_lock_pi due to racing between the parent and child threads.
The same issue can occur in the futex_requeue test, because it expects waiterfn to make progress to futex_wait before the parent starts to requeue. This is mitigated by the parent sleeping for WAKE_WAIT_US, but it still fails occasionally. This can be reproduced by adding a sleep in the waiterfn before futex_wait:
TAP version 13 1..2 not ok 1 futex_requeue simple returned: 0 not ok 2 futex_requeue simple returned: 0 not ok 3 futex_requeue many returned: 0 not ok 4 futex_requeue many returned: 0
This issue can be resolved by checking whether the child thread is in a sleeping state. This is because when the child thread goes to sleep, it indicates that it is waiting for the futex lock.
Fixes: 7cb5dd8e2c8c ("selftests: futex: Add futex compare requeue test") Signed-off-by: Yuwen Chen ywen.chen@foxmail.com Co-developed-by: Edward Liaw edliaw@google.com Signed-off-by: Edward Liaw edliaw@google.com --- .../futex/functional/futex_requeue.c | 58 ++++++++++++++++--- 1 file changed, 51 insertions(+), 7 deletions(-)
diff --git a/tools/testing/selftests/futex/functional/futex_requeue.c b/tools/testing/selftests/futex/functional/futex_requeue.c index 7a22458c7fc96..994295fac6972 100644 --- a/tools/testing/selftests/futex/functional/futex_requeue.c +++ b/tools/testing/selftests/futex/functional/futex_requeue.c @@ -7,6 +7,7 @@
#include <pthread.h> #include <limits.h> +#include <linux/compiler.h>
#include "futextest.h" #include "../../kselftest_harness.h" @@ -15,6 +16,7 @@ #define WAKE_WAIT_US 10000
volatile futex_t *f1; +static pthread_barrier_t barrier;
void *waiterfn(void *arg) { @@ -23,28 +25,59 @@ void *waiterfn(void *arg) to.tv_sec = 0; to.tv_nsec = timeout_ns;
+ WRITE_ONCE(*((pid_t *)arg), gettid()); + pthread_barrier_wait(&barrier); + if (futex_wait(f1, *f1, &to, 0)) printf("waiter failed errno %d\n", errno);
return NULL; }
+static int get_thread_state(pid_t pid) +{ + FILE *fp; + char buf[80], tag[80]; + char val = 0; + + snprintf(buf, sizeof(buf), "/proc/%d/status", pid); + fp = fopen(buf, "r"); + if (!fp) + return -1; + + while (fgets(buf, sizeof(buf), fp)) + if (fscanf(fp, "%s %c\n", tag, &val) == 2 && !strcmp(tag, "State:")) + break; + + fclose(fp); + return val; +} + TEST(requeue_single) { volatile futex_t _f1 = 0; volatile futex_t f2 = 0; pthread_t waiter; - int res; + pid_t tids; + int res, state;
f1 = &_f1; + pthread_barrier_init(&barrier, NULL, 2);
/* * Requeue a waiter from f1 to f2, and wake f2. */ - if (pthread_create(&waiter, NULL, waiterfn, NULL)) + if (pthread_create(&waiter, NULL, waiterfn, &tids)) ksft_exit_fail_msg("pthread_create failed\n");
- usleep(WAKE_WAIT_US); + pthread_barrier_wait(&barrier); + pthread_barrier_destroy(&barrier); + while ((state = get_thread_state(READ_ONCE(tids))) != 'S') { + usleep(WAKE_WAIT_US); + + if (state < 0) + break; + }
ksft_print_dbg_msg("Requeuing 1 futex from f1 to f2\n"); res = futex_cmp_requeue(f1, 0, &f2, 0, 1, 0); @@ -71,7 +104,8 @@ TEST(requeue_multiple) volatile futex_t _f1 = 0; volatile futex_t f2 = 0; pthread_t waiter[10]; - int res, i; + pid_t tids[10]; + int res, i, state;
f1 = &_f1;
@@ -80,11 +114,21 @@ TEST(requeue_multiple) * At futex_wake, wake INT_MAX (should be exactly 7). */ for (i = 0; i < 10; i++) { - if (pthread_create(&waiter[i], NULL, waiterfn, NULL)) + pthread_barrier_init(&barrier, NULL, 2); + + if (pthread_create(&waiter[i], NULL, waiterfn, &tids[i])) ksft_exit_fail_msg("pthread_create failed\n"); - }
- usleep(WAKE_WAIT_US); + pthread_barrier_wait(&barrier); + pthread_barrier_destroy(&barrier); + + while ((state = get_thread_state(READ_ONCE(tids[i]))) != 'S') { + usleep(WAKE_WAIT_US); + + if (state < 0) + break; + } + }
ksft_print_dbg_msg("Waking 3 futexes at f1 and requeuing 7 futexes from f1 to f2\n"); res = futex_cmp_requeue(f1, 0, &f2, 3, 7, 0);
linux-kselftest-mirror@lists.linaro.org
-
Yuwen Chen