On Wed, Jan 28 2026 at 18:24, Yuwen Chen wrote:
> #include "futextest.h"
> +#include "futex_thread.h"
> #include "kselftest_harness.h"
>
> -#define timeout_ns 30000000
> -#define WAKE_WAIT_US 10000
> +#define FUTEX_WAIT_TIMEOUT_S 3 /* 3s */
Please remove this horrible tail comment and make the define self
explaining:
#define FUTEX_WAIT_TIMEOUT_SECS 3
Does not need a redundant comment, right?
> +#define WAIT_THREAD_DELAY_US (10000 * 100) /* 1s */
Clearly there is not a more convoluted way to express microseconds per
second. Also WAIT_THREAD_DELAY_US is a gross misnomer.
#include <vdso/time64.h>
#define WAIT_THREAD_CREATE_TIMEOUT_USECS (USECS_PER_SEC)
Is too comprehensible, right?
Aside of that why does this need to be a per source constant. With the
wchan readout this can be the same for any thread creation, no?
> + ASSERT_EQ(0, futex_thread_create(&waiter, waiterfn, NULL));
> + futex_wait_for_thread(&waiter, WAIT_THREAD_DELAY_US);
Q: Why does this need two different functions?
A: Because ignoring the return value of the wait function is a great idea
> EXPECT_EQ(1, futex_cmp_requeue(f1, 0, &f2, 0, 1, 0));
> EXPECT_EQ(1, futex_wake(&f2, 1, 0));
> +
> + futex_thread_destroy(&waiter);
> }
>
> TEST(requeue_multiple)
> {
> volatile futex_t _f1 = 0;
> volatile futex_t f2 = 0;
> - pthread_t waiter[10];
> - int i;
> + struct futex_thread waiter[10];
https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#variable-declarations
And please read the rest of the document as well.
> f1 = &_f1;
>
> @@ -61,13 +62,17 @@ TEST(requeue_multiple)
> * Create 10 waiters at f1. At futex_requeue, wake 3 and requeue 7.
> * At futex_wake, wake INT_MAX (should be exactly 7).
> */
> - for (i = 0; i < 10; i++)
> - ASSERT_EQ(0, pthread_create(&waiter[i], NULL, waiterfn, NULL));
> + for (int i = 0; i < 10; i++)
> + ASSERT_EQ(0, futex_thread_create(&waiter[i], waiterfn, NULL));
>
> - usleep(WAKE_WAIT_US);
> + for (int i = 0; i < 10; i++)
> + futex_wait_for_thread(&waiter[i], WAIT_THREAD_DELAY_US / 10);
Again. What's the point of this split and why is the timeout here different?
It really does not matter at all whether the thread creation waits for
each thread separately or not and "optimizing" the timeout value here is
just pointless. This is a test suite and it does not matter whether the
failure case is detected after 3 seconds or 0.3 seconds. It should not
happen at all.
> EXPECT_EQ(10, futex_cmp_requeue(f1, 0, &f2, 3, 7, 0));
> EXPECT_EQ(7, futex_wake(&f2, INT_MAX, 0));
> +++ b/tools/testing/selftests/futex/include/futex_thread.h
> @@ -0,0 +1,100 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * library for futex tests
> + *
> + * Copyright 2026 Meizu Ltd.
I'm impressed about you slapping your company copyright on something I
almost verbatim provided to you without any form of appropriate
attribution. You might talk to your company lawyers about that.
> + */
> +#ifndef _FUTEX_THREAD_H
> +#define _FUTEX_THREAD_H
> +#include <stdio.h>
> +#include <string.h>
> +#include <errno.h>
> +
> +#define WAIT_THREAD_RETRIES 100
> +
> +struct futex_thread {
> + pthread_t thread;
> + pthread_barrier_t barrier;
> + pid_t tid;
> + void *(*threadfn)(void *);
> + void *arg;
> +};
I provided you a perfectly formatted data structure and you
converted it into something unreadable.
> +
> +static int __wait_for_thread(FILE *fp, int timeout_us)
Remove this pointless timeout argument.
> +{
> + char buf[80] = "";
> +
> + for (int i = 0; i < WAIT_THREAD_RETRIES; i++) {
> + if (!fgets(buf, sizeof(buf), fp))
> + return -EIO;
> + if (!strncmp(buf, "futex", 5))
> + return 0;
> + usleep(timeout_us / WAIT_THREAD_RETRIES);
And use a constant. The good case is determined by observing "futex" in
the wait channel. The bad case is irrelevant.
> + rewind(fp);
> + }
> + return -ETIMEDOUT;
> +}
> +
> +static void *__futex_thread_fn(void *arg)
> +{
> + struct futex_thread *t = arg;
> +
> + t->tid = gettid();
> + pthread_barrier_wait(&t->barrier);
> + return t->threadfn(t->arg);
> +}
> +
> +/**
> + * futex_wait_for_thread - Wait for the child thread to sleep in the futex
> context
> + * @t: Threads for testing.
Threads? How many of them. And what is this testing?
@t is a pointer to a data structure which the caller provides, but is
opaque to the caller in the same way as the pthread_t * argument of
pthread_create() is opaque to the caller.
> + * @timeout_us: Optional timeout for operation
Timeout is optional? Optional means 0 is valid, which results in a divide
by zero exception. This timeout argument is not required at all.
> + */
> +static inline int futex_wait_for_thread(struct futex_thread *t, int
> timeout_us)
Why "inline"? The compiler is clever enough to inline it and there is no
need for multiple instances of that when there is more than one place
using this.
> +{
> + char fname[80];
> + FILE *fp;
> + int res;
> +
> + snprintf(fname, sizeof(fname), "/proc/%d/wchan", t->tid);
> + fp = fopen(fname, "r");
> + if (!fp)
> + return -EIO;
> + res = __wait_for_thread(fp, timeout_us);
> + fclose(fp);
> + return res;
> +}
> +
> +/**
> + * futex_waitv - Create thread for testing.
> + * @t: Data used for returning from the thread
@t is a pointer to a caller provided struct and has nothing to do with
the returning from the thread.
> + * @threadfn: The new thread starts execution by invoking threadfn()
> + * @arg: arg is passed as the sole argument of threadfn()
> + */
> +static inline int futex_thread_create(struct futex_thread *t, void
> *(*threadfn)(void *), void *arg)
s/inline//
> +{
> + int ret;
> +
> + pthread_barrier_init(&t->barrier, NULL, 2);
> + t->tid = 0;
> + t->threadfn = threadfn;
> + t->arg = arg;
> +
> + ret = pthread_create(&t->thread, NULL, __futex_thread_fn, t);
> + if (ret)
> + return ret;
> +
> + pthread_barrier_wait(&t->barrier);
> + return 0;
This wants to be
return futex_wait_for_thread(t->tid);
> +}
> +
> +/**
> + * futex_waitv - Destroy thread.
> + * @t: Threads for testing.
How many threads are you testing for what?
This is about destroying the threads.
> + */
> +static inline void futex_thread_destroy(struct futex_thread *t)
This wants an 'void **retval' argument, which has to be set to NULL if
the caller is not interested in the return value;
> +{
> + pthread_join(t->thread, NULL);
And this becomes:
pthread_join(t->thread, retval);
Otherwise the
> + return t->threadfn(t->arg);
in futex_thread_fn() is completely pointless.
If you want to make it flexible for checking the return value then make
it correct. Otherwise remove the '*' from the threadfn definition and
return NULL in futex_thread_fn().
Also this needs to be split into pieces:
1) Provide the header file with the new functionality
2) Convert futex tests over to it (one patch per test)
Thanks,
tglx
