[LTP] [PATCH v8 1/2] cpuset02: Convert the test6 from cpuset_memory_testset.sh to C code
Li Wang
liwang@redhat.com
Fri Mar 28 11:57:38 CET 2025
On Fri, Mar 28, 2025 at 6:20 PM Petr Vorel <pvorel@suse.cz> wrote:
> Hi Li
>
> > Hi Wei,
>
> > Geneally looks good, but some tiny places need improvement.
> > See comments inline.
>
> Thanks for the review!
>
> > 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.
>
> Wei might had a different reason, but I see all functions in
> cpuset_memory_testset.sh (including test6 being rewritten to this test)
> call
> cpuset_funcs.sh which does 'echo 0 > .../cpuset.sched_load_balance', see:
>
> test6()
> {
> ...
> cpuset_set "$CPUSET/0" "$cpu_of_node0" "0" "0" 2>
> $CPUSET_TMP/stderr
>
> cpuset_set()
> {
> ...
> local load_balance="$4"
> ...
> /bin/echo $load_balance > $path/cpuset.sched_load_balance
>
> I wonder whether it's good or not.
>
Ok, I see, it was used to disable the task from moving between CPUs
in the cpuset. But to be honest, that's _not_ necessary, because whatever
the task is bound to which CPU of the cpuset, it all belongs to that
specified
node by limited Cgroup.
So I would drop this knob setting in new C test.
>
> Kind regards,
> Petr
>
> > { }
> > > };
>
> > > 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.
>
> I suppose we cannot do the reservation via struct tst_test, right? (using
> e.g. TST_SR_SKIP_RO from include/tst_sys_conf.h). That would require at
> least to
> add tst_get_nodemap() to struct tst_test, right?
>
You're right.
Do tst_test.hugepage reservation is not precise to a specified node.
We have to request that with dedicated code so far.
Maybe we can add something to the hpage reservation to support that,
but I don't see that feature as widely needed in LTP test. It might not
valuable to bring more complexity to the code just for this rarely used.
> > > + }
> > > +}
> > > +
> > > +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?
>
> +1, definitely not needed here (we don't use shell loader).
>
>
> > > + .mntpoint = MNTPOINT,
> > > + .needs_hugetlbfs = 1,
> > > + .setup = setup,
> > > + .forks_child = 1,
> > > + .cleanup = cleanup,
> > > + .test_all = run_test,
>
>
>
> > > + .needs_checkpoints = 1,
>
>
> > Remove this line?
>
> +1, Cyril asked for removing checkpoints, this is left.
>
> https://lore.kernel.org/ltp/Z88Ymlng3tEOKi0P@yuki.lan/
>
> > > + .needs_cgroup_ver = TST_CG_V1,
>
>
> > The test is also useful to CG_V2, so let's remove this line as well.
>
> +1. The original shell script required v1. But C API allows to use both.
>
> Kind regards,
> Petr
>
>
--
Regards,
Li Wang
More information about the ltp
mailing list