Re: [LTP] [PATCH 3/3] cgroup: Add memcontrol02

[Cyril Hrubis <chrubis@suse.cz>]

Hi!
>  cgroup_fj_function_debug cgroup_fj_function.sh debug
>  cgroup_fj_function_cpuset cgroup_fj_function.sh cpuset
> diff --git a/testcases/kernel/controllers/memcg/.gitignore b/testcases/kernel/controllers/memcg/.gitignore
> index c3565f85c..f7de40d53 100644
> --- a/testcases/kernel/controllers/memcg/.gitignore
> +++ b/testcases/kernel/controllers/memcg/.gitignore
> @@ -6,3 +6,4 @@
>  /regression/memcg_test_4
>  /stress/memcg_process_stress
>  memcontrol01
> +memcontrol02
> diff --git a/testcases/kernel/controllers/memcg/memcontrol02.c b/testcases/kernel/controllers/memcg/memcontrol02.c
> new file mode 100644
> index 000000000..002637db0
> --- /dev/null
> +++ b/testcases/kernel/controllers/memcg/memcontrol02.c
> @@ -0,0 +1,151 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*\
> + *
> + * [Description]
> + *
> + * Conversion of second kself test in cgroup/test_memcontrol.c.
> + *
> + * This test creates a memory cgroup, allocates some anonymous memory
> + * and some pagecache and check memory.current and some memory.stat
> + * values.
> + *
> + * Note that the V1 rss and cache counters were renamed to anon and
> + * file in V2. Besides error reporting, this test mainly differs from
> + * the kselftest in that it supports V1.
> + */
> +#define _GNU_SOURCE
> +
> +#include <stdlib.h>
> +#include <stdio.h>
> +
> +#include "tst_test.h"
> +#include "tst_cgroup.h"
> +
> +#define MB(x) (x << 20)
> +
> +static size_t page_size;
> +static const struct tst_cgroup_group *cg_test;
> +static int is_v1_memcg;
> +static struct tst_cgroup_group *cg_child;
> +static int fd;
> +
> +/*
> + * Checks if two given values differ by less than err% of their sum.
> + */
> +static inline int values_close(const ssize_t a,
> +			       const ssize_t b,
> +			       const ssize_t err)
> +{
> +	return labs(a - b) <= (a + b) / 100 * err;
> +}
> +
> +static void alloc_anon_50M_check(void)
> +{
> +	const ssize_t size = MB(50);
> +	char *buf, *ptr;
> +	ssize_t anon, current;
> +	const char *const anon_key_fmt = is_v1_memcg ? "rss %zd" : "anon %zd";
> +
> +	buf = SAFE_MALLOC(size);
> +	for (ptr = buf; ptr < buf + size; ptr += page_size)
> +		*ptr = 0;
> +
> +	SAFE_CGROUP_SCANF(cg_child, "memory.current", "%zd", &current);
> +	TST_EXP_POSITIVE(current >= size,
> +			 "(memory.current=%zd) >= (size=%zd)", current, size);
> +
> +	SAFE_CGROUP_LINES_SCANF(cg_child, "memory.stat", anon_key_fmt, &anon);
> +
> +	TST_EXP_POSITIVE(anon > 0, "(memory.stat.anon=%zd) > 0", anon);
> +
> +	TST_EXP_POSITIVE(values_close(size, current, 3),
> +			 "(size=%zd) ~= (memory.stat.anon=%zd)", size, current);
> +	TST_EXP_POSITIVE(values_close(anon, current, 3),
> +			 "(memory.current=%zd) ~= (memory.stat.anon=%zd)",
> +			 current, anon);
> +}
> +
> +static void alloc_pagecache(const int fd, size_t size)
> +{
> +	char buf[BUFSIZ];
> +	struct stat st;
> +	size_t i;
> +
> +	SAFE_FSTAT(fd, &st);
> +	size += st.st_size;
> +
> +	SAFE_FTRUNCATE(fd, size);
> +
> +	for (i = 0; i < size; i += sizeof(buf))
> +		SAFE_READ(0, fd, buf, sizeof(buf));
> +}
> +
> +static void alloc_pagecache_50M_check(void)
> +{
> +	const size_t size = MB(50);
> +	size_t current, file;
> +	const char *const file_key_fmt = is_v1_memcg ? "cache %zd" : "file %zd";
> +
> +	fd = SAFE_OPEN("page_cache", O_RDWR | O_CREAT);
> +	alloc_pagecache(fd, size);
> +
> +	SAFE_CGROUP_SCANF(cg_child, "memory.current", "%zu", &current);
> +	TST_EXP_POSITIVE(current >= size,
> +			 "(memory.current=%zu) >= (size=%zu)", current, size);
> +
> +	SAFE_CGROUP_LINES_SCANF(cg_child, "memory.stat", file_key_fmt, &file);
> +	TST_EXP_POSITIVE(file > 0, "(memory.stat.file=%zd) > 0", file);
> +
> +	TST_EXP_POSITIVE(values_close(file, current, 10),
> +			 "(memory.current=%zd) ~= (memory.stat.file=%zd)",
> +			 current, file);
> +
> +	SAFE_CLOSE(fd);
> +}
> +
> +static void test_memcg_current(unsigned int n)
> +{
> +	size_t current;
> +
> +	cg_child = tst_cgroup_group_mk(cg_test, "child");
> +	SAFE_CGROUP_SCANF(cg_child, "memory.current", "%zu", &current);
> +	TST_EXP_POSITIVE(current == 0, "(current=%zu) == 0", current);

I find these TST_EXP_POSITIVE() macros slightly confusing.

Note that we do have TST_EXP_VAL(), so this can be better defined as
TST_EXP_VAL(current, 0);

But even then all the macros all written in a way that they do expect
a syscall as a first parameter and the messages are not clear.

Maybe we need a different solution. We already have tst_assert_foo()
functions for sysfs/proc files so maybe we need something as compare
function for the cgroup file attributes:

	enum cmp {
		CMP_EQ,
		CMP_NE,
		CMP_LT,
		CMP_GT,
		CMP_LE,
		CMP_GE,
		CMP_EPS,
	};

	CGROUP_ASSERT_CMP_SIZE(cg_child, "memory.current", CMP_EQ, 0);

	CGROUP_ASSERT_CMP_SIZE(cg_child, "memory.current", CMP_EPS, file, 10);


	or even simple macro that would compare two values accordingly
	to the OP and print PASS/FAIL would be better than this.

> +	if (!SAFE_FORK()) {
> +		SAFE_CGROUP_PRINTF(cg_child, "cgroup.procs", "%d", getpid());
> +		if (!n)
> +			alloc_anon_50M_check();
> +		else
> +			alloc_pagecache_50M_check();
> +	} else {
> +		tst_reap_children();
> +		cg_child = tst_cgroup_group_rm(cg_child);
> +	}
> +}
> +
> +static void setup(void)
> +{
> +	page_size = SAFE_SYSCONF(_SC_PAGESIZE);
> +
> +	tst_cgroup_require("memory", NULL);
> +	cg_test = tst_cgroup_get_test_group();
> +
> +	is_v1_memcg = TST_CGROUP_VER(cg_test, "memory") == TST_CGROUP_V1;
> +}
> +
> +static void cleanup(void)
> +{
> +	if (cg_child)
> +		cg_child = tst_cgroup_group_rm(cg_child);
> +
> +	tst_cgroup_cleanup();
> +}
> +
> +static struct tst_test test = {
> +	.setup = setup,
> +	.cleanup = cleanup,
> +	.tcnt = 2,
> +	.test = test_memcg_current,
> +	.needs_tmpdir = 1,
> +	.forks_child = 1,
> +};
> -- 
> 2.34.0
> 
> 
> -- 
> Mailing list info: https://lists.linux.it/listinfo/ltp

-- 
Cyril Hrubis
chrubis@suse.cz

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