Re: [LTP] [PATCH v3 2/2] Refactor exit_group01 using new API

Richard Palethorpe
2023-10-16T10:34:15Z

Hello,

Andrea Cervesato <andrea.cervesato@suse.de> writes:

> From: Andrea Cervesato <andrea.cervesato@suse.com>
>
> We provided a different approach to exit_group() testing, spawning
> multiple threads inside the child and checking if they get killed with
> the parent process.
>
> Signed-off-by: Andrea Cervesato <andrea.cervesato@suse.com>
> ---
> Added thread_alive() function that checks existance of running threads, instead
> of using kill(tid, 0) method.
> tst_gettid() usage
>
> testcases/kernel/syscalls/exit_group/Makefile | 2 +
> .../kernel/syscalls/exit_group/exit_group01.c | 160 ++++++++++++------
> 2 files changed, 111 insertions(+), 51 deletions(-)
>
> diff --git a/testcases/kernel/syscalls/exit_group/Makefile b/testcases/kernel/syscalls/exit_group/Makefile
> index 1273a4e9c..adbac3c51 100644
> --- a/testcases/kernel/syscalls/exit_group/Makefile
> +++ b/testcases/kernel/syscalls/exit_group/Makefile
> @@ -3,6 +3,8 @@
>
> top_srcdir ?= ../../../..
>
> +exit_group01: CFLAGS+=-pthread
> +
> include $(top_srcdir)/include/mk/testcases.mk
>
> include $(top_srcdir)/include/mk/generic_leaf_target.mk
> diff --git a/testcases/kernel/syscalls/exit_group/exit_group01.c b/testcases/kernel/syscalls/exit_group/exit_group01.c
> index 5bf5b0218..2fcfff2f2 100644
> --- a/testcases/kernel/syscalls/exit_group/exit_group01.c
> +++ b/testcases/kernel/syscalls/exit_group/exit_group01.c
> @@ -1,68 +1,126 @@
> -/******************************************************************************
> - * Copyright (c) Crackerjack Project., 2007 *
> - * Ported to LTP by Manas Kumar Nayak <maknayak@in.ibm.com> *
> - * Copyright (C) 2015 Cyril Hrubis <chrubis@suse.cz> *
> - * *
> - * This program is free software; you can redistribute it and/or modify *
> - * it under the terms of the GNU General Public License as published by *
> - * the Free Software Foundation; either version 2 of the License, or *
> - * (at your option) any later version. *
> - * *
> - * This program is distributed in the hope that it will be useful, *
> - * but WITHOUT ANY WARRANTY; without even the implied warranty of *
> - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See *
> - * the GNU General Public License for more details. *
> - * *
> - * You should have received a copy of the GNU General Public License *
> - * along with this program; if not, write to the Free Software Foundation, *
> - * Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA *
> - * *
> - ******************************************************************************/
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (c) Crackerjack Project., 2007
> + * Ported to LTP by Manas Kumar Nayak <maknayak@in.ibm.com>
> + * Copyright (c) 2015 Linux Test Project
> + * Copyright (C) 2015 Cyril Hrubis <chrubis@suse.cz>
> + * Copyright (C) 2023 SUSE LLC Andrea Cervesato <andrea.cervesato@suse.com>
> + */
>
> -#include <stdio.h>
> -#include <errno.h>
> -#include <linux/unistd.h>
> -#include <sys/wait.h>
> +/*\
> + * [Description]
> + *
> + * This test checks if exit_group() correctly ends a spawned child and all its
> + * running threads.
> + */
>
> -#include "test.h"
> -#include "safe_macros.h"
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include "tst_safe_pthread.h"
> +#include "tst_test.h"
> #include "lapi/syscalls.h"
>
> -char *TCID = "exit_group01";
> -int testno;
> -int TST_TOTAL = 1;
> +#define THREADS_NUM 10
> +
> +static pid_t *tids;
>
> -static void verify_exit_group(void)
> +static void thread_alive(const int tid)
> {
> - pid_t cpid, w;
> - int status;
> + char folder[128];
> + struct stat sb;
> + char state;
>
> - cpid = fork();
> - if (cpid == -1)
> - tst_brkm(TFAIL | TERRNO, NULL, "fork failed");
> + snprintf(folder, sizeof(folder), "/proc/%i/stat", tid);
>
> - if (cpid == 0) {
> - TEST(tst_syscall(__NR_exit_group, 4));
> - } else {
> - w = SAFE_WAIT(NULL, &status);
> -
> - if (WIFEXITED(status) && (WEXITSTATUS(status) == 4)) {
> - tst_resm(TPASS, "exit_group() succeeded");
> - } else {
> - tst_resm(TFAIL | TERRNO,
> - "exit_group() failed (wait status = %d)", w);
> + for (;;) {
> + if (stat(folder, &sb) == -1) {
> + if (errno != ENOENT)
> + tst_brk(TBROK | TERRNO, "stat() error");
> +
> + break;
> + }
> +
> + SAFE_FILE_SCANF(folder, "%*i %*s %c", &state);
> +
> + if (state != 'S') {
> + tst_brk(TBROK,
> + "Thread %d is supposed to sleep but it's in '%c' state",
> + tid, state);
> }

This could fail due to PID reuse or maybe because state == 'Z'.

Perhaps instead we could check the threads are put into the Z state?

I guess that the child threads are reparented to init and it reaps
them. So you will have to prevent that by setting PR_SET_CHILD_SUBREAPER
on the main test process with prctl.

The man pages are not clear on this though. I guess this is how it works
from reading the kernel code.

> +
> + usleep(1000);

This probably should be sched_yield. It's hard to know what a sensible
sleep time would be for any given system.

> }
> }
>
> -int main(int ac, char **av)
> +static void *worker(void *arg)
> {
> - int lc;
> + int i = *((int *)arg);
>
> - tst_parse_opts(ac, av, NULL, NULL);
> + tids[i] = tst_gettid();
>
> - for (lc = 0; TEST_LOOPING(lc); lc++)
> - verify_exit_group();
> + TST_CHECKPOINT_WAKE(0);
> + pause();
>
> - tst_exit();
> + return arg;
> }
> +
> +static void spawn_threads(void)
> +{
> + pthread_t threads[THREADS_NUM];
> +
> + for (int i = 0; i < THREADS_NUM; i++) {
> + SAFE_PTHREAD_CREATE(&threads[i], NULL, worker, (void *)&i);
> + TST_CHECKPOINT_WAIT(0);
> +
> + /* wait for paused thread */
> + TST_PROCESS_STATE_WAIT(tids[i], 'S', 0);
> + }
> +}
> +
> +static void run(void)
> +{
> + pid_t pid;
> + int status;
> +
> + pid = SAFE_FORK();
> + if (!pid) {
> + spawn_threads();
> +
> + TEST(tst_syscall(__NR_exit_group, 4));
> + if (TST_RET == -1)
> + tst_brk(TBROK | TERRNO, "exit_group() error");
> +
> + return;
> + }
> +
> + SAFE_WAITPID(pid, &status, 0);

You could move this after thread_alive and check that the child threads
all eventually transition to Z.

You maybe need to wait on all the child threads as well if this process
is set as a sub-reaper. I don't see anything in the kernel to suggest
waiting on a normal PID will wait for its child threads.

Also exit_group appears to signal all the child threads as we would have
to do in userland if the syscall didn't exist.

> +
> + for (int i = 0; i < THREADS_NUM; i++)
> + thread_alive(tids[i]);
> +
> + TST_EXP_EXPR(WIFEXITED(status) && WEXITSTATUS(status) == 4,
> + "exit_group() succeeded");
> +}
> +
> +static void setup(void)
> +{
> + tids = SAFE_MMAP(
> + NULL,
> + sizeof(pid_t) * THREADS_NUM,
> + PROT_READ | PROT_WRITE,
> + MAP_SHARED | MAP_ANONYMOUS,
> + -1, 0);
> +}
> +
> +static void cleanup(void)
> +{
> + SAFE_MUNMAP(tids, sizeof(pid_t) * THREADS_NUM);
> +}
> +
> +static struct tst_test test = {
> + .setup = setup,
> + .cleanup = cleanup,
> + .test_all = run,
> + .forks_child = 1,
> + .needs_checkpoints = 1,
> +};
> --
> 2.35.3

--
Thank you,
Richard.