[PATCH v2 0/4] selftests/futex: Fix flaky futex_wait and futex_waitv tests

[PATCH v2 0/4] selftests/futex: Fix flaky futex_wait and futex_waitv tests

[Mathieu Desnoyers]

Hi Thomas, Peter,

The futex_wait and futex_waitv selftests assume that a waiter thread
blocks within 10ms, and that the main thread wakes it in time. Under
load, both assumptions fail. With the built-in locktorture, the tests
failed in 0.5% to 1% of the runs in 8-CPU VMs. Repeated runs of
futex_waitv also run out of System V shared memory segments.

These four patches fix both problems:

 1  Let the futex_waitv() helper take no timeout.
 2  Retry the wake in futex_wait until the waiter has returned, instead
    of sleeping a fixed time.
 3  Do the same in futex_waitv, and join its waiter.
 4  Remove the shared memory segments that the tests create.

How this was developed
======================

An LLM coding assistant wrote these patches and their messages, under
my direction and review.

Changes since v1
================

- v1 was patch 2 alone. It retried the wake for up to 5s, and gave the
  waiter a timeout of 10s. Both bounds were timing assumptions too. In
  v2, the main thread retries until the waiter has returned from
  futex_wait(), and the waiter waits without a timeout.
- New patches 1, 3 and 4.

Based on v7.4-rc1, 6628e7d4c6b2.

v1: https://lore.kernel.org/all/20261108142309.1527337-1-mathieu.desnoyers@efficios.com/

Mathieu Desnoyers (4):
  selftests/futex: Let futex_waitv() take no timeout
  selftests/futex: Retry the wake in futex_wait instead of sleeping
  selftests/futex: Retry the wake in futex_waitv instead of sleeping
  selftests/futex: Remove the shared memory segments of the futex tests

 .../selftests/futex/functional/futex_wait.c   | 59 +++++++++++++------
 .../selftests/futex/functional/futex_waitv.c  | 52 +++++++++++-----
 .../selftests/futex/include/futex2test.h      | 14 +++--
 3 files changed, 86 insertions(+), 39 deletions(-)


base-commit: 6628e7d4c6b25c8d18f1345e3a20d21829aae7e4
-- 
2.47.3

[PATCH v2 1/4] selftests/futex: Let futex_waitv() take no timeout

[Mathieu Desnoyers]

The futex_waitv() helper of futex2test.h copies the caller's timeout
into a struct __kernel_timespec. It dereferences the pointer without a
check, so a caller cannot pass NULL. The syscall takes NULL as no
timeout.

Pass NULL to the syscall when the caller passes NULL. The futex_waitv
tests use this in a later patch, to wait until they are woken.

Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
---
 tools/testing/selftests/futex/include/futex2test.h | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/tools/testing/selftests/futex/include/futex2test.h b/tools/testing/selftests/futex/include/futex2test.h
index 53e88b60ac6d..a4616b6edcff 100644
--- a/tools/testing/selftests/futex/include/futex2test.h
+++ b/tools/testing/selftests/futex/include/futex2test.h
@@ -68,12 +68,16 @@ struct futex32_numa {
 static inline int futex_waitv(volatile struct futex_waitv *waiters, unsigned long nr_waiters,
 			      unsigned long flags, struct timespec *timo, clockid_t clockid)
 {
-		struct __kernel_timespec ts = {
-			.tv_sec = timo->tv_sec,
-			.tv_nsec = timo->tv_nsec,
-		};
+		struct __kernel_timespec ts, *tsp = NULL;
 
-		return syscall(__NR_futex_waitv, waiters, nr_waiters, flags, &ts, clockid);
+		/* No timeout: wait until woken. */
+		if (timo) {
+			ts.tv_sec = timo->tv_sec;
+			ts.tv_nsec = timo->tv_nsec;
+			tsp = &ts;
+		}
+
+		return syscall(__NR_futex_waitv, waiters, nr_waiters, flags, tsp, clockid);
 }
 
 /*
-- 
2.47.3

[PATCH v2 2/4] selftests/futex: Retry the wake in futex_wait instead of sleeping

[Mathieu Desnoyers]

Each futex_wait test starts a waiter thread, which calls futex_wait()
with a 30ms timeout. The main thread sleeps 10ms, then expects
futex_wake() to wake one waiter. So the test assumes that the waiter
reaches futex_wait() within 10ms, and that the main thread runs again
within 30ms.

A busy machine breaks both. The built-in locktorture at times holds a
spinlock for 100ms with preemption off. With it, private_futex failed in
about 0.5% of the runs in 8-CPU VMs, on v7.4-rc1. Timestamps showed two
cases. The waiter started up to 75ms late, after the wake. Or the main
thread woke up to 105ms late, after the waiter had timed out.

Fix this without relying on timing. The waiter sets a flag once
futex_wait() returns. The main thread retries futex_wake() every
millisecond until it sees that flag, and expects one waiter woken in
all. The waiter waits without a timeout. If a wake is lost, the timeout
of the kselftest harness ends the test. Check what pthread_join()
returns.

With this patch, private_futex passed 3000 runs of 3000 in the same VMs,
and the three tests 1000 runs of 1000.

Fixes: c3d128581f64 ("selftests: futex: Add futex wait test")
Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
---
 .../selftests/futex/functional/futex_wait.c   | 57 ++++++++++++-------
 1 file changed, 38 insertions(+), 19 deletions(-)

diff --git a/tools/testing/selftests/futex/functional/futex_wait.c b/tools/testing/selftests/futex/functional/futex_wait.c
index 5292f5d2c3b5..4bf7d8ee238b 100644
--- a/tools/testing/selftests/futex/functional/futex_wait.c
+++ b/tools/testing/selftests/futex/functional/futex_wait.c
@@ -14,11 +14,11 @@
 #include "futextest.h"
 #include "kselftest_harness.h"
 
-#define timeout_ns  30000000
-#define WAKE_WAIT_US 10000
+#define WAKE_RETRY_US 1000
 #define SHM_PATH "futex_shm_file"
 
 void *futex;
+static int waiter_done;
 
 struct waiter_args {
 	struct __test_metadata	*_metadata;
@@ -29,13 +29,10 @@ static void *waiterfn(void *arg)
 {
 	struct waiter_args *args = (struct waiter_args *)arg;
 	struct __test_metadata *_metadata = args->_metadata;
-	struct timespec to;
 	int res;
 
-	to.tv_sec = 0;
-	to.tv_nsec = timeout_ns;
-
-	res = futex_wait(futex, 0, &to, args->flags);
+	res = futex_wait(futex, 0, NULL, args->flags);
+	__atomic_store_n(&waiter_done, 1, __ATOMIC_RELEASE);
 	if (res) {
 		EXPECT_EQ(res, 0)
 			TH_LOG("waiter failed errno %d: %s", errno, strerror(errno));
@@ -45,6 +42,25 @@ static void *waiterfn(void *arg)
 	return NULL;
 }
 
+/*
+ * Wake the waiter of @uaddr until it has returned from futex_wait(). The
+ * waiter may not have reached futex_wait() yet, and a wake then finds no
+ * waiter. Return how many waiters futex_wake() woke, or -1 on error.
+ */
+static int wake_waiter(void *uaddr, unsigned int flags)
+{
+	int res, woken = 0;
+
+	while (!__atomic_load_n(&waiter_done, __ATOMIC_ACQUIRE)) {
+		res = futex_wake(uaddr, 1, flags);
+		if (res < 0)
+			return res;
+		woken += res;
+		usleep(WAKE_RETRY_US);
+	}
+	return woken;
+}
+
 TEST(private_futex)
 {
 	struct waiter_args *args = malloc(sizeof(*args));
@@ -58,17 +74,18 @@ TEST(private_futex)
 
 	/* Testing a private futex */
 	TH_LOG("Calling private futex_wait on futex: %p", futex);
+	waiter_done = 0;
 	ASSERT_EQ(pthread_create(&waiter, NULL, waiterfn, args), 0)
 		TH_LOG("pthread_create failed");
 
-	usleep(WAKE_WAIT_US);
-
 	TH_LOG("Calling private futex_wake on futex: %p", futex);
-	res = futex_wake(futex, 1, FUTEX_PRIVATE_FLAG);
+	res = wake_waiter(futex, FUTEX_PRIVATE_FLAG);
 	EXPECT_EQ(res, 1)
 		TH_LOG("futex_wake private returned: %d %s", res, res < 0 ? strerror(errno) : "");
 
-	pthread_join(waiter, NULL);
+	res = pthread_join(waiter, NULL);
+	EXPECT_EQ(res, 0)
+		TH_LOG("pthread_join failed: %s", strerror(res));
 }
 
 TEST(anon_page)
@@ -103,19 +120,20 @@ TEST(anon_page)
 	futex = shared_data;
 
 	TH_LOG("Calling shared (page anon) futex_wait on futex: %p", futex);
+	waiter_done = 0;
 	ASSERT_EQ(pthread_create(&waiter, NULL, waiterfn, args), 0)
 		TH_LOG("pthread_create failed");
 
-	usleep(WAKE_WAIT_US);
-
 	TH_LOG("Calling shared (page anon) futex_wake on futex: %p", futex);
-	res = futex_wake(futex, 1, 0);
+	res = wake_waiter(futex, 0);
 	EXPECT_EQ(res, 1) {
 		TH_LOG("futex_wake shared (page anon) returned: %d %s",
 		       res, res < 0 ? strerror(errno) : "");
 	}
 
-	pthread_join(waiter, NULL);
+	res = pthread_join(waiter, NULL);
+	EXPECT_EQ(res, 0)
+		TH_LOG("pthread_join failed: %s", strerror(res));
 	shmdt(shared_data);
 }
 
@@ -158,19 +176,20 @@ TEST(file_backed)
 	futex = shm;
 
 	TH_LOG("Calling shared (file backed) futex_wait on futex: %p", futex);
+	waiter_done = 0;
 	ASSERT_EQ(pthread_create(&waiter, NULL, waiterfn, args), 0)
 		TH_LOG("pthread_create failed");
 
-	usleep(WAKE_WAIT_US);
-
 	TH_LOG("Calling shared (file backed) futex_wake on futex: %p", futex);
-	res = futex_wake(shm, 1, 0);
+	res = wake_waiter(shm, 0);
 	EXPECT_EQ(res, 1) {
 		TH_LOG("futex_wake shared (file backed) returned: %d %s",
 		       res, res < 0 ? strerror(errno) : "");
 	}
 
-	pthread_join(waiter, NULL);
+	res = pthread_join(waiter, NULL);
+	EXPECT_EQ(res, 0)
+		TH_LOG("pthread_join failed: %s", strerror(res));
 	munmap(shm, sizeof(f_private));
 	remove(SHM_PATH);
 	close(fd);
-- 
2.47.3

[PATCH v2 3/4] selftests/futex: Retry the wake in futex_waitv instead of sleeping

[Mathieu Desnoyers]

The private_waitv and shared_waitv tests start a waiter thread, which
calls futex_waitv() with a timeout of 1s. The main thread sleeps 10ms,
then expects futex_wake() to wake that waiter. So the test assumes that
the waiter reaches futex_waitv() within 10ms.

A busy machine breaks this. The built-in locktorture at times holds a
spinlock for 100ms with preemption off. With it, the tests of
futex_waitv failed in about 1% of the runs in 8-CPU VMs, on v7.4-rc1.
In each failure, futex_wake() found no waiter in private_waitv or
shared_waitv.

The two tests also do not join the waiter. A test can then end before
the waiter checks what futex_waitv() returned.

Fix this as the previous patch does for futex_wait. The waiter sets a
flag once futex_waitv() returns. The main thread retries futex_wake()
every millisecond until it sees that flag, and expects one waiter woken
in all. The waiter waits without a timeout. If a wake is lost, the
timeout of the kselftest harness ends the test. Join the waiter at the
end of each test, and check that the join succeeds.

With this patch, the tests passed 4000 runs of 4000 in the same VMs.

Fixes: 5e59c1d1c78c ("selftests: futex: Add sys_futex_waitv() test")
Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
---
 .../selftests/futex/functional/futex_waitv.c  | 50 +++++++++++++------
 1 file changed, 35 insertions(+), 15 deletions(-)

diff --git a/tools/testing/selftests/futex/functional/futex_waitv.c b/tools/testing/selftests/futex/functional/futex_waitv.c
index 4858d5faeecf..ccfa31fd20fa 100644
--- a/tools/testing/selftests/futex/functional/futex_waitv.c
+++ b/tools/testing/selftests/futex/functional/futex_waitv.c
@@ -20,25 +20,20 @@
 #include "futex2test.h"
 #include "kselftest_harness.h"
 
-#define WAKE_WAIT_US 10000
+#define WAKE_RETRY_US 1000
 #define NR_FUTEXES 30
 static struct futex_waitv waitv[NR_FUTEXES];
 u_int32_t futexes[NR_FUTEXES] = {0};
+static int waiter_done;
 
 
 void *waiterfn(void *arg)
 {
 	struct __test_metadata *_metadata = (struct __test_metadata *)arg;
-	struct timespec to;
 	int res;
 
-	/* setting absolute timeout for futex2 */
-	ASSERT_EQ(clock_gettime(CLOCK_MONOTONIC, &to), 0)
-		TH_LOG("gettime64 failed");
-
-	to.tv_sec++;
-
-	res = futex_waitv(waitv, NR_FUTEXES, 0, &to, CLOCK_MONOTONIC);
+	res = futex_waitv(waitv, NR_FUTEXES, 0, NULL, CLOCK_MONOTONIC);
+	__atomic_store_n(&waiter_done, 1, __ATOMIC_RELEASE);
 	if (res < 0) {
 		EXPECT_EQ(res, NR_FUTEXES - 1)
 			TH_LOG("futex_waitv failed: %s", strerror(errno));
@@ -50,6 +45,25 @@ void *waiterfn(void *arg)
 	return NULL;
 }
 
+/*
+ * Wake the waiter of @uaddr until it has returned from futex_waitv(). The
+ * waiter may not have reached futex_waitv() yet, and a wake then finds no
+ * waiter. Return how many waiters futex_wake() woke, or -1 on error.
+ */
+static int wake_waiter(void *uaddr, unsigned int flags)
+{
+	int res, woken = 0;
+
+	while (!__atomic_load_n(&waiter_done, __ATOMIC_ACQUIRE)) {
+		res = futex_wake(uaddr, 1, flags);
+		if (res < 0)
+			return res;
+		woken += res;
+		usleep(WAKE_RETRY_US);
+	}
+	return woken;
+}
+
 TEST(private_waitv)
 {
 	pthread_t waiter;
@@ -66,14 +80,17 @@ TEST(private_waitv)
 	}
 
 	/* Private waitv */
+	waiter_done = 0;
 	ASSERT_EQ(pthread_create(&waiter, NULL, waiterfn, _metadata), 0)
 		TH_LOG("pthread_create failed");
 
-	usleep(WAKE_WAIT_US);
-
-	res = futex_wake(u64_to_ptr(waitv[NR_FUTEXES - 1].uaddr), 1, FUTEX_PRIVATE_FLAG);
+	res = wake_waiter(u64_to_ptr(waitv[NR_FUTEXES - 1].uaddr), FUTEX_PRIVATE_FLAG);
 	EXPECT_EQ(res, 1)
 		TH_LOG("futex_wake private returned: %d %s", res, res < 0 ? strerror(errno) : "");
+
+	res = pthread_join(waiter, NULL);
+	EXPECT_EQ(res, 0)
+		TH_LOG("pthread_join failed: %s", strerror(res));
 }
 
 TEST(shared_waitv)
@@ -104,15 +121,18 @@ TEST(shared_waitv)
 		waitv[i].__reserved = 0;
 	}
 
+	waiter_done = 0;
 	ASSERT_EQ(pthread_create(&waiter, NULL, waiterfn, _metadata), 0)
 		TH_LOG("pthread_create failed");
 
-	usleep(WAKE_WAIT_US);
-
-	res = futex_wake(u64_to_ptr(waitv[NR_FUTEXES - 1].uaddr), 1, 0);
+	res = wake_waiter(u64_to_ptr(waitv[NR_FUTEXES - 1].uaddr), 0);
 	EXPECT_EQ(res, 1)
 		TH_LOG("futex_wake shared returned: %d %s", res, res < 0 ? strerror(errno) : "");
 
+	res = pthread_join(waiter, NULL);
+	EXPECT_EQ(res, 0)
+		TH_LOG("pthread_join failed: %s", strerror(res));
+
 	for (i = 0; i < NR_FUTEXES; i++)
 		shmdt(u64_to_ptr(waitv[i].uaddr));
 }
-- 
2.47.3

[PATCH v2 4/4] selftests/futex: Remove the shared memory segments of the futex tests

[Mathieu Desnoyers]

anon_page in futex_wait, and shared_waitv in futex_waitv, create System V
shared memory segments with shmget(IPC_PRIVATE). They detach them with
shmdt(), but never remove them. So each run leaks one segment for
anon_page, and 30 for shared_waitv. After about 136 runs of futex_waitv,
shmget() fails, at the default limit of 4096 segments.

Mark each segment for removal with shmctl(IPC_RMID) once it is attached.
The kernel then removes it at the last detach, or when the test exits.

With this patch, futex_waitv passed 2000 runs in a row in each of two
8-CPU VMs, and futex_wait 2000 runs.

Fixes: c3d128581f64 ("selftests: futex: Add futex wait test")
Fixes: 5e59c1d1c78c ("selftests: futex: Add sys_futex_waitv() test")
Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
---
 tools/testing/selftests/futex/functional/futex_wait.c  | 2 ++
 tools/testing/selftests/futex/functional/futex_waitv.c | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/tools/testing/selftests/futex/functional/futex_wait.c b/tools/testing/selftests/futex/functional/futex_wait.c
index 4bf7d8ee238b..4b7550aff4d0 100644
--- a/tools/testing/selftests/futex/functional/futex_wait.c
+++ b/tools/testing/selftests/futex/functional/futex_wait.c
@@ -115,6 +115,8 @@ TEST(anon_page)
 		ASSERT_NE(shared_data, (void *)-1)
 			TH_LOG("shmat failed: %s", strerror(errno));
 	}
+	/* The segment goes away once detached. */
+	shmctl(shm_id, IPC_RMID, NULL);
 
 	*shared_data = 0;
 	futex = shared_data;
diff --git a/tools/testing/selftests/futex/functional/futex_waitv.c b/tools/testing/selftests/futex/functional/futex_waitv.c
index ccfa31fd20fa..94ada55b83cf 100644
--- a/tools/testing/selftests/futex/functional/futex_waitv.c
+++ b/tools/testing/selftests/futex/functional/futex_waitv.c
@@ -114,6 +114,8 @@ TEST(shared_waitv)
 
 		unsigned int *shared_data = shmat(shm_id, NULL, 0);
 
+		/* The segment goes away once detached. */
+		shmctl(shm_id, IPC_RMID, NULL);
 		*shared_data = 0;
 		waitv[i].uaddr = (uintptr_t)shared_data;
 		waitv[i].flags = FUTEX_32;
-- 
2.47.3