Re: [LTP] [PATCH v6] kill01: New case cgroup kill

[Petr Vorel <pvorel@suse.cz>]

Hi Wei,

NOTE: it's slightly easier to review if you write sum of changes from previous version.

...
> +++ b/lib/tst_cgroup.c
> @@ -94,9 +94,10 @@ enum cgroup_ctrl_indx {
>  	CTRL_MISC,
>  	CTRL_PERFEVENT,
>  	CTRL_DEBUG,
> -	CTRL_RDMA
> +	CTRL_RDMA,
> +	CTRL_PSEUDO
>  };
> -#define CTRLS_MAX CTRL_RDMA
> +#define CTRLS_MAX CTRL_PSEUDO

Maybe CTRL_PSEUDO should be added as a separate commit with a proper description
why it's needed.

...
> +static const struct cgroup_file cgroup_pseudo_ctrl_files[] = {
> +	{ }
> +};
> +
>  #define CTRL_NAME_MAX 31
>  #define CGROUP_CTRL_MEMBER(x, y)[y] = { .ctrl_name = #x, .files = \
>  	x ## _ctrl_files, .ctrl_indx = y, NULL, 0 }
> @@ -281,6 +287,7 @@ static struct cgroup_ctrl controllers[] = {
>  	CGROUP_CTRL_MEMBER(perf_event, CTRL_PERFEVENT),
>  	CGROUP_CTRL_MEMBER(debug, CTRL_DEBUG),
>  	CGROUP_CTRL_MEMBER(rdma, CTRL_RDMA),
> +	CGROUP_CTRL_MEMBER(cgroup_pseudo, CTRL_PSEUDO),
>  	{ }
>  };

> @@ -826,6 +833,10 @@ void tst_cg_require(const char *const ctrl_name,
>  	if (options->needs_ver != TST_CG_V2)
>  		cgroup_mount_v1(ctrl);

> +	if (!strcmp(ctrl->ctrl_name, "cgroup_pseudo")) {
> +		ctrl->ctrl_root = roots;
> +	}

tst_cgroup.c:836: WARNING: braces {} are not necessary for single statement blocks
=> please remove { }

Also strcmp for "cgroup_pseudo" is checked 3x in tst_cg_require(), why not creating
a variable and use it in if ()?
int cgroup_pseudo = !strcmp(ctrl->ctrl_name, "cgroup_pseudo");

> +
>  	if (!ctrl->ctrl_root) {
>  		tst_brk(TCONF,
>  			"'%s' controller required, but not available",
> @@ -848,13 +859,15 @@ mkdirs:
>  			ctrl->ctrl_name);
>  	}

> -	if (cgroup_ctrl_on_v2(ctrl)) {
> -		if (root->we_mounted_it) {
> -			SAFE_FILE_PRINTFAT(root->mnt_dir.dir_fd,
> -					   cgsc, "+%s", ctrl->ctrl_name);
> -		} else {
> -			tst_file_printfat(root->mnt_dir.dir_fd,
> -					  cgsc, "+%s", ctrl->ctrl_name);
> +	if (strcmp(ctrl->ctrl_name,"cgroup_pseudo")) {
nit: missing space after comma. I actually found by searching for previous
strcmp line (with comma) and it found just 2 places instead of 3, but make check
would report it for you:

make check-tst_cgroup
tst_cgroup.c:836: WARNING: braces {} are not necessary for single statement blocks
tst_cgroup.c:862: ERROR: space required after that ',' (ctx:VxV)
tst_cgroup.c:879: ERROR: space required after that ',' (ctx:VxV)

> +		if (cgroup_ctrl_on_v2(ctrl)) {
> +			if (root->we_mounted_it) {
> +				SAFE_FILE_PRINTFAT(root->mnt_dir.dir_fd,
> +						cgsc, "+%s", ctrl->ctrl_name);
> +			} else {
> +				tst_file_printfat(root->mnt_dir.dir_fd,
> +						cgsc, "+%s", ctrl->ctrl_name);
> +			}
>  		}
>  	}

> @@ -863,15 +876,17 @@ mkdirs:
>  	else
>  		root->ltp_dir.ctrl_field |= root->mnt_dir.ctrl_field;

> -	if (cgroup_ctrl_on_v2(ctrl)) {
> -		SAFE_FILE_PRINTFAT(root->ltp_dir.dir_fd,
> -				   cgsc, "+%s", ctrl->ctrl_name);
> -	} else {
> -		SAFE_FILE_PRINTFAT(root->ltp_dir.dir_fd,
> -				   "cgroup.clone_children", "%d", 1);
> +	if (strcmp(ctrl->ctrl_name,"cgroup_pseudo")) {
> +		if (cgroup_ctrl_on_v2(ctrl)) {
> +			SAFE_FILE_PRINTFAT(root->ltp_dir.dir_fd,
> +					cgsc, "+%s", ctrl->ctrl_name);
> +		} else {
> +			SAFE_FILE_PRINTFAT(root->ltp_dir.dir_fd,
> +					"cgroup.clone_children", "%d", 1);

> -		if (ctrl->ctrl_indx == CTRL_CPUSET)
> -			cgroup_copy_cpuset(root);
> +			if (ctrl->ctrl_indx == CTRL_CPUSET)
> +				cgroup_copy_cpuset(root);
> +		}
>  	}

>  	cgroup_dir_mk(&root->ltp_dir, cgroup_ltp_drain_dir, &root->drain_dir);
> @@ -1049,8 +1064,10 @@ static void cgroup_group_add_dir(const struct tst_cg_group *const parent,
>  		if (!parent || dir->dir_root->ver == TST_CG_V1)
>  			continue;

> -		SAFE_CG_PRINTF(parent, "cgroup.subtree_control",
> -				   "+%s", ctrl->ctrl_name);
> +		if (strcmp(ctrl->ctrl_name, "cgroup_pseudo")) {
> +			SAFE_CG_PRINTF(parent, "cgroup.subtree_control",
> +					"+%s", ctrl->ctrl_name);
> +		}
>  	}

>  	for (i = 0; cg->dirs[i]; i++)
> diff --git a/runtest/controllers b/runtest/controllers
> index 8d1b936bf..2f69a8ec2 100644
> --- a/runtest/controllers
> +++ b/runtest/controllers
> @@ -23,6 +23,7 @@ memcontrol01 memcontrol01
>  memcontrol02 memcontrol02
>  memcontrol03 memcontrol03
>  memcontrol04 memcontrol04
> +kill01 kill01

>  cgroup_fj_function_debug cgroup_fj_function.sh debug
>  cgroup_fj_function_cpuset cgroup_fj_function.sh cpuset
> diff --git a/testcases/kernel/controllers/cgroup/kill/.gitignore b/testcases/kernel/controllers/cgroup/kill/.gitignore
> new file mode 100644
> index 000000000..4f9649e27
> --- /dev/null
> +++ b/testcases/kernel/controllers/cgroup/kill/.gitignore
> @@ -0,0 +1 @@
> +/kill01
> diff --git a/testcases/kernel/controllers/cgroup/kill/Makefile b/testcases/kernel/controllers/cgroup/kill/Makefile
> new file mode 100644
> index 000000000..bf5aea9e7
> --- /dev/null
> +++ b/testcases/kernel/controllers/cgroup/kill/Makefile
> @@ -0,0 +1,6 @@
> +# SPDX-License-Identifier: GPL-2.0-or-later
> +
> +top_srcdir		?= ../../../../../
> +
> +include $(top_srcdir)/include/mk/testcases.mk
> +include $(top_srcdir)/include/mk/generic_leaf_target.mk
> diff --git a/testcases/kernel/controllers/cgroup/kill/kill01.c b/testcases/kernel/controllers/cgroup/kill/kill01.c
> new file mode 100644
> index 000000000..0da6de350
> --- /dev/null
> +++ b/testcases/kernel/controllers/cgroup/kill/kill01.c
> @@ -0,0 +1,149 @@
> +// 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 kernel self test case
nit: I'd just wrote kselftest instead of "kernel self test case".

> + * tools/testing/selftests/cgroup/test_kill.c

BTW am I correct that you converted only test_cgkill_simple() ?
There are 2 other: test_cgkill_tree() and test_cgkill_forkbomb(),
I have no idea if they are worth to be implemented or should be implemented now.
But maybe it'd be worth to mention that only simple test case was backported.


> + *
> + */
> +
> +#include <errno.h>
> +#include <linux/limits.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <sys/types.h>
> +#include <unistd.h>
> +#include <sys/wait.h>
nit: only <sys/wait.h> is needed (others either imported by tst_test.h or needed
in the original kselftest but not in LTP port).

> +
> +#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)
constants should be upper case (PID_NUM and BUF_LEN).

Kind regards,
Petr

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