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

[Petr Vorel <pvorel@suse.cz>]

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.

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