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

[Petr Vorel <pvorel@suse.cz>]

Hi Wei,

...
> +++ b/testcases/kernel/mem/cpuset/cpuset02.c
> @@ -0,0 +1,132 @@
> +// 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.

very nit: I would add the author name:

 * Based on test6 from cpuset_memory_testset.sh written by Miao Xie.

(We don't need to keep the copyright, but because we likely sooner or later
delete cpuset_memory_testset.sh it's nice to give the original author his credit
for his original idea.)

> + */
> +
> +#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 <numa.h>
<numa.h> is not needed, please delete it.

> +#include <numaif.h>
> +#include <errno.h>
<errno.h> is not needed either, please delete it.

> +#include "tst_numa.h"
> +#include "tst_safe_stdio.h"
tst_safe_stdio.h is not needed either, please delete it.

> +#include "numa_helper.h"

numa_helper.h is not needed either, please delete it.

...
> +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);
> +	SAFE_FILE_PRINTF("/proc/sys/vm/nr_hugepages", "%d", 1);

You changed the /proc/sys/vm/nr_hugepages to 1, because Cyril objected the code
in v6:

SAFE_FILE_PRINTF("/proc/sys/vm/nr_hugepages", "%d", 2 * node->cnt);

But as I note there [1], the original shell test did it this way and kernel docs
allows more than 1 to allocate. I'm obviously missing something.

[1] https://lore.kernel.org/ltp/20250325133611.GB372417@pevik/

> +
> +	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 NUMA system");
This fails on system with single NUMA memory node. Either it should be compared as:
	if (node->cnt < 1)

or you're checking for 2 NUMA memory nodes (IMHO you want just single ATM.

Also maybe worth to change to "test requires at least one NUMA memory node" as
other tests do, you do have check for NUMA itsef anyway in
TST_TEST_TCONF(NUMA_ERROR_MSG).

> +
> +	check_node_id = node->map[0];
> +
> +	hpage_size = SAFE_READ_MEMINFO(MEMINFO_HPAGE_SIZE)*1024;
> +}
> +
> +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,
> +	.mntpoint = MNTPOINT,
> +	.needs_hugetlbfs = 1,
> +	.setup = setup,
> +	.forks_child = 1,
> +	.cleanup = cleanup,
> +	.test_all = run_test,
> +	.hugepages = {3, TST_NEEDS},
> +	.needs_checkpoints = 1,
> +	.needs_cgroup_ver = TST_CG_V1,
> +	.needs_cgroup_ctrls = (const char *const []){ "cpuset", NULL },
> +	.save_restore = (const struct tst_path_val[]) {
> +		{"/proc/sys/vm/nr_hugepages", NULL, TST_SR_TBROK},
Shouldn't here be rather TST_SR_TCONF?

Thanks for the changelog, the rest of the changes LGTM.

Kind regards,
Petr

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