posix-timers: Prefer delivery of signals to the current thread

From: Dmitry Vyukov <[email protected]>

Test that POSIX timers using CLOCK_PROCESS_CPUTIME_ID eventually deliver a signal to all running threads. This effectively tests that the kernel doesn't prefer any one thread (or subset of threads) for signal delivery.

Signed-off-by Dmitry Vyukov <[email protected]>

Signed-off-by Marco Elver <[email protected]>

---
v6:
- Update wording on what the test aims to test.
- Fix formatting per checkpatch.pl.

---
 tools/testing/selftests/timers/posix_timers.c | 77 +++++++++++++++++++
 1 file changed, 77 insertions(+)

tools/testing/selftests/timers/posix_timers.c +23 -0

index 0ba500056e63..8a17c0e8d82b 100644
@@ -188,6 +188,80 @@ static int check_timer_create(int which)
 	return 0;
 }
 
+int remain;
+__thread int got_signal;
+
+static void *distribution_thread(void *arg)
+{
+	while (__atomic_load_n(&remain, __ATOMIC_RELAXED));
+	return NULL;
+}
+
+static void distribution_handler(int nr)
+{
+	if (!__atomic_exchange_n(&got_signal, 1, __ATOMIC_RELAXED))
+		__atomic_fetch_sub(&remain, 1, __ATOMIC_RELAXED);
+}
+
+/*
+ * Test that all running threads _eventually_ receive CLOCK_PROCESS_CPUTIME_ID
+ * timer signals. This primarily tests that the kernel does not favour any one.
+ */
+static int check_timer_distribution(void)
+{
+	int err, i;
+	timer_t id;

tools/testing/selftests/timers/posix_timers.c +21 -0

+		return -1;
+	}
+	err = timer_settime(id, 0, &val, NULL);
+	if (err < 0) {
+		perror("Can't set timer\n");
+		return -1;
+	}
+
+	for (i = 0; i < nthreads; i++) {
+		if (pthread_create(&threads[i], NULL, distribution_thread, NULL)) {
+			perror("Can't create thread\n");
+			return -1;
+		}
+	}
+
+	/* Wait for all threads to receive the signal. */
+	while (__atomic_load_n(&remain, __ATOMIC_RELAXED));
+
+	for (i = 0; i < nthreads; i++) {
+		if (pthread_join(threads[i], NULL)) {
+			perror("Can't join thread\n");

tools/testing/selftests/timers/posix_timers.c +6 -0

+	return 0;
+}
+
 int main(int argc, char **argv)
 {
 	printf("Testing posix timers. False negative may happen on CPU execution \n");

@@ -217,5 +291,8 @@ int main(int argc, char **argv)
 	if (check_timer_create(CLOCK_PROCESS_CPUTIME_ID) < 0)
 		return ksft_exit_fail();
 
+	if (check_timer_distribution() < 0)
+		return ksft_exit_fail();
+
 	return ksft_exit_pass();
 }

Quoted 47 lines · Marco Elver

From: Dmitry Vyukov <[email protected]>

Test that POSIX timers using CLOCK_PROCESS_CPUTIME_ID eventually deliver a signal to all running threads. This effectively tests that the kernel doesn't prefer any one thread (or subset of threads) for signal delivery.

Signed-off-by Dmitry Vyukov <[email protected]>

Signed-off-by Marco Elver <[email protected]>

---
v6:
- Update wording on what the test aims to test.
- Fix formatting per checkpatch.pl.

---
 tools/testing/selftests/timers/posix_timers.c | 77 +++++++++++++++++++
 1 file changed, 77 insertions(+)

tools/testing/selftests/timers/posix_timers.c +23 -0

index 0ba500056e63..8a17c0e8d82b 100644
@@ -188,6 +188,80 @@ static int check_timer_create(int which)
 	return 0;
 }
 
+int remain;
+__thread int got_signal;
+
+static void *distribution_thread(void *arg)
+{
+	while (__atomic_load_n(&remain, __ATOMIC_RELAXED));
+	return NULL;
+}
+
+static void distribution_handler(int nr)
+{
+	if (!__atomic_exchange_n(&got_signal, 1, __ATOMIC_RELAXED))
+		__atomic_fetch_sub(&remain, 1, __ATOMIC_RELAXED);
+}
+
+/*
+ * Test that all running threads _eventually_ receive CLOCK_PROCESS_CPUTIME_ID
+ * timer signals. This primarily tests that the kernel does not favour any one.
+ */
+static int check_timer_distribution(void)
+{
+	int err, i;
+	timer_t id;

tools/testing/selftests/timers/posix_timers.c static int check_timer_create(int which) · line 223

Quoted 10 lines

tools/testing/selftests/timers/posix_timers.c +10 -0

+	const int nthreads = 10;
+	pthread_t threads[nthreads];
+	struct itimerspec val = {
+		.it_value.tv_sec = 0,
+		.it_value.tv_nsec = 1000 * 1000,
+		.it_interval.tv_sec = 0,
+		.it_interval.tv_nsec = 1000 * 1000,
+	};
+
+	printf("Check timer_create() per process signal distribution... ");

Use APIs from kselftest.h. Use ksft_print_msg() here.

tools/testing/selftests/timers/posix_timers.c static int check_timer_create(int which) · line 230

Quoted 7 lines

tools/testing/selftests/timers/posix_timers.c +7 -0

+	fflush(stdout);
+
+	remain = nthreads + 1;  /* worker threads + this thread */
+	signal(SIGALRM, distribution_handler);
+	err = timer_create(CLOCK_PROCESS_CPUTIME_ID, NULL, &id);
+	if (err < 0) {
+		perror("Can't create timer\n");

ksft_perror() here

Quoted 21 lines

tools/testing/selftests/timers/posix_timers.c +21 -0

+		return -1;
+	}
+	err = timer_settime(id, 0, &val, NULL);
+	if (err < 0) {
+		perror("Can't set timer\n");
+		return -1;
+	}
+
+	for (i = 0; i < nthreads; i++) {
+		if (pthread_create(&threads[i], NULL, distribution_thread, NULL)) {
+			perror("Can't create thread\n");
+			return -1;
+		}
+	}
+
+	/* Wait for all threads to receive the signal. */
+	while (__atomic_load_n(&remain, __ATOMIC_RELAXED));
+
+	for (i = 0; i < nthreads; i++) {
+		if (pthread_join(threads[i], NULL)) {
+			perror("Can't join thread\n");

tools/testing/selftests/timers/posix_timers.c static int check_timer_create(int which) · line 261

Quoted 10 lines

tools/testing/selftests/timers/posix_timers.c +10 -0

+			return -1;
+		}
+	}
+
+	if (timer_delete(id)) {
+		perror("Can't delete timer\n");
+		return -1;
+	}
+
+	printf("[OK]\n");

ksft_test_result or _pass variant as needed?

Quoted 15 lines

tools/testing/selftests/timers/posix_timers.c +6 -0

+	return 0;
+}
+
 int main(int argc, char **argv)
 {
 	printf("Testing posix timers. False negative may happen on CPU execution \n");

@@ -217,5 +291,8 @@ int main(int argc, char **argv)
 	if (check_timer_create(CLOCK_PROCESS_CPUTIME_ID) < 0)
 		return ksft_exit_fail();
 
+	if (check_timer_distribution() < 0)
+		return ksft_exit_fail();
+
 	return ksft_exit_pass();
 }

Signature · 2 lines

BR,
Muhammad Usama Anjum

Muhammad,

I am sorry, but... are you aware that this patch was applied over a year ago, and then this code was updated to use the ksft_API?

Oleg.

Quoted 123 lines · Muhammad Usama Anjum

Quoted 47 lines · Marco Elver

From: Dmitry Vyukov <[email protected]>

Test that POSIX timers using CLOCK_PROCESS_CPUTIME_ID eventually deliver a signal to all running threads. This effectively tests that the kernel doesn't prefer any one thread (or subset of threads) for signal delivery.

Signed-off-by Dmitry Vyukov <[email protected]>

Signed-off-by Marco Elver <[email protected]>

---
v6:
- Update wording on what the test aims to test.
- Fix formatting per checkpatch.pl.

---
 tools/testing/selftests/timers/posix_timers.c | 77 +++++++++++++++++++
 1 file changed, 77 insertions(+)

tools/testing/selftests/timers/posix_timers.c +23 -0

index 0ba500056e63..8a17c0e8d82b 100644
@@ -188,6 +188,80 @@ static int check_timer_create(int which)
 	return 0;
 }
 
+int remain;
+__thread int got_signal;
+
+static void *distribution_thread(void *arg)
+{
+	while (__atomic_load_n(&remain, __ATOMIC_RELAXED));
+	return NULL;
+}
+
+static void distribution_handler(int nr)
+{
+	if (!__atomic_exchange_n(&got_signal, 1, __ATOMIC_RELAXED))
+		__atomic_fetch_sub(&remain, 1, __ATOMIC_RELAXED);
+}
+
+/*
+ * Test that all running threads _eventually_ receive CLOCK_PROCESS_CPUTIME_ID
+ * timer signals. This primarily tests that the kernel does not favour any one.
+ */
+static int check_timer_distribution(void)
+{
+	int err, i;
+	timer_t id;

tools/testing/selftests/timers/posix_timers.c static int check_timer_create(int which) · line 223

Quoted 10 lines

tools/testing/selftests/timers/posix_timers.c +10 -0

+	const int nthreads = 10;
+	pthread_t threads[nthreads];
+	struct itimerspec val = {
+		.it_value.tv_sec = 0,
+		.it_value.tv_nsec = 1000 * 1000,
+		.it_interval.tv_sec = 0,
+		.it_interval.tv_nsec = 1000 * 1000,
+	};
+
+	printf("Check timer_create() per process signal distribution... ");

Use APIs from kselftest.h. Use ksft_print_msg() here.

tools/testing/selftests/timers/posix_timers.c static int check_timer_create(int which) · line 230

Quoted 7 lines

tools/testing/selftests/timers/posix_timers.c +7 -0

+	fflush(stdout);
+
+	remain = nthreads + 1;  /* worker threads + this thread */
+	signal(SIGALRM, distribution_handler);
+	err = timer_create(CLOCK_PROCESS_CPUTIME_ID, NULL, &id);
+	if (err < 0) {
+		perror("Can't create timer\n");

ksft_perror() here

Quoted 21 lines

tools/testing/selftests/timers/posix_timers.c +21 -0

+		return -1;
+	}
+	err = timer_settime(id, 0, &val, NULL);
+	if (err < 0) {
+		perror("Can't set timer\n");
+		return -1;
+	}
+
+	for (i = 0; i < nthreads; i++) {
+		if (pthread_create(&threads[i], NULL, distribution_thread, NULL)) {
+			perror("Can't create thread\n");
+			return -1;
+		}
+	}
+
+	/* Wait for all threads to receive the signal. */
+	while (__atomic_load_n(&remain, __ATOMIC_RELAXED));
+
+	for (i = 0; i < nthreads; i++) {
+		if (pthread_join(threads[i], NULL)) {
+			perror("Can't join thread\n");

tools/testing/selftests/timers/posix_timers.c static int check_timer_create(int which) · line 261

Quoted 10 lines

tools/testing/selftests/timers/posix_timers.c +10 -0

+			return -1;
+		}
+	}
+
+	if (timer_delete(id)) {
+		perror("Can't delete timer\n");
+		return -1;
+	}
+
+	printf("[OK]\n");

ksft_test_result or _pass variant as needed?

Quoted 15 lines

tools/testing/selftests/timers/posix_timers.c +6 -0

+	return 0;
+}
+
 int main(int argc, char **argv)
 {
 	printf("Testing posix timers. False negative may happen on CPU execution \n");

@@ -217,5 +291,8 @@ int main(int argc, char **argv)
 	if (check_timer_create(CLOCK_PROCESS_CPUTIME_ID) < 0)
 		return ksft_exit_fail();
 
+	if (check_timer_distribution() < 0)
+		return ksft_exit_fail();
+
 	return ksft_exit_pass();
 }

Signature · 2 lines

BR,
Muhammad Usama Anjum

From: Dmitry Vyukov <[email protected]>

POSIX timers using the CLOCK_PROCESS_CPUTIME_ID clock prefer the main
thread of a thread group for signal delivery. However, this has a
significant downside: it requires waking up a potentially idle thread.

Instead, prefer to deliver signals to the current thread (in the same thread group) if SIGEV_THREAD_ID is not set by the user. This does not change guaranteed semantics, since POSIX process CPU time timers have never guaranteed that signal delivery is to a specific thread (without SIGEV_THREAD_ID set).

The effect is that we no longer wake up potentially idle threads, and the kernel is no longer biased towards delivering the timer signal to any particular thread (which better distributes the timer signals esp. when multiple timers fire concurrently).

Signed-off-by Dmitry Vyukov <[email protected]>

Suggested-by Oleg Nesterov <[email protected]>

Reviewed-by Oleg Nesterov <[email protected]>

Signed-off-by Marco Elver <[email protected]>

From: Dmitry Vyukov <[email protected]>

POSIX timers using the CLOCK_PROCESS_CPUTIME_ID clock prefer the main
thread of a thread group for signal delivery. However, this has a
significant downside: it requires waking up a potentially idle thread.

Instead, prefer to deliver signals to the current thread (in the same thread group) if SIGEV_THREAD_ID is not set by the user. This does not change guaranteed semantics, since POSIX process CPU time timers have never guaranteed that signal delivery is to a specific thread (without SIGEV_THREAD_ID set).

The effect is that we no longer wake up potentially idle threads, and the kernel is no longer biased towards delivering the timer signal to any particular thread (which better distributes the timer signals esp. when multiple timers fire concurrently).

Signed-off-by Dmitry Vyukov <[email protected]>

Suggested-by Oleg Nesterov <[email protected]>

Reviewed-by Oleg Nesterov <[email protected]>

Signed-off-by Marco Elver <[email protected]>

---
v6:
- Split test from this patch.
- Update wording on what this patch aims to improve.

v5:
- Rebased onto v6.2.

v4:
- Restructured checks in send_sigqueue() as suggested.

v3:
- Switched to the completely different implementation (much simpler) based on the Oleg's idea.

RFC v2:
- Added additional Cc as Thomas asked.

---
 kernel/signal.c | 25 ++++++++++++++++++++++---
 1 file changed, 22 insertions(+), 3 deletions(-)

kernel/signal.c +17 -3

index 8cb28f1df294..605445fa27d4 100644
@@ -1003,8 +1003,7 @@ static void complete_signal(int sig, struct task_struct *p, enum pid_type type)
        /*
         * Now find a thread we can wake up to take the signal off the queue.
         *
-        * If the main thread wants the signal, it gets first crack.
-        * Probably the least surprising to the average bear.
+        * Try the suggested task first (may or may not be the main thread).
         */
        if (wants_signal(sig, p))
                t = p;

@@ -1970,8 +1969,23 @@ int send_sigqueue(struct sigqueue *q, struct pid *pid, enum pid_type type)
 
        ret = -1;
        rcu_read_lock();
+       /*
+        * This function is used by POSIX timers to deliver a timer signal.
+        * Where type is PIDTYPE_PID (such as for timers with SIGEV_THREAD_ID
+        * set), the signal must be delivered to the specific thread (queues
+        * into t->pending).
+        *
+        * Where type is not PIDTYPE_PID, signals must just be delivered to the
+        * current process. In this case, prefer to deliver to current if it is
+        * in the same thread group as the target, as it avoids unnecessarily
+        * waking up a potentially idle task.
+        */
        t = pid_task(pid, type);
-       if (!t || !likely(lock_task_sighand(t, &flags)))
+       if (!t)
+               goto ret;
+       if (type != PIDTYPE_PID && same_thread_group(t, current))
+               t = current;
+       if (!likely(lock_task_sighand(t, &flags)))
                goto ret;
 
        ret = 1; /* the signal is ignored */

@@ -1993,6 +2007,11 @@ int send_sigqueue(struct sigqueue *q, struct pid *pid, enum pid_type type)
        q->info.si_overrun = 0;
 

On Thu, 16 Mar 2023 at 13:31, Marco Elver <[email protected]> wrote:
One last semi-gentle ping. ;-)

1. We're seeing that in some applications that use POSIX timers heavily, but where the main thread is mostly idle, the main thread receives a disproportional amount of the signals along with being woken up constantly. This is bad, because the main thread usually waits with the help of a futex or really long sleeps. Now the main thread will steal time (to go back to sleep) from another thread that could have instead just proceeded with whatever it was doing.

2. Delivering signals to random threads is currently way too expensive. We need to resort to this crazy algorithm: 1) receive timer signal, 2) check if main thread, 3) if main thread (which is likely), pick a random thread and do tgkill. To find a random thread, iterate /proc/self/task, but that's just abysmal for various reasons. Other alternatives, like inherited task clock perf events are too expensive as soon as we need to enable/disable the timers (does IPIs), and maintaining O(#threads) timers is just as horrible.

This patch solves both the above issues.

We acknowledge the unfortunate situation of attributing this patch to one clear subsystem and owner: it straddles into signal delivery and POSIX timers territory, and perhaps some scheduling. The patch itself only touches kernel/signal.c.

If anyone has serious objections, please shout (soon'ish). Given the patch has been reviewed by Oleg, and scrutinized by Dmitry and myself, presumably we need to find a tree that currently takes kernel/signal.c patches?

Thanks!

-- Marco

From: Dmitry Vyukov <[email protected]>

POSIX timers using the CLOCK_PROCESS_CPUTIME_ID clock prefer the main
thread of a thread group for signal delivery. However, this has a
significant downside: it requires waking up a potentially idle thread.

Instead, prefer to deliver signals to the current thread (in the same thread group) if SIGEV_THREAD_ID is not set by the user. This does not change guaranteed semantics, since POSIX process CPU time timers have never guaranteed that signal delivery is to a specific thread (without SIGEV_THREAD_ID set).

The effect is that we no longer wake up potentially idle threads, and the kernel is no longer biased towards delivering the timer signal to any particular thread (which better distributes the timer signals esp. when multiple timers fire concurrently).

Signed-off-by Dmitry Vyukov <[email protected]>

Suggested-by Oleg Nesterov <[email protected]>

Reviewed-by Oleg Nesterov <[email protected]>

Signed-off-by Marco Elver <[email protected]>

---
 kernel/signal.c | 25 ++++++++++++++++++++++---
 1 file changed, 22 insertions(+), 3 deletions(-)

kernel/signal.c +22 -3

index 8cb28f1df294..605445fa27d4 100644
@@ -1003,8 +1003,7 @@ static void complete_signal(int sig, struct task_struct *p, enum pid_type type)
 	/*
 	 * Now find a thread we can wake up to take the signal off the queue.
 	 *
-	 * If the main thread wants the signal, it gets first crack.
-	 * Probably the least surprising to the average bear.
+	 * Try the suggested task first (may or may not be the main thread).
 	 */
 	if (wants_signal(sig, p))
 		t = p;

@@ -1970,8 +1969,23 @@ int send_sigqueue(struct sigqueue *q, struct pid *pid, enum pid_type type)
 
 	ret = -1;
 	rcu_read_lock();
+	/*
+	 * This function is used by POSIX timers to deliver a timer signal.
+	 * Where type is PIDTYPE_PID (such as for timers with SIGEV_THREAD_ID
+	 * set), the signal must be delivered to the specific thread (queues
+	 * into t->pending).
+	 *
+	 * Where type is not PIDTYPE_PID, signals must just be delivered to the
+	 * current process. In this case, prefer to deliver to current if it is
+	 * in the same thread group as the target, as it avoids unnecessarily
+	 * waking up a potentially idle task.
+	 */
 	t = pid_task(pid, type);
-	if (!t || !likely(lock_task_sighand(t, &flags)))
+	if (!t)
+		goto ret;
+	if (type != PIDTYPE_PID && same_thread_group(t, current))
+		t = current;
+	if (!likely(lock_task_sighand(t, &flags)))
 		goto ret;
 
 	ret = 1; /* the signal is ignored */

@@ -1993,6 +2007,11 @@ int send_sigqueue(struct sigqueue *q, struct pid *pid, enum pid_type type)
 	q->info.si_overrun = 0;
 
 	signalfd_notify(t, sig);
+	/*
+	 * If the type is not PIDTYPE_PID, we just use shared_pending, which
+	 * won't guarantee that the specified task will receive the signal, but
+	 * is sufficient if t==current in the common case.
+	 */
 	pending = (type != PIDTYPE_PID) ? &t->signal->shared_pending : &t->pending;
 	list_add_tail(&q->list, &pending->list);
 	sigaddset(&pending->signal, sig);

Signature · 1 line

2.40.0.rc1.284.g88254d51c5-goog

From: Dmitry Vyukov <[email protected]>

POSIX timers using the CLOCK_PROCESS_CPUTIME_ID clock prefer the main
thread of a thread group for signal delivery. However, this has a
significant downside: it requires waking up a potentially idle thread.

Instead, prefer to deliver signals to the current thread (in the same thread group) if SIGEV_THREAD_ID is not set by the user. This does not change guaranteed semantics, since POSIX process CPU time timers have never guaranteed that signal delivery is to a specific thread (without SIGEV_THREAD_ID set).

The effect is that we no longer wake up potentially idle threads, and the kernel is no longer biased towards delivering the timer signal to any particular thread (which better distributes the timer signals esp. when multiple timers fire concurrently).

Signed-off-by Dmitry Vyukov <[email protected]>

Suggested-by Oleg Nesterov <[email protected]>

Reviewed-by Oleg Nesterov <[email protected]>

Signed-off-by Marco Elver <[email protected]>

Quoted 18 lines · Marco Elver

From: Dmitry Vyukov <[email protected]>

POSIX timers using the CLOCK_PROCESS_CPUTIME_ID clock prefer the main
thread of a thread group for signal delivery. However, this has a
significant downside: it requires waking up a potentially idle thread.

Instead, prefer to deliver signals to the current thread (in the same thread group) if SIGEV_THREAD_ID is not set by the user. This does not change guaranteed semantics, since POSIX process CPU time timers have never guaranteed that signal delivery is to a specific thread (without SIGEV_THREAD_ID set).

The effect is that we no longer wake up potentially idle threads, and the kernel is no longer biased towards delivering the timer signal to any particular thread (which better distributes the timer signals esp. when multiple timers fire concurrently).

Quoted 4 lines

Signed-off-by Dmitry Vyukov <[email protected]>

Suggested-by Oleg Nesterov <[email protected]>

Reviewed-by Oleg Nesterov <[email protected]>

Signed-off-by Marco Elver <[email protected]>

Apologies for drudging up this old thread.

I wanted to ask if anyone had objections to including this in the -stable trees?

After this and the follow-on patch e797203fb3ba ("selftests/timers/posix_timers: Test delivery of signals across threads") landed, folks testing older kernels with the latest selftests started to see the new test checking for this behavior to stall. Thomas did submit an adjustment to the test here to avoid the
stall: https://lore.kernel.org/lkml/[email protected]/,
but it didn't seem to land, however that would just result in the test failing instead of hanging.

This change does seem to cherry-pick cleanly back to at least stable/linux-5.10.y cleanly, so it looks simple to pull this change back. But I wanted to make sure there wasn't anything subtle I was missing before sending patches.

Apologies for drudging up this old thread.
I wanted to ask if anyone had objections to including this in the -stable trees?

After this and the follow-on patch e797203fb3ba ("selftests/timers/posix_timers: Test delivery of signals across threads") landed, folks testing older kernels with the latest selftests started to see the new test checking for this behavior to stall. Thomas did submit an adjustment to the test here to avoid the
stall: https://lore.kernel.org/lkml/[email protected]/,
but it didn't seem to land, however that would just result in the test failing instead of hanging.

This change does seem to cherry-pick cleanly back to at least stable/linux-5.10.y cleanly, so it looks simple to pull this change back. But I wanted to make sure there wasn't anything subtle I was missing before sending patches.

Quoted 4 lines · John Stultz

This change does seem to cherry-pick cleanly back to at least stable/linux-5.10.y cleanly, so it looks simple to pull this change back. But I wanted to make sure there wasn't anything subtle I was missing before sending patches.

This test in particular exercises new functionality/behaviour, which really has no business to be backported into stable just to make the relevant test usable on older kernels.

Why would testing with latest tests against an older kernel be valid per se?

Quoted 3 lines · Thomas Gleixner

This test in particular exercises new functionality/behaviour, which really has no business to be backported into stable just to make the relevant test usable on older kernels.

That's fair. I didn't have all the context around what motivated the change and the follow-on test, which is why I'm asking here.

Quoted 2 lines

Why would testing with latest tests against an older kernel be valid per se?

So yeah, it definitely can get fuzzy trying to split hairs between when a change in behavior is a "new feature" or a "fix".

Greg could probably articulate it better, but my understanding is the main point for running newer tests on older kernels is that newer tests will have more coverage of what is expected of the kernel. For features that older kernels don't support, ideally the tests will check for that functionality like userland applications would, and skip that portion of the test if it's unsupported. This way, we're able to find issues (important enough to warrant tests having been created) that have not yet been patched in the -stable trees.

In this case, there is a behavioral change combined with a compliance test, which makes it look a bit more like a fix, rather than a feature (additionally the lack of a way for userland to probe for this new "feature" makes it seem fix-like). But the intended result of this is just spurring this discussion to see if it makes sense to backport or not. Disabling/ignoring the test (maybe after Thomas' fix to avoid it from hanging :) is a fine solution too, but not one I'd want folks to do until they've synced with maintainers and had full context.

The test if fragile as hell as there is absolutely no guarantee that the signal target distribution is as expected. The expectation is based on a statistical assumption which does not really hold.

Quoted 3 lines · Thomas Gleixner

The test if fragile as hell as there is absolutely no guarantee that the signal target distribution is as expected. The expectation is based on a statistical assumption which does not really hold.

Agreed. I too never liked this test-case.

I forgot everything about this patch and test-case, I can't really read your patch right now (sorry), so I am sure I missed something, but

tools/testing/selftests/timers/posix_timers.c static int check_timer_create(int which) · line 198

Quoted 11 lines

static void *distribution_thread(void *arg)
{

tools/testing/selftests/timers/posix_timers.c +6 -2

-	while (__atomic_load_n(&remain, __ATOMIC_RELAXED));
-	return NULL;
+	while (__atomic_load_n(&remain, __ATOMIC_RELAXED) && !done) {
+		if (got_signal)
+			usleep(10);
+	}
+
+	return (void *)got_signal;
 }

Why distribution_thread() can't simply exit if got_signal != 0 ?

See https://lore.kernel.org/all/[email protected]/

Quoted 4 lines · Oleg Nesterov

Why distribution_thread() can't simply exit if got_signal != 0 ?

See https://lore.kernel.org/all/[email protected]/

Indeed. It's too obvious :)

Subject: selftests/timers/posix_timers: Make signal distribution test less fragile
From: Thomas Gleixner <[email protected]>

The signal distribution test has a tendency to hang for a long time as the signal delivery is not really evenly distributed. In fact it might never be distributed across all threads ever in the way it is written.

Address this by:

1) Adding a timeout which aborts the test

2) Letting the test threads exit once they got a signal instead of running continuously. That ensures that the other threads will have a chance to expire the timer and get the signal.

3) Adding a detection whether all signals arrvied at the main thread, which allows to run the test on older kernels and emit 'SKIP'.

While at it get rid of the pointless atomic operation on a the thread local variable in the signal handler.

Signed-off-by Thomas Gleixner <[email protected]>

On Wed, Apr 3, 2024 at 9:32 AM Thomas Gleixner <[email protected]> wrote:
Thanks for this, Thomas!

Just FYI: testing with 6.1, the test no longer hangs, but I don't see the SKIP behavior. It just fails:
not ok 6 check signal distribution
# Totals: pass:5 fail:1 xfail:0 xpass:0 skip:0 error:0

I've not had time yet to dig into what's going on, but let me know if you need any further details.

Quoted 8 lines · John Stultz

On Wed, Apr 3, 2024 at 9:32 AM Thomas Gleixner <[email protected]> wrote:
Thanks for this, Thomas!

Just FYI: testing with 6.1, the test no longer hangs, but I don't see the SKIP behavior. It just fails:
not ok 6 check signal distribution
# Totals: pass:5 fail:1 xfail:0 xpass:0 skip:0 error:0

Quoted 2 lines

I've not had time yet to dig into what's going on, but let me know if you need any further details.

That's weird. I ran it on my laptop with 6.1.y ...

What kind of machine is that?

Quoted 13 lines · Thomas Gleixner

Quoted 10 lines · John Stultz

On Wed, Apr 3, 2024 at 9:32 AM Thomas Gleixner <tglx@linutronixde> wrote:
Thanks for this, Thomas!

Just FYI: testing with 6.1, the test no longer hangs, but I don't see the SKIP behavior. It just fails:
not ok 6 check signal distribution
# Totals: pass:5 fail:1 xfail:0 xpass:0 skip:0 error:0

I've not had time yet to dig into what's going on, but let me know if you need any further details.

Quoted 3 lines

That's weird. I ran it on my laptop with 6.1.y ...

What kind of machine is that?

I was running it in a VM.

Interestingly with 64cpus it sometimes will do the skip behavior, but with 4 cpus it seems to always fail.

IOW, we cannot test this reliably at all with the current approach.

Quoted 2 lines · Thomas Gleixner

IOW, we cannot test this reliably at all with the current approach.

Agreed!

So how about a REALLY SIMPLE test-case below?

Lacks error checking, should be updated to match tools/testing/selftests.

Without commit bcb7ee79029dca assert(sig_cnt > SIG_CNT) fails, the very 1st tick wakes the leader up.

With that commit it doesn't fail.

Yes, this changes the "semantics" of check_timer_distribution(), perhaps it should be renamed.

But I do not see a better approach, and in fact I think that

Test that all running threads _eventually_ receive CLOCK_PROCESS_CPUTIME_ID

is the wrong goal.

Do you agree?

Dmitry, Thomas,

To simplify the review I've attached the code with this patch applied below.

Yes, this changes the "semantics" of check_timer_distribution(), perhaps it should be renamed.

But I do not see a better approach, and in fact I think that

Test that all running threads _eventually_ receive CLOCK_PROCESS_CPUTIME_ID

is the wrong goal.

Do you agree?

Oleg.
-------------------------------------------------------------------------------

static pthread_t ctd_thread;
static volatile int ctd_count, ctd_failed;

static void ctd_sighandler(int sig)

{
        if (pthread_self() != ctd_thread)
                ctd_failed = 1;
        ctd_count--;
}

static void *ctd_thread_func(void *arg)

{
        struct itimerspec val = {
                .it_value.tv_sec = 0,
                .it_value.tv_nsec = 1000 * 1000,
                .it_interval.tv_sec = 0,
                .it_interval.tv_nsec = 1000 * 1000,
        };
        timer_t id;

        /* 1/10 seconds to ensure the leader sleeps */
        usleep(10000);

        ctd_count = 100;
        if (timer_create(CLOCK_PROCESS_CPUTIME_ID, NULL, &id))
                return "Can't create timer";
        if (timer_settime(id, 0, &val, NULL))
                return "Can't set timer";

        while (ctd_count > 0 && !ctd_failed)
                ;

        if (timer_delete(id))
                return "Can't delete timer";

        return NULL;
}

/*
* Test that only the running thread receives the timer signal. */
static int check_timer_distribution(void)

{
        const char *errmsg;

        signal(SIGALRM, ctd_sighandler);

        errmsg = "Can't create thread";
        if (pthread_create(&ctd_thread, NULL, ctd_thread_func, NULL))
                goto err;

        errmsg = "Can't join thread";
        if (pthread_join(ctd_thread, (void **)&errmsg) || errmsg)
                goto err;

        if (ctd_failed)
                ksft_test_result_skip("No signal distribution. Assuming old kernel\n");

        else
                ksft_test_result_pass("check signal distribution\n");

        return 0;
err:
        ksft_print_msg(errmsg);
        return -1;
}

Quoted 2 lines · Oleg Nesterov

        if (ctd_failed)
                ksft_test_result_skip("No signal distribution. Assuming old kernel\n");

Shouldn't the test fail here? The goal of a test is to fail when things don't work.
I don't see any other ksft_test_result_fail() calls, and it does not look that the test will hang on incorrect distribution.

Quoted 4 lines

        if (ctd_failed)
                ksft_test_result_skip("No signal distribution. Assuming old kernel\n");

Shouldn't the test fail here? The goal of a test is to fail when things don't work.

I don't see any other ksft_test_result_fail() calls, and it does not look that the test will hang on incorrect distribution.

Quoted 7 lines · Dmitry Vyukov

Quoted 4 lines

        if (ctd_failed)
                ksft_test_result_skip("No signal distribution. Assuming old kernel\n");

Shouldn't the test fail here? The goal of a test is to fail when things don't work.

I've copied this from the previous patch from Thomas, I am fine either way.

Quoted 2 lines

I don't see any other ksft_test_result_fail() calls, and it does not look that the test will hang on incorrect distribution.

Yes, it should never hang.

To me this test should simply do

	ksft_test_result(!ctd_failed, "check signal distribution\n");
	return 0;

but I am not familiar with tools/testing/selftests/ and I am not sure I understand the last email from Thomas.

The discussion started about running new tests on older kernels. As this is a feature and not a bug fix that obviously fails on older kernels.

So something like the uncompiled below should work.

tools/testing/selftests/timers/posix_timers.c +6 -0

+static int check_timer_distribution(void)
+{
+	const char *errmsg;
+
+	if (!check_kernel_version(6, 3)) {
+		ksft_test_result_skip("check signal distribution (old kernel)\n");
 		return 0;

Quoted 3 lines · Thomas Gleixner

The discussion started about running new tests on older kernels. As this is a feature and not a bug fix that obviously fails on older kernels.

OK, I see... please see below.

Quoted 1 line

So something like the uncompiled below should work.

Hmm... this patch doesn't apply to Linus's tree...

It seems that this is because in your tree check_timer_distribution() does

        if (timer_delete(id)) {
                ksft_perror("Can't delete timer");
                return 0;
        }

while in Linus's tree it returns -1 if timer_delete() fails. Nevermind.

Thomas, I am almost shy to continue this discussion and waste your time ;) But ...

tools/testing/selftests/timers/posix_timers.c static int check_timer_create(int which) · line 234

Quoted 10 lines

tools/testing/selftests/timers/posix_timers.c +10 -0

+static bool check_kernel_version(unsigned int min_major, unsigned int min_minor)
+{
+     unsigned int major, minor;
+     struct utsname info;
+
+     uname(&info);
+     if (sscanf(info.release, "%u.%u.", &major, &minor) != 2)
+             ksft_exit_fail();
+     return major > min_major || (major == min_major && minor >= min_minor);
+}

this looks useful regardless. Perhaps it should be moved into tools/testing/selftests/kselftest.h as ksft_ck_kernel_version() ?

Quoted 7 lines

tools/testing/selftests/timers/posix_timers.c +6 -0

+static int check_timer_distribution(void)
+{
+     const char *errmsg;
+
+     if (!check_kernel_version(6, 3)) {
+             ksft_test_result_skip("check signal distribution (old kernel)\n");
              return 0;

...

Quoted 1 line

+ ksft_test_result(!ctd_failed, "check signal distribution\n");

Perhaps

        if (!ctd_failed)
                ksft_test_result_pass("check signal distribution\n");
        else if (check_kernel_version(6, 3))
                ksft_test_result_fail("check signal distribution\n");
        else
                ksft_test_result_skip("check signal distribution (old kernel)\n");

makes more sense?

This way it can be used on the older kernels with bcb7ee79029d backported.

Oleg.

On 04/09, Thomas Gleixner wrote:
It seems that this is because in your tree check_timer_distribution() does

	if (timer_delete(id)) {
		ksft_perror("Can't delete timer");
		return 0;
	}

while in Linus's tree it returns -1 if timer_delete() fails. Nevermind.

Quoted 5 lines

tools/testing/selftests/timers/posix_timers.c +5 -0

+static bool check_kernel_version(unsigned int min_major, unsigned int min_minor)
+{
+	unsigned int major, minor;
+	struct utsname info;
+

tools/testing/selftests/timers/posix_timers.c static int check_timer_create(int which) · line 234

Quoted 5 lines

tools/testing/selftests/timers/posix_timers.c +5 -0

+	uname(&info);
+	if (sscanf(info.release, "%u.%u.", &major, &minor) != 2)
+		ksft_exit_fail();
+	return major > min_major || (major == min_major && minor >= min_minor);
+}

this looks useful regardless. Perhaps it should be moved into tools/testing/selftests/kselftest.h as ksft_ck_kernel_version() ?

Quoted 7 lines

tools/testing/selftests/timers/posix_timers.c +6 -0

+static int check_timer_distribution(void)
+{
+	const char *errmsg;
+
+	if (!check_kernel_version(6, 3)) {
+		ksft_test_result_skip("check signal distribution (old kernel)\n");
 		return 0;

..

tools/testing/selftests/timers/posix_timers.c static int check_timer_create(int which) · line 258

Quoted 1 line

tools/testing/selftests/timers/posix_timers.c +1 -0

+	ksft_test_result(!ctd_failed, "check signal distribution\n");

Perhaps

	if (!ctd_failed)
		ksft_test_result_pass("check signal distribution\n");
	else if (check_kernel_version(6, 3))
		ksft_test_result_fail("check signal distribution\n");
	else
		ksft_test_result_skip("check signal distribution (old kernel)\n");

makes more sense?

This way it can be used on the older kernels with bcb7ee79029d backported.

Thomas says:

The signal distribution test has a tendency to hang for a long time as the signal delivery is not really evenly distributed. In fact it might never be distributed across all threads ever in the way it is written.

To me even the

This primarily tests that the kernel does not favour any one.

comment doesn't look right. The kernel does favour a thread which hits the timer interrupt when CLOCK_PROCESS_CPUTIME_ID expires.

The new version simply checks that the group leader sleeping in join() never receives SIGALRM, cpu_timer_fire() should always send the signal to the thread which burns cpu.

Without the commit bcb7ee79029d ("posix-timers: Prefer delivery of signals to the current thread") the test-case fails immediately, the very 1st tick wakes the leader up. Otherwise it quickly succeeds after 100 ticks.

As Thomas suggested, the new version doesn't report the failure on the pre v6.3 kernels that do not have the commit bcb7ee79029d; this is a feature that obviously fails on the older kernels. So the patch adds the new simple ksft_ck_kernel_version() helper and uses ksft_test_result_skip() if check_timer_distribution() fails on the older kernel.

Signed-off-by Oleg Nesterov <[email protected]>

On Tue, Apr 9, 2024 at 6:39 AM Oleg Nesterov <[email protected]> wrote:
This is working great here (on both 6.6 and the older 6.1)! Thanks so much for fixing this!
One nit below, but otherwise:

Tested-by John Stultz <[email protected]>

tools/testing/selftests/timers/posix_timers.c static int check_timer_create(int which) · line 250

Quoted 2 lines

tools/testing/selftests/timers/posix_timers.c +2 -0

+err:
+       ksft_print_msg(errmsg);

This bit is causing the following warning:
posix_timers.c:250:2: warning: format not a string literal and no format arguments [-Wformat-security]

  250 |  ksft_print_msg(errmsg);
      |  ^~~~~~~~~~~~~~

A simple fix is just to switch it to:
ksft_print_msg("%s", errmsg);

Quoted 15 lines · John Stultz

On Tue, Apr 9, 2024 at 6:39 AM Oleg Nesterov <[email protected]> wrote:
This is working great here (on both 6.6 and the older 6.1)! Thanks so much for fixing this!
One nit below, but otherwise:

Tested-by John Stultz <[email protected]>

tools/testing/selftests/timers/posix_timers.c static int check_timer_create(int which) · line 250

Quoted 2 lines

tools/testing/selftests/timers/posix_timers.c +2 -0

+err:
+       ksft_print_msg(errmsg);

This bit is causing the following warning:
posix_timers.c:250:2: warning: format not a string literal and no format arguments [-Wformat-security]

  250 |  ksft_print_msg(errmsg);
      |  ^~~~~~~~~~~~~~

Quoted 2 lines

A simple fix is just to switch it to:
ksft_print_msg("%s", errmsg);

Can you please send a delta patch against tip timers/urgent?

Thomas says:

The signal distribution test has a tendency to hang for a long time as the signal delivery is not really evenly distributed. In fact it might never be distributed across all threads ever in the way it is written.

To me even the

This primarily tests that the kernel does not favour any one.

comment doesn't look right. The kernel does favour a thread which hits the timer interrupt when CLOCK_PROCESS_CPUTIME_ID expires.

The new version simply checks that the group leader sleeping in join() never receives SIGALRM, cpu_timer_fire() should always send the signal to the thread which burns cpu.

Without the commit bcb7ee79029d ("posix-timers: Prefer delivery of signals to the current thread") the test-case fails immediately, the very 1st tick wakes the leader up. Otherwise it quickly succeeds after 100 ticks.

Quoted 3 lines · Oleg Nesterov

Without the commit bcb7ee79029d ("posix-timers: Prefer delivery of signals to the current thread") the test-case fails immediately, the very 1st tick wakes the leader up. Otherwise it quickly succeeds after 100 ticks.

This has landed in -next and is causing warning spam throughout kselftest when built with clang:

/home/broonie/git/bisect/tools/testing/selftests/kselftest.h:435:6: warning: variable 'major' is used uninitialized whenever '||' condition is true [-Wsometimes-uninitialized]

        if (uname(&info) || sscanf(info.release, "%u.%u.", &major, &minor) != 2)
            ^~~~~~~~~~~~
/home/broonie/git/bisect/tools/testing/selftests/kselftest.h:438:9: note: uninitialized use occurs here
        return major > min_major || (major == min_major && minor >= min_minor);
               ^~~~~
/home/broonie/git/bisect/tools/testing/selftests/kselftest.h:435:6: note: remove the '||' if its condition is always false
        if (uname(&info) || sscanf(info.release, "%u.%u.", &major, &minor) != 2)
            ^~~~~~~~~~~~~~~
/home/broonie/git/bisect/tools/testing/selftests/kselftest.h:432:20: note: initialize the variable 'major' to silence this warning
        unsigned int major, minor;
                          ^
                           = 0

Thomas says:

The signal distribution test has a tendency to hang for a long time as the signal delivery is not really evenly distributed. In fact it might never be distributed across all threads ever in the way it is written.

To me even the

This primarily tests that the kernel does not favour any one.

Quoted 10 lines · Oleg Nesterov

Thomas says:

The signal distribution test has a tendency to hang for a long time as the signal delivery is not really evenly distributed. In fact it might never be distributed across all threads ever in the way it is written.

To me even the

This primarily tests that the kernel does not favour any one.

Further to my previous mail it's also broken the arm64 selftest builds, they use kselftest.h with nolibc in order to test low level functionality mainly used by libc implementations and nolibc doesn't implement uname():

In file included from za-fork.c:12:
../../kselftest.h:433:17: error: variable has incomplete type 'struct utsname'

        struct utsname info;
                       ^
../../kselftest.h:433:9: note: forward declaration of 'struct utsname'
        struct utsname info;
               ^
../../kselftest.h:435:6: error: call to undeclared function 'uname'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
        if (uname(&info) || sscanf(info.release, "%u.%u.", &major, &minor) != 2)
            ^
../../kselftest.h:435:22: error: call to undeclared function 'sscanf'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
        if (uname(&info) || sscanf(info.release, "%u.%u.", &major, &minor) != 2)

Quoted 8 lines · Mark Brown

Further to my previous mail it's also broken the arm64 selftest builds, they use kselftest.h with nolibc in order to test low level functionality mainly used by libc implementations and nolibc doesn't implement uname():

In file included from za-fork.c:12:
../../kselftest.h:433:17: error: variable has incomplete type 'struct utsname'

Quoted 10 lines

        struct utsname info;
                       ^
../../kselftest.h:433:9: note: forward declaration of 'struct utsname'
        struct utsname info;
               ^
../../kselftest.h:435:6: error: call to undeclared function 'uname'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
        if (uname(&info) || sscanf(info.release, "%u.%u.", &major, &minor) != 2)
            ^
../../kselftest.h:435:22: error: call to undeclared function 'sscanf'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
        if (uname(&info) || sscanf(info.release, "%u.%u.", &major, &minor) != 2)

Grrr. Let me stare at this.

Quoted 1 line · Thomas Gleixner

Grrr. Let me stare at this.

Damn ;)

Can't we just turn ksft_min_kernel_version() into

	static inline int ksft_min_kernel_version(unsigned int min_major,
						  unsigned int min_minor)
	{
	#ifdef NOLIBC
		return -1;
	#else

Not sure what should check_timer_distribution() do in this case, to me ksft_test_result_fail() is fine.

As Mark explains ksft_min_kernel_version() can't be compiled with nolibc, it doesn't implement uname().

Fixes 6d029c25b71f ("selftests/timers/posix_timers: Reimplement check_timer_distribution()")

Reported-by Mark Brown <[email protected]>

Closes https://lore.kernel.org/all/[email protected]/ open here

Signed-off-by Oleg Nesterov <[email protected]>

Quoted 22 lines · Oleg Nesterov

Quoted 3 lines · Thomas Gleixner

The test if fragile as hell as there is absolutely no guarantee that the signal target distribution is as expected. The expectation is based on a statistical assumption which does not really hold.

Agreed. I too never liked this test-case.

I forgot everything about this patch and test-case, I can't really read your patch right now (sorry), so I am sure I missed something, but

Quoted 11 lines

static void *distribution_thread(void *arg)
{

tools/testing/selftests/timers/posix_timers.c +6 -2

-    while (__atomic_load_n(&remain, __ATOMIC_RELAXED));
-    return NULL;
+    while (__atomic_load_n(&remain, __ATOMIC_RELAXED) && !done) {
+            if (got_signal)
+                    usleep(10);
+    }
+
+    return (void *)got_signal;
 }

Quoted 3 lines

Why distribution_thread() can't simply exit if got_signal != 0 ?

See https://lore.kernel.org/all/[email protected]/

Indeed. It's too obvious :)

Quoted 3 lines · Thomas Gleixner

Quoted 3 lines

Why distribution_thread() can't simply exit if got_signal != 0 ?

Quoted 3 lines

Quoted 1 line

See https://lore.kernel.org/all/[email protected]/

Indeed. It's too obvious :)

This test models the intended use-case that was the motivation for the change:
We want to sample execution of a running multi-threaded program, it has multiple active threads (that don't exit), since all threads are running and consuming CPU,

they all should get a signal eventually.

If threads will exit once they get a signal,

then the test will pass
even if signal delivery is biased towards a single running thread all the time (the previous kernel impl).

Quoted 1 line · Dmitry Vyukov

they all should get a signal eventually.

Well, yes and no.

No, in a sense that the motivation was not to ensure that all threads get a signal, the motivation was to ensure that cpu_timer_fire() paths will use the current task as the default target for signal_wake_up/etc. This is just optimization.

But yes, all should get a signal eventually. And this will happen with or without the commit bcb7ee79029dca ("posix-timers: Prefer delivery of signals to the current thread"). Any thread can dequeue a shared signal, say, on return from interrupt.

Just without that commit this "eventually" means A_LOT_OF_TIME statistically.

But yes, I agree, if thread exits once it get a signal, then A_LOT_OF_TIME will be significantly decreased. But again, this is just statistical issue, I do not see how can we test the commit bcb7ee79029dca reliably.

OTOH. If the threads do not exit after they get signal, then _in theory_ nothing can guarantee that this test-case will ever complete even with that commit. It is possible that one of the threads will "never" have a chance to run cpu_timer_fire().

In short, I leave this to you and Thomas. I have no idea how to write a "good" test for that commit.

Well... perhaps the main thread should just sleep in pause(), and distribution_handler() should check that gettid() != getpid() ? Something like this maybe... We need to ensure that the main thread enters pause before timer_settime().

Quoted 5 lines · Oleg Nesterov

And this will happen with
or without the commit bcb7ee79029dca ("posix-timers: Prefer delivery of signals to the current thread"). Any thread can dequeue a shared signal, say, on return from interrupt.

Quoted 2 lines

Just without that commit this "eventually" means A_LOT_OF_TIME statistically.

bcb7ee79029dca only directs the wakeup to current, but the signal is still queued in the process wide shared pending list. So the thread which sees sigpending() first will grab and deliver it to itself.

What we can actually test is the avoidance of waking up the main thread by doing the following in the main thread:

I'm testing a modification which implements something like the above and the success condition is that the main thread does not return early from nanosleep() and has no signal accounted. It survived 2000 iterations by now.

Perhaps I am totally confused, but.

Quoted 5 lines · Dmitry Vyukov

Quoted 3 lines · Thomas Gleixner

Quoted 3 lines

Why distribution_thread() can't simply exit if got_signal != 0 ?

Quoted 8 lines

Quoted 3 lines

Quoted 1 line

See https://lore.kernel.org/all/[email protected]/

Indeed. It's too obvious :)

This test models the intended use-case that was the motivation for the change:
We want to sample execution of a running multi-threaded program, it has multiple active threads (that don't exit), since all threads are running and consuming CPU,

Yes,

Quoted 1 line

they all should get a signal eventually.

Well, yes and no.

No, in a sense that the motivation was not to ensure that all threads get a signal, the motivation was to ensure that cpu_timer_fire() paths will use the current task as the default target for signal_wake_up/etc. This is just optimization.

But yes, all should get a signal eventually. And this will happen with or without the commit bcb7ee79029dca ("posix-timers: Prefer delivery of signals to the current thread"). Any thread can dequeue a shared signal, say, on return from interrupt.

Just without that commit this "eventually" means A_LOT_OF_TIME statistically.

Quoted 1 line

If threads will exit once they get a signal,

just in case, the main thread should not exit ...

Quoted 3 lines

then the test will pass
even if signal delivery is biased towards a single running thread all the time (the previous kernel impl).

See above.

But yes, I agree, if thread exits once it get a signal, then A_LOT_OF_TIME will be significantly decreased. But again, this is just statistical issue, I do not see how can we test the commit bcb7ee79029dca reliably.

OTOH. If the threads do not exit after they get signal, then _in theory_ nothing can guarantee that this test-case will ever complete even with that commit. It is possible that one of the threads will "never" have a chance to run cpu_timer_fire().

In short, I leave this to you and Thomas. I have no idea how to write a "good" test for that commit.

Well... perhaps the main thread should just sleep in pause(), and distribution_handler() should check that gettid() != getpid() ? Something like this maybe... We need to ensure that the main thread enters pause before timer_settime().

Oleg.