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

[Cyril Hrubis <chrubis@suse.cz>]

Hi!
> diff --git a/testcases/kernel/mem/.gitignore b/testcases/kernel/mem/.gitignore
> index d88484fa1..0845297cb 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/cpuset02.c b/testcases/kernel/mem/cpuset/cpuset02.c
> new file mode 100644
> index 000000000..05ed5c791
> --- /dev/null
> +++ b/testcases/kernel/mem/cpuset/cpuset02.c
> @@ -0,0 +1,147 @@
> +// 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 is reimplementing the test6 of cpuset_memory_testset.sh

This is _WRONG_.

You have to describe what the test does here, not where it came from.

> + */
> +
> +#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"
> +
> +#if HAVE_NUMA_H
> +#include <numa.h>
> +#endif
> +#if HAVE_NUMAIF_H
> +#include <numaif.h>
> +#endif
> +
> +#include "tst_safe_stdio.h"
> +#include "mem.h"
> +#include "numa_helper.h"
> +
> +#define MNTPOINT "hugetlbfs/"
> +#define CPUSET "cpuset_mnt"
> +
> +#ifdef HAVE_NUMA_V2
> +
> +static long hpage_size;
> +static int ncpus;
> +static int *nodes;
> +static int nnodes;
> +
> +static void count_cpus_mems(void)
> +{
> +	while (path_exist(PATH_SYS_SYSTEM "/cpu/cpu%d", ncpus))
> +		ncpus++;
> +	if (get_allowed_nodes_arr(NH_MEMS | NH_CPUS, &nnodes, &nodes) < 0)
> +		tst_brk(TBROK | TERRNO, "get_allowed_nodes_arr");
> +	if (nnodes <= 1)
> +		tst_brk(TCONF, "requires a NUMA system.");
> +}

You must use the tst_get_nodemap() instead which also allows you to
specify how much free memory is required for each NUMA node.

> +static void run_test(void)
> +{
> +	char path[256];
> +	char tmp_path[256];
> +
> +	snprintf(path, sizeof(path), "./%s/0", CPUSET);
> +	if (access(path, F_OK) != 0)
> +		SAFE_MKDIR(path, 0777);
> +	snprintf(tmp_path, sizeof(tmp_path), "./%s/cpuset.cpus", path);
> +	SAFE_FILE_PRINTF(tmp_path, "%s", "0");
> +	snprintf(tmp_path, sizeof(tmp_path), "./%s/cpuset.mems", path);
> +	SAFE_FILE_PRINTF(tmp_path, "%s", "0");
> +	snprintf(tmp_path, sizeof(tmp_path), "./%s/cpuset.sched_load_balance", path);
> +	SAFE_FILE_PRINTF(tmp_path, "%s", "0");
> +	SAFE_FILE_PRINTF("/proc/sys/vm/nr_hugepages", "%d", 2 * nnodes);
> +
> +	int pid;
> +	char str_hpage_size[20];
> +
> +	snprintf(str_hpage_size, sizeof(str_hpage_size), "%ld", hpage_size);
> +	snprintf(tmp_path, sizeof(tmp_path), "%shugepagefile", MNTPOINT);
> +
> +	char *argv[10] = {
> +		"--mmap-file",
> +		"--hugepage",
> +		"-s",
> +		str_hpage_size,
> +		"-f",
> +		tmp_path,
> +		NULL,
> +	};
> +
> +	int fd = SAFE_OPEN("memory_result", O_WRONLY | O_CREAT | O_TRUNC, 0644);
> +
> +	SAFE_DUP2(fd, STDOUT_FILENO);
> +	pid = tst_run_script("cpuset_memory_test", argv);
> +	close(fd);
> +
> +	sleep(1);
> +	snprintf(tmp_path, sizeof(tmp_path), "./%s/tasks", path);
> +	SAFE_FILE_PRINTF(tmp_path, "%d", pid);
> +	kill(pid, SIGUSR1);
> +	sleep(1);
> +	kill(pid, SIGUSR1);
> +	sleep(1);
> +	kill(pid, SIGINT);
> +	SAFE_WAITPID(pid, NULL, 0);
> +
> +	char node[20];
> +	FILE *file;
> +
> +	file  = SAFE_FOPEN("memory_result", "r");
> +
> +	if (fgets(node, sizeof(node), file) == NULL)
> +		tst_res(TFAIL, "read memory_result failed");
> +
> +	fclose(file);

The whole point of the rewrite is to get rid of this mess where we
propagate the test results in files.

The code that allocates the memory has to be in this test and use proper
synchronization between processes, i.e. checkpoints. This would also
avoid the need to propagate test results because the forked child could
report results directly.

> +	TST_EXP_PASS(strncmp(node, "0", 1));
> +}
> +
> +static void setup(void)
> +{
> +	count_cpus_mems();
> +
> +	hpage_size = SAFE_READ_MEMINFO(MEMINFO_HPAGE_SIZE)*1024;
> +	SAFE_MKDIR(CPUSET, 0777);
> +	SAFE_MOUNT("cpuset", CPUSET, "cgroup", 0, "cpuset");

You have to use the .needs_cgroup_ctrls in tst_test instead.

> +}
> +
> +static void cleanup(void)
> +{
> +	SAFE_UMOUNT(CPUSET);
> +}
> +
> +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},
> +	.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