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

[Richard Palethorpe <rpalethorpe@suse.de>]

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.

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp