Re: [LTP] [PATCH v4] cpuset02: Reimplementing the test6 of cpuset_memory_testset.sh as a separate C testcase

Wei Gao via ltp
2024-11-05T05:30:48Z

On Fri, Nov 01, 2024 at 11:58:08AM +0100, Petr Vorel wrote:
> Hi Wei, Cyril,
>
> > "Reimplementing the test6 of cpuset_memory_testset.sh as a separate C testcase"
> This could be followed by removing test6 from cpuset_memory_testset.sh?
>
> The conversion itself from shell test test6 LGTM.
>
> I suppose the reason is that moving code to C improves stability of the test.
> But this would be nice to mention in the commit message. (Remember "what" versus
> "why" - often the reason for the change is more important that describing the
> change.)
>
> > index 8e41c0223..366d67ce9 100644
> > --- a/testcases/kernel/mem/cpuset/Makefile
> > +++ b/testcases/kernel/mem/cpuset/Makefile
> > @@ -19,6 +19,13 @@
>
> > top_srcdir ?= ../../../..
>
> > +LTPLIBS = numa
> > +
> > include $(top_srcdir)/include/mk/testcases.mk
> > include $(top_srcdir)/testcases/kernel/mem/include/libmem.mk
> > +
> > +LDLIBS += $(NUMA_LIBS)
> > +LDLIBS += $(top_srcdir)/testcases/kernel/controllers/cpuset/cpuset_lib/libcpu_set.a
> This is not enough, because there is no build dependency on libcpu_set.a.
>
> Other tests which depends on libcpu_set.a are in the subdirectories (e.g.
> testcases/kernel/controllers/cpuset/cpuset_hotplug_test/) and they use
> testcases/kernel/controllers/cpuset/Makefile.inc. Can you somehow use it?
>
> Maybe we should move cpuset_lib code to libs/ directory, when used elsewhere.
I have created another new patch for move lib code:
https://patchwork.ozlabs.org/project/ltp/patch/20241105052507.29630-1-wegao@suse.com/
>
> That would also help to avoid ugly includes like:
> > +#include "../../controllers/cpuset/cpuset_lib/cpuset.h"
>
> (This could be also improved with CFLAGS += -I../../controllers/cpuset/cpuset_lib/.)
>
>
> > +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..a62c98b3f
> > --- /dev/null
> > +++ b/testcases/kernel/mem/cpuset/cpuset02.c
> > @@ -0,0 +1,140 @@
> > +// SPDX-License-Identifier: LGPL-2.1-or-later
> > +/*
> > + * Copyright (c) 2009 FUJITSU LIMITED Miao Xie <miaox@cn.fujitsu.com>
> > + * Copyright (c) 2023 SUSE LLC <wegao@suse.com>
> > + */
> > +
> > +/*\
> > + * [Description]
> > + *
> > + * This test check cpuset's mems work with hugepage file.
> Maybe: Test checks cpuset.mems works with hugepage file.
>
> > + *
> Please remove the blank line above ^.
> > + */
> ...
>
> > +static void count_cpus_mems(void)
> > +{
> > + node = tst_get_nodemap(TST_NUMA_MEM, PAGES_ALLOCATED * getpagesize() / 1024);
> > + if (node->cnt <= 1)
> > + tst_brk(TCONF, "test requires NUMA system");
> > +}
> nit: Function used only once, I would move the code into setup().
>
> Later (separate effort), this function (with size parameter) could be in
> libs/numa/tst_numa.c, because it's used in many tests, or even added into struct
> tst_test.
>
> > +static void run_test(void)
> > +{
> ...
> > + pid = SAFE_FORK();
> > + if (!pid) {
> > + child();
> > + } else {
> > + SAFE_CG_PRINTF(cg_cpuset_0, "cgroup.procs", "%d", pid);
> > +
> > + TST_CHECKPOINT_WAKE(0);
> > + TST_CHECKPOINT_WAIT(1);
> > +
> > + SAFE_WAITPID(pid, NULL, 0);
> > +
> > + cg_cpuset_0 = tst_cg_group_rm(cg_cpuset_0);
> > + }
>
> Very nit: IMHO saving indent would be more readable:
>
> if (!pid) {
> child();
> return; // or call exit() in child() and no return here.
> }
>
> SAFE_CG_PRINTF(cg_cpuset_0, "cgroup.procs", "%d", pid);
>
> TST_CHECKPOINT_WAKE(0);
> TST_CHECKPOINT_WAIT(1);
>
> SAFE_WAITPID(pid, NULL, 0);
>
> cg_cpuset_0 = tst_cg_group_rm(cg_cpuset_0);
>
> Kind regards,
> Petr