Re: [LTP] [PATCH v8 1/2] cpuset02: Convert the test6 from cpuset_memory_testset.sh to C code

Li Wang via ltp
2025-03-28T09:35:45Z

Hi Wei,

Geneally looks good, but some tiny places need improvement.
See comments inline.

On Fri, Mar 28, 2025 at 4:00 PM Wei Gao <wegao@suse.com> wrote:

> Signed-off-by: Wei Gao <wegao@suse.com>
> Reviewed-by: Petr Vorel <pvorel@suse.cz>
> Suggested-by: Li Wang <liwang@redhat.com>
> ---
> lib/tst_cgroup.c | 1 +
> runtest/mm | 1 +
> testcases/kernel/mem/.gitignore | 1 +
> testcases/kernel/mem/cpuset/Makefile | 5 +
> testcases/kernel/mem/cpuset/cpuset02.c | 138 +++++++++++++++++++++++++
> 5 files changed, 146 insertions(+)
> create mode 100644 testcases/kernel/mem/cpuset/cpuset02.c
>
> diff --git a/lib/tst_cgroup.c b/lib/tst_cgroup.c
> index 73b696c58..545c779e7 100644
> --- a/lib/tst_cgroup.c
> +++ b/lib/tst_cgroup.c
> @@ -204,6 +204,7 @@ static const struct cgroup_file cpuset_ctrl_files[] = {
> { "cpuset.cpus", "cpuset.cpus", CTRL_CPUSET },
> { "cpuset.mems", "cpuset.mems", CTRL_CPUSET },
> { "cpuset.memory_migrate", "cpuset.memory_migrate", CTRL_CPUSET },
> + { "cpuset.sched_load_balance", "cpuset.sched_load_balance",
> CTRL_CPUSET },
>

cpuset.sched_load_balance is useful to enable/disable the scheduler can move
tasks between CPUs in the cpuset.

Is there any purpose to add this knob in cpuset02 patch? I didn't see you
touch it in the test.

{ }
> };
>
> diff --git a/runtest/mm b/runtest/mm
> index d8e62af81..5af29b0ea 100644
> --- a/runtest/mm
> +++ b/runtest/mm
> @@ -75,6 +75,7 @@ ksm06_2 ksm06 -n 8000
> ksm07 ksm07
>
> cpuset01 cpuset01
> +cpuset02 cpuset02
>
> oom01 oom01
> oom02 oom02
> diff --git a/testcases/kernel/mem/.gitignore
> b/testcases/kernel/mem/.gitignore
> index 699e022fb..e24e96001 100644
> --- a/testcases/kernel/mem/.gitignore
> +++ b/testcases/kernel/mem/.gitignore
> @@ -1,4 +1,5 @@
> /cpuset/cpuset01
> +/cpuset/cpuset02
> /hugetlb/hugefallocate/hugefallocate01
> /hugetlb/hugefallocate/hugefallocate02
> /hugetlb/hugefork/hugefork01
> diff --git a/testcases/kernel/mem/cpuset/Makefile
> b/testcases/kernel/mem/cpuset/Makefile
> index bac13e02b..7010c7be4 100644
> --- a/testcases/kernel/mem/cpuset/Makefile
> +++ b/testcases/kernel/mem/cpuset/Makefile
> @@ -19,6 +19,11 @@
>
> top_srcdir ?= ../../../..
>
> +LTPLIBS = numa
> +
> include $(top_srcdir)/include/mk/testcases.mk
> include $(top_srcdir)/testcases/kernel/include/lib.mk
> +
> +cpuset02: LTPLDLIBS = -lltpnuma
> +
> include $(top_srcdir)/include/mk/generic_leaf_target.mk
> diff --git a/testcases/kernel/mem/cpuset/cpuset02.c
> b/testcases/kernel/mem/cpuset/cpuset02.c
> new file mode 100644
> index 000000000..26b77f8be
> --- /dev/null
> +++ b/testcases/kernel/mem/cpuset/cpuset02.c
> @@ -0,0 +1,138 @@
> +// SPDX-License-Identifier: LGPL-2.1-or-later
> +/*
> + * Copyright (c) 2025 SUSE LLC <wegao@suse.com>
> + */
> +
> +/*\
> + * Test checks cpuset.mems works with hugepage file.
> + * Based on test6 from cpuset_memory_testset.sh written by Miao Xie.
> + */
> +
> +#define _GNU_SOURCE
> +#include <stdio.h>
> +#include <sys/mount.h>
> +#include <limits.h>
> +#include <sys/param.h>
> +#include <sys/types.h>
> +#include "tst_test.h"
> +
> +#ifdef HAVE_NUMA_V2
> +#include <numaif.h>
> +#include "tst_numa.h"
> +
> +#define MNTPOINT "hugetlbfs/"
> +#define HUGE_PAGE_FILE MNTPOINT "hugepagefile"
> +
> +static long hpage_size;
> +static struct tst_nodemap *node;
> +static int check_node_id;
> +static struct tst_cg_group *cg_cpuset_0;
> +
> +static void touch_memory_and_check_node(char *p, int size)
> +{
> + int i;
> + int node = -1;
> + long ret;
> + int pagesize = sysconf(_SC_PAGESIZE);
> +
> + for (i = 0; i < size; i += pagesize)
> + p[i] = 0xef;
> +
> + ret = get_mempolicy(&node, NULL, 0, p, MPOL_F_NODE | MPOL_F_ADDR);
> + if (ret < 0)
> + tst_brk(TBROK | TERRNO, "get_mempolicy() failed");
> +
> + if (node == check_node_id)
> + tst_res(TPASS, "check node pass");
>

I suggest printing the node for detailed info.
tst_res(TPASS, "1 huge page allocated on node-%d as expected", node);

> + else
> + tst_res(TFAIL, "check node failed");
>

tst_res(TFAIL, "1 huge page allocated on node-%d unexpected", node);

> +}
> +
> +static void child(void)
> +{
> + char *p;
> + int fd_hugepage;
> +
> + fd_hugepage = SAFE_OPEN(HUGE_PAGE_FILE, O_CREAT | O_RDWR, 0755);
> + p = SAFE_MMAP(NULL, hpage_size, PROT_WRITE | PROT_READ,
> + MAP_SHARED, fd_hugepage, 0);
> +
> + touch_memory_and_check_node(p, hpage_size);
> +
> + SAFE_MUNMAP(p, hpage_size);
> + SAFE_CLOSE(fd_hugepage);
> +}
> +
> +static void run_test(void)
> +{
> + int pid;
> + char node_id_str[256];
> +
> + cg_cpuset_0 = tst_cg_group_mk(tst_cg, "0");
> +
> + sprintf(node_id_str, "%u", check_node_id);
> + SAFE_CG_PRINT(cg_cpuset_0, "cpuset.mems", node_id_str);
> +
> + pid = SAFE_FORK();
> +
> + if (!pid) {
> + SAFE_CG_PRINTF(cg_cpuset_0, "cgroup.procs", "%d", pid);
> + child();
> + return;
> + }
> +
> + SAFE_WAITPID(pid, NULL, 0);
> +
> + cg_cpuset_0 = tst_cg_group_rm(cg_cpuset_0);
> +}
> +
> +static void setup(void)
> +{
> + node = tst_get_nodemap(TST_NUMA_MEM, getpagesize() / 1024);
> + if (node->cnt <= 1)
> + tst_brk(TCONF, "test requires at least 2 NUMA memory
> nodes");
> +
> + check_node_id = node->map[node->cnt - 1];
> +
> + hpage_size = SAFE_READ_MEMINFO(MEMINFO_HPAGE_SIZE)*1024;
> +
> + char path[256];
> + unsigned int i;
> +
> + for (i = 0; i < node->cnt; i++) {
> + unsigned int current_node_id = node->map[i];
> +
> + sprintf(path,
> +
> "/sys/devices/system/node/node%d/hugepages/hugepages-%ldkB/nr_hugepages",
> + current_node_id, hpage_size / 1024);
> + SAFE_FILE_PRINTF(path, "%d", 1);
>

SAFE_ macro will break if fails to set the value. However, the reservation
may not
succeed here due to memory fragmentation. So we can just use FILE_PRINTF().

Then, we need an additional check for the target node, if unable to
reserve, then TCONF there.

> + }
> +}
> +
> +static void cleanup(void)
> +{
> + if (cg_cpuset_0)
> + cg_cpuset_0 = tst_cg_group_rm(cg_cpuset_0);
> +}
> +
> +static struct tst_test test = {
> + .needs_root = 1,
>

> + .runs_script = 1,
>

Can we remove this line?

> + .mntpoint = MNTPOINT,
> + .needs_hugetlbfs = 1,
> + .setup = setup,
> + .forks_child = 1,
> + .cleanup = cleanup,
> + .test_all = run_test,
>

> + .needs_checkpoints = 1,
>

Remove this line?

> + .needs_cgroup_ver = TST_CG_V1,
>

The test is also useful to CG_V2, so let's remove this line as well.

> + .needs_cgroup_ctrls = (const char *const []){ "cpuset", NULL },
> + .save_restore = (const struct tst_path_val[]) {
> + {"/proc/sys/vm/nr_hugepages", NULL, TST_SR_TCONF},
> + {}
> + },
> +};
> +
> +#else
> +TST_TEST_TCONF(NUMA_ERROR_MSG);
> +#endif
> --
> 2.35.3
>
>

--
Regards,
Li Wang