I'm sending some patches that were orignally in https://lore.kernel.org/lkml/20230606132949.068951363@linutronix.de/ to prevent the timer_distribution test from hanging and also fix some format inconsistencies.
Edward Liaw (3): selftests/timers/posix_timers: Make signal distribution test less fragile selftests/timers/posix_timers: Use TAP reporting format selftests/timers/posix_timers: Use llabs for long long
tools/testing/selftests/timers/posix_timers.c | 196 ++++++++---------- 1 file changed, 89 insertions(+), 107 deletions(-)
-- 2.44.0.rc1.240.g4c46232300-goog
The signal distribution test has a tendency to hang for a long time as the signal delivery is not really evenly distributed.
Increasing the timer interval to 10ms makes this less likely. Add a timeout to catch the case where it hangs and terminate the test gracefully.
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 tglx@linutronix.de [edliaw: Rebase and fix checkpatch recommendations] Signed-off-by: Edward Liaw edliaw@google.com --- tools/testing/selftests/timers/posix_timers.c | 43 ++++++++++++------- 1 file changed, 27 insertions(+), 16 deletions(-)
diff --git a/tools/testing/selftests/timers/posix_timers.c b/tools/testing/selftests/timers/posix_timers.c index d49dd3ffd0d9..03779b6b3c20 100644 --- a/tools/testing/selftests/timers/posix_timers.c +++ b/tools/testing/selftests/timers/posix_timers.c @@ -24,7 +24,8 @@ static volatile int done; /* Busy loop in userspace to elapse ITIMER_VIRTUAL */ static void user_loop(void) { - while (!done); + while (!done) + continue; }
/* @@ -184,18 +185,19 @@ static int check_timer_create(int which) return 0; }
-int remain; -__thread int got_signal; +static int remain; +static __thread int got_signal;
static void *distribution_thread(void *arg) { - while (__atomic_load_n(&remain, __ATOMIC_RELAXED)); + while (__atomic_load_n(&remain, __ATOMIC_RELAXED) && !done) + continue; return NULL; }
static void distribution_handler(int nr) { - if (!__atomic_exchange_n(&got_signal, 1, __ATOMIC_RELAXED)) + if (++got_signal == 1) __atomic_fetch_sub(&remain, 1, __ATOMIC_RELAXED); }
@@ -205,17 +207,19 @@ static void distribution_handler(int nr) */ static int check_timer_distribution(void) { - int err, i; - timer_t id; const int nthreads = 10; pthread_t threads[nthreads]; struct itimerspec val = { .it_value.tv_sec = 0, - .it_value.tv_nsec = 1000 * 1000, + .it_value.tv_nsec = 20 * 1000 * 1000, .it_interval.tv_sec = 0, - .it_interval.tv_nsec = 1000 * 1000, + .it_interval.tv_nsec = 20 * 1000 * 1000, }; + time_t start, now; + int err, i; + timer_t id;
+ done = 0; remain = nthreads + 1; /* worker threads + this thread */ signal(SIGALRM, distribution_handler); err = timer_create(CLOCK_PROCESS_CPUTIME_ID, NULL, &id); @@ -240,7 +244,18 @@ static int check_timer_distribution(void) }
/* Wait for all threads to receive the signal. */ - while (__atomic_load_n(&remain, __ATOMIC_RELAXED)); + now = start = time(NULL); + while (__atomic_load_n(&remain, __ATOMIC_RELAXED)) { + now = time(NULL); + if (now - start > 5) + break; + } + done = 1; + + if (timer_delete(id)) { + ksft_perror("Can't delete timer\n"); + return -1; + }
for (i = 0; i < nthreads; i++) { err = pthread_join(threads[i], NULL); @@ -251,12 +266,8 @@ static int check_timer_distribution(void) } }
- if (timer_delete(id)) { - ksft_perror("Can't delete timer"); - return -1; - } + ksft_test_result((now - start <= 5), "%s\n", __func__);
- ksft_test_result_pass("check_timer_distribution\n"); return 0; }
@@ -265,7 +276,7 @@ int main(int argc, char **argv) ksft_print_header(); ksft_set_plan(6);
- ksft_print_msg("Testing posix timers. False negative may happen on CPU execution \n"); + ksft_print_msg("Testing posix timers. False negative may happen on CPU execution\n"); ksft_print_msg("based timers if other threads run on the CPU...\n");
if (check_itimer(ITIMER_VIRTUAL) < 0)
No functional change.
Signed-off-by: Thomas Gleixner tglx@linutronix.de [edliaw: Fix checkpatch recommendations] Signed-off-by: Edward Liaw edliaw@google.com --- tools/testing/selftests/timers/posix_timers.c | 161 +++++++----------- 1 file changed, 66 insertions(+), 95 deletions(-)
diff --git a/tools/testing/selftests/timers/posix_timers.c b/tools/testing/selftests/timers/posix_timers.c index 03779b6b3c20..0f550fc9e879 100644 --- a/tools/testing/selftests/timers/posix_timers.c +++ b/tools/testing/selftests/timers/posix_timers.c @@ -10,6 +10,7 @@ #include <sys/time.h> #include <stdio.h> #include <signal.h> +#include <string.h> #include <unistd.h> #include <time.h> #include <pthread.h> @@ -19,6 +20,20 @@ #define DELAY 2 #define USECS_PER_SEC 1000000
+static void __fatal_error(const char *test, const char *name, const char *what) +{ + char buf[64]; + + strerror_r(errno, buf, sizeof(buf)); + + if (name && strlen(name)) + ksft_exit_fail_msg("%s %s %s %s\n", test, name, what, buf); + else + ksft_exit_fail_msg("%s %s %s\n", test, what, buf); +} + +#define fatal_error(name, what) __fatal_error(__func__, name, what) + static volatile int done;
/* Busy loop in userspace to elapse ITIMER_VIRTUAL */ @@ -67,15 +82,13 @@ static int check_diff(struct timeval start, struct timeval end) diff = end.tv_usec - start.tv_usec; diff += (end.tv_sec - start.tv_sec) * USECS_PER_SEC;
- if (abs(diff - DELAY * USECS_PER_SEC) > USECS_PER_SEC / 2) { - printf("Diff too high: %lld..", diff); + if (abs(diff - DELAY * USECS_PER_SEC) > USECS_PER_SEC / 2) return -1; - }
return 0; }
-static int check_itimer(int which) +static void check_itimer(int which) { const char *name; int err; @@ -91,7 +104,7 @@ static int check_itimer(int which) else if (which == ITIMER_REAL) name = "ITIMER_REAL"; else - return -1; + ksft_exit_fail_msg("Unknown setitimer() type %d\n", which);
done = 0;
@@ -103,16 +116,12 @@ static int check_itimer(int which) signal(SIGALRM, sig_handler);
err = gettimeofday(&start, NULL); - if (err < 0) { - ksft_perror("Can't call gettimeofday()"); - return -1; - } + if (err < 0) + fatal_error(name, "gettimeofday()");
err = setitimer(which, &val, NULL); - if (err < 0) { - ksft_perror("Can't set timer"); - return -1; - } + if (err < 0) + fatal_error(name, "setitimer()");
if (which == ITIMER_VIRTUAL) user_loop(); @@ -122,19 +131,15 @@ static int check_itimer(int which) idle_loop();
err = gettimeofday(&end, NULL); - if (err < 0) { - ksft_perror("Can't call gettimeofday()"); - return -1; - } + if (err < 0) + fatal_error(name, "gettimeofday()");
- ksft_test_result(check_diff(start, end) == 0, "%s\n", name); - - return 0; + ksft_test_result(!check_diff(start, end), "%s %s\n", __func__, name); }
-static int check_timer_create(int which) +static void check_timer_create(int which) { - const char *type; + const char *name; int err; timer_t id; struct timeval start, end; @@ -142,47 +147,37 @@ static int check_timer_create(int which) .it_value.tv_sec = DELAY, };
- if (which == CLOCK_THREAD_CPUTIME_ID) { - type = "thread"; - } else if (which == CLOCK_PROCESS_CPUTIME_ID) { - type = "process"; - } else { - ksft_print_msg("Unknown timer_create() type %d\n", which); - return -1; - } + if (which == CLOCK_THREAD_CPUTIME_ID) + name = "thread"; + else if (which == CLOCK_PROCESS_CPUTIME_ID) + name = "process"; + else + ksft_exit_fail_msg("Unknown timer_create() type %d\n", which);
done = 0; err = timer_create(which, NULL, &id); - if (err < 0) { - ksft_perror("Can't create timer"); - return -1; - } - signal(SIGALRM, sig_handler); + if (err < 0) + fatal_error(name, "timer_create()"); + + if (signal(SIGALRM, sig_handler) == SIG_ERR) + fatal_error(name, "signal()");
err = gettimeofday(&start, NULL); - if (err < 0) { - ksft_perror("Can't call gettimeofday()"); - return -1; - } + if (err < 0) + fatal_error(name, "gettimeofday()");
err = timer_settime(id, 0, &val, NULL); - if (err < 0) { - ksft_perror("Can't set timer"); - return -1; - } + if (err < 0) + fatal_error(name, "timer_settime()");
user_loop();
err = gettimeofday(&end, NULL); - if (err < 0) { - ksft_perror("Can't call gettimeofday()"); - return -1; - } + if (err < 0) + fatal_error(name, "gettimeofday()");
ksft_test_result(check_diff(start, end) == 0, - "timer_create() per %s\n", type); - - return 0; + "%s %s\n", __func__, name); }
static int remain; @@ -205,7 +200,7 @@ static void distribution_handler(int nr) * 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) +static void check_timer_distribution(void) { const int nthreads = 10; pthread_t threads[nthreads]; @@ -221,26 +216,20 @@ static int check_timer_distribution(void)
done = 0; remain = nthreads + 1; /* worker threads + this thread */ - signal(SIGALRM, distribution_handler); + if (signal(SIGALRM, distribution_handler) == SIG_ERR) + fatal_error(NULL, "signal()"); + err = timer_create(CLOCK_PROCESS_CPUTIME_ID, NULL, &id); - if (err < 0) { - ksft_perror("Can't create timer"); - return -1; - } + if (err < 0) + fatal_error(NULL, "timer_create()"); + err = timer_settime(id, 0, &val, NULL); - if (err < 0) { - ksft_perror("Can't set timer"); - return -1; - } + if (err < 0) + fatal_error(NULL, "timer_settime()");
for (i = 0; i < nthreads; i++) { - err = pthread_create(&threads[i], NULL, distribution_thread, - NULL); - if (err) { - ksft_print_msg("Can't create thread: %s (%d)\n", - strerror(errno), errno); - return -1; - } + if (pthread_create(&threads[i], NULL, distribution_thread, NULL)) + fatal_error(NULL, "pthread_create()"); }
/* Wait for all threads to receive the signal. */ @@ -252,23 +241,15 @@ static int check_timer_distribution(void) } done = 1;
- if (timer_delete(id)) { - ksft_perror("Can't delete timer\n"); - return -1; - } + if (timer_delete(id)) + fatal_error(NULL, "timer_delete()");
for (i = 0; i < nthreads; i++) { - err = pthread_join(threads[i], NULL); - if (err) { - ksft_print_msg("Can't join thread: %s (%d)\n", - strerror(errno), errno); - return -1; - } + if (pthread_join(threads[i], NULL)) + fatal_error(NULL, "pthread_join()"); }
ksft_test_result((now - start <= 5), "%s\n", __func__); - - return 0; }
int main(int argc, char **argv) @@ -279,17 +260,10 @@ int main(int argc, char **argv) ksft_print_msg("Testing posix timers. False negative may happen on CPU execution\n"); ksft_print_msg("based timers if other threads run on the CPU...\n");
- if (check_itimer(ITIMER_VIRTUAL) < 0) - return ksft_exit_fail(); - - if (check_itimer(ITIMER_PROF) < 0) - return ksft_exit_fail(); - - if (check_itimer(ITIMER_REAL) < 0) - return ksft_exit_fail(); - - if (check_timer_create(CLOCK_THREAD_CPUTIME_ID) < 0) - return ksft_exit_fail(); + check_itimer(ITIMER_VIRTUAL); + check_itimer(ITIMER_PROF); + check_itimer(ITIMER_REAL); + check_timer_create(CLOCK_THREAD_CPUTIME_ID);
/* * It's unfortunately hard to reliably test a timer expiration @@ -300,11 +274,8 @@ int main(int argc, char **argv) * to ensure true parallelism. So test only one thread until we * find a better solution. */ - if (check_timer_create(CLOCK_PROCESS_CPUTIME_ID) < 0) - return ksft_exit_fail(); - - if (check_timer_distribution() < 0) - return ksft_exit_fail(); + check_timer_create(CLOCK_PROCESS_CPUTIME_ID); + check_timer_distribution();
ksft_finished(); }
Fixes clang warning: absolute value function 'abs' given an argument of type 'long long' but has parameter of type 'int' which may cause truncation of value.
Signed-off-by: Edward Liaw edliaw@google.com --- tools/testing/selftests/timers/posix_timers.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/testing/selftests/timers/posix_timers.c b/tools/testing/selftests/timers/posix_timers.c index 0f550fc9e879..78b4b2d3dc44 100644 --- a/tools/testing/selftests/timers/posix_timers.c +++ b/tools/testing/selftests/timers/posix_timers.c @@ -82,7 +82,7 @@ static int check_diff(struct timeval start, struct timeval end) diff = end.tv_usec - start.tv_usec; diff += (end.tv_sec - start.tv_sec) * USECS_PER_SEC;
- if (abs(diff - DELAY * USECS_PER_SEC) > USECS_PER_SEC / 2) + if (llabs(diff - DELAY * USECS_PER_SEC) > USECS_PER_SEC / 2) return -1;
return 0;
On Mon, Mar 04 2024 at 18:11, Edward Liaw wrote:
I'm sending some patches that were orignally in https://lore.kernel.org/lkml/20230606132949.068951363@linutronix.de/ to prevent the timer_distribution test from hanging and also fix some format inconsistencies.
Thanks for picking those up and moving them forward. Any particular reason why you didn't pick up the full set?
Thanks,
tglx
linux-kselftest-mirror@lists.linaro.org
-
Edward Liaw -
Thomas Gleixner