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

Petr Vorel
2023-03-29T06:28:41Z

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