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

[Wei Gao via ltp <ltp@lists.linux.it>]

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

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