Re: [LTP] [PATCH v9 1/2] kill01: New case cgroup kill

[Cyril Hrubis <chrubis@suse.cz>]

Hi!
> ---
>  lib/tst_cgroup.c                              |   1 +
>  runtest/controllers                           |   1 +
>  .../kernel/controllers/cgroup/.gitignore      |   1 +
>  .../kernel/controllers/cgroup/cgroup_core03.c | 144 ++++++++++++++++++
>  4 files changed, 147 insertions(+)
>  create mode 100644 testcases/kernel/controllers/cgroup/cgroup_core03.c
> 
> diff --git a/lib/tst_cgroup.c b/lib/tst_cgroup.c
> index 50699bc63..77575431d 100644
> --- a/lib/tst_cgroup.c
> +++ b/lib/tst_cgroup.c
> @@ -166,6 +166,7 @@ static const struct cgroup_file cgroup_ctrl_files[] = {
>  	{ "cgroup.controllers", NULL, 0 },
>  	{ "cgroup.subtree_control", NULL, 0 },
>  	{ "cgroup.clone_children", "cgroup.clone_children", 0 },
> +	{ "cgroup.kill", NULL, 0 },
>  	{ }
>  };
>  
> diff --git a/runtest/controllers b/runtest/controllers
> index 8d1b936bf..93c52c439 100644
> --- a/runtest/controllers
> +++ b/runtest/controllers
> @@ -1,6 +1,7 @@
>  #DESCRIPTION:Resource Management testing
>  cgroup_core01	cgroup_core01
>  cgroup_core02	cgroup_core02
> +cgroup_core03	cgroup_core03
>  cgroup		cgroup_regression_test.sh
>  memcg_regression	memcg_regression_test.sh
>  memcg_test_3	memcg_test_3
> diff --git a/testcases/kernel/controllers/cgroup/.gitignore b/testcases/kernel/controllers/cgroup/.gitignore
> index 8deae77da..9f1d1ada9 100644
> --- a/testcases/kernel/controllers/cgroup/.gitignore
> +++ b/testcases/kernel/controllers/cgroup/.gitignore
> @@ -2,3 +2,4 @@
>  /cgroup_regression_getdelays
>  /cgroup_core01
>  /cgroup_core02
> +/cgroup_core03
> diff --git a/testcases/kernel/controllers/cgroup/cgroup_core03.c b/testcases/kernel/controllers/cgroup/cgroup_core03.c
> new file mode 100644
> index 000000000..2a6941c44
> --- /dev/null
> +++ b/testcases/kernel/controllers/cgroup/cgroup_core03.c
> @@ -0,0 +1,144 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (c) 2012 Christian Brauner <brauner-AT-kernel.org>
> + * Copyright (c) 2023 SUSE LLC <wegao@suse.com>
> + */
> +
> +/*\
> + * [Description]
> + *
> + * This test is copied from kselftest
> + * tools/testing/selftests/cgroup/test_kill.c
> + * Only simple test implemented within current case, the other cases such
> + * as test_cgkill_tree and test_cgkill_forkbomb can be created later.
> + *
> + */
> +
> +#include <sys/wait.h>
> +
> +#include "lapi/syscalls.h"
> +#include "tst_test.h"
> +
> +#define MAX_PID_NUM 100
> +#define PID_NUM MIN(MAX_PID_NUM, (tst_ncpus_available() + 1))
> +#define BUF_LEN (20 * PID_NUM)
> +
> +static int *data_ptr;
> +static char *buf;
> +static struct tst_cg_group *cg_child_test_simple;
> +
> +static int wait_for_pid(pid_t pid)
> +{
> +	int status, ret;
> +
> +again:
> +	ret = waitpid(pid, &status, 0);
> +	if (ret == -1) {
> +		if (errno == EINTR)
> +			goto again;
> +
> +		return -1;
> +	}
> +
> +	if (WIFSIGNALED(status))
> +		return 0;
> +
> +	return -1;
> +}
> +
> +/*
> + * A simple process running in a sleep loop until being
> + * re-parented.
> + */
> +static void child_fn(void)
> +{
> +	int ppid = getppid();
> +
> +	while (getppid() == ppid)
> +		usleep(1000);
> +

What's the reason we are waiting for reparenting here? Aren't these
processes killed anyways? I suppose that we can just call pause() here
instead.

> +}
> +
> +static int cg_run_nowait(const struct tst_cg_group *const cg,
> +		  void (*fn)(void))
> +{
> +	int pid;
> +
> +	pid = SAFE_FORK();
> +	if (pid == 0) {
> +		SAFE_CG_PRINTF(cg, "cgroup.procs", "%d", getpid());
> +		tst_atomic_inc(data_ptr);
> +		if (tst_atomic_load(data_ptr) == PID_NUM)
> +			TST_CHECKPOINT_WAKE(0);

I do not think that this is 100% correct, as it is we may end up calling
the TST_CHECKOPOINT_WAKE(0) more than once here, since there is a race
between increment and load. An if that happens only one of the calls to
TST_CHECKPOINT_WAKE(0) will succeeds, while the rest would fail and
break the test.

First of all the tst_atomic_inc() returns the value after the increment,
so that we can actually get the counter value without a race.

Secondly I think that we can get rid of the atomic operations
completely, we just have to loop N times over the TST_CHECKPOINT_WAIT()
in the run() function and call TST_CHECKPOINT_WAKE() in each child
instance. We can also add TST_CHECKPOINT_WAIT_NR() to the test library
that would also take a number of wait calls passed as a parameter.

> +		fn();
> +	}
> +
> +	return pid;
> +}
> +
> +static int cg_wait_for_proc_count(const struct tst_cg_group *cg, int count)

The function does not wait for anything, so it should be called
cg_count_procs() or similar.

> +{
> +	char *ptr;
> +
> +	int nr = 0;
> +
> +	SAFE_CG_READ(cg, "cgroup.procs", buf, BUF_LEN);
> +
> +	for (ptr = buf; *ptr; ptr++)
> +		if (*ptr == '\n')
> +			nr++;
> +
> +	if (nr >= count)
> +		return 0;
> +
> +	tst_res(TINFO, "Expect process num is %d but get %d", count, nr);
> +
> +	return -1;
> +}
> +
> +static void run(void)
> +{
> +	pid_t pids[MAX_PID_NUM];
> +	int i;
> +	*data_ptr = 0;
> +
> +	cg_child_test_simple = tst_cg_group_mk(tst_cg, "cg_test_simple");
> +
> +	memset(buf, 0, BUF_LEN);
> +
> +	for (i = 0; i < PID_NUM; i++)
> +		pids[i] = cg_run_nowait(cg_child_test_simple, child_fn);
> +
> +	TST_CHECKPOINT_WAIT(0);
> +
> +	TST_EXP_PASS(cg_wait_for_proc_count(cg_child_test_simple, PID_NUM));

This is very minor, but I would make the function return the number of
pids found in the cgroup and called it as:

	TST_EXP_VAL(cg_count_procs(cg_child_test_simple), pid_num);

> +	SAFE_CG_PRINTF(cg_child_test_simple, "cgroup.kill", "%d", 1);
> +
> +	for (i = 0; i < PID_NUM; i++)
> +		TST_EXP_PASS_SILENT(wait_for_pid(pids[i]));

Then we can do here:

	TST_EXP_VAL(cg_count_procs(cg_child_test_simple), 0);

> +	cg_child_test_simple = tst_cg_group_rm(cg_child_test_simple);
> +}
> +
> +static void setup(void)
> +{
> +	buf = tst_alloc(BUF_LEN);
> +	data_ptr = SAFE_MMAP(NULL, sizeof(uintptr_t), PROT_READ | PROT_WRITE,
> +						 MAP_SHARED | MAP_ANONYMOUS, -1, 0);
> +}
> +
> +static void cleanup(void)
> +{
> +	if (data_ptr)
> +		SAFE_MUNMAP(data_ptr, sizeof(uintptr_t));
> +}
> +
> +static struct tst_test test = {
> +	.test_all = run,
> +	.setup = setup,
> +	.cleanup = cleanup,
> +	.forks_child = 1,
> +	.max_runtime = 20,
> +	.needs_cgroup_ctrls = (const char *const []){ "pseudo", NULL },
> +	.needs_cgroup_ver = TST_CG_V2,
> +	.needs_checkpoints = 1,
> +};
> -- 
> 2.35.3
> 
> 
> -- 
> Mailing list info: https://lists.linux.it/listinfo/ltp

-- 
Cyril Hrubis
chrubis@suse.cz

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