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

[Cyril Hrubis <chrubis@suse.cz>]

Hi!
> 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 },
>  	{ }
>  };
>  
> 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..f43d8e98a
> --- /dev/null
> +++ b/testcases/kernel/mem/cpuset/cpuset02.c
> @@ -0,0 +1,139 @@
> +// 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>
> + */
> +
> +/*\
> + * Test checks cpuset.mems works with hugepage file.
> + */
> +
> +#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>
> +#include <numaif.h>
> +#include <errno.h>
> +#include "tst_numa.h"
> +#include "tst_safe_stdio.h"
> +#include "numa_helper.h"
> +
> +#define MNTPOINT "hugetlbfs/"
> +#define HUGE_PAGE_FILE MNTPOINT "hugepagefile"
> +#define PAGES_ALLOCATED 16u
> +
> +static long hpage_size;
> +static struct tst_nodemap *node;
> +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 == 0)
> +		tst_res(TPASS, "check node pass");
> +	else
> +		tst_res(TFAIL, "check node failed");
> +}
> +
> +static void child(void)
> +{
> +	char *p;
> +	int fd_hugepage;
> +
> +	TST_CHECKPOINT_WAIT(0);
> +
> +	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);
> +
> +	TST_CHECKPOINT_WAKE(1);
> +}
> +
> +static void run_test(void)
> +{
> +	int pid;
> +
> +	cg_cpuset_0 = tst_cg_group_mk(tst_cg, "0");
> +
> +	SAFE_CG_PRINT(cg_cpuset_0, "cpuset.cpus", "0");
> +	SAFE_CG_PRINT(cg_cpuset_0, "cpuset.mems", "0");
> +	SAFE_CG_PRINT(cg_cpuset_0, "cpuset.sched_load_balance", "0");

Here you are locking the process to node0 but that does not necessarily
means that the node0 has enough memory or CPUs. You have to choose a
node from the nodemap that you got in the setup.

Also if you want to have a node with CPU, so that you can lock the
process there, you need to add TST_NUMA_CPU flag to the
tst_get_nodemap(). And yes, there are machines that can have node with
just memory or just CPU.

However in the touch_memory_and_check_node() we just check a node the
memory was allocated on. So I do not think that we need to lock the
process on a particular CPU. And I do not think that we need the se the
sched_load_balance either.

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

What is this needed for and why is there the 2*node->cnt useful for? As
far as I can tell, we allocate a single page on a given node.

> +	pid = SAFE_FORK();
> +
> +	if (!pid) {
> +		child();
> +		return;
> +	}
> +
> +	SAFE_CG_PRINTF(cg_cpuset_0, "cgroup.procs", "%d", pid);

We can just get rid of all the checkpoints if this is done by the child
process before it calls the child() function.

> +	TST_CHECKPOINT_WAKE(0);
> +	TST_CHECKPOINT_WAIT(1);
> +
> +	SAFE_WAITPID(pid, NULL, 0);

We should not wait processes if we are not interested in their return
value. Such children are collected automatically by the test library.

> +	cg_cpuset_0 = tst_cg_group_rm(cg_cpuset_0);
> +}
> +
> +static void setup(void)
> +{
> +	node = tst_get_nodemap(TST_NUMA_MEM, PAGES_ALLOCATED * getpagesize() / 1024);
                                               ^
					       Why is this here? The
					       child in the test
					       allocates just a single
					       page.
> +	if (node->cnt <= 1)
> +		tst_brk(TCONF, "test requires NUMA system");
> +
> +	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},
> +		{}
> +	},
> +};
> +
> +#else
> +TST_TEST_TCONF(NUMA_ERROR_MSG);
> +#endif
> -- 
> 2.35.3
> 
> 
> -- 
> Mailing list info: https://lists.linux.it/listinfo/ltp

-- 
Cyril Hrubis
chrubis@suse.cz

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