[LTP] [PATCH 1/3] API/cgroup: Add safe_cgroup_lines_scanf

[LTP] [PATCH 1/3] API/cgroup: Add safe_cgroup_lines_scanf

[Richard Palethorpe via ltp]

Similar to file_lines_scanf. Allows us to read a particular key-value
pair from a controller file. Can replace kselftest's cg_read_key_*
when converting tests.

Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
---
 include/tst_cgroup.h | 12 +++++++++++-
 lib/tst_cgroup.c     | 33 +++++++++++++++++++++++++++++++++
 2 files changed, 44 insertions(+), 1 deletion(-)

diff --git a/include/tst_cgroup.h b/include/tst_cgroup.h
index 632050e86..561216296 100644
--- a/include/tst_cgroup.h
+++ b/include/tst_cgroup.h
@@ -188,9 +188,19 @@ void safe_cgroup_printf(const char *const file, const int lineno,
 void safe_cgroup_scanf(const char *file, const int lineno,
 		       const struct tst_cgroup_group *const cg,
 		       const char *const file_name,
-		       const char *fmt, ...)
+		       const char *const fmt, ...)
 		       __attribute__ ((format (scanf, 5, 6), nonnull));
 
+#define SAFE_CGROUP_LINES_SCANF(cg, file_name, fmt, ...)		\
+	safe_cgroup_lines_scanf(__FILE__, __LINE__,			\
+				(cg), (file_name), (fmt), __VA_ARGS__)
+
+void safe_cgroup_lines_scanf(const char *const file, const int lineno,
+			     const struct tst_cgroup_group *const cg,
+			     const char *const file_name,
+			     const char *const fmt, ...)
+			__attribute__ ((format (scanf, 5, 6), nonnull));
+
 #define SAFE_CGROUP_OCCURSIN(cg, file_name, needle)		\
 	safe_cgroup_occursin(__FILE__, __LINE__,		\
 			     (cg), (file_name), (needle))
diff --git a/lib/tst_cgroup.c b/lib/tst_cgroup.c
index c08ff2f20..961596256 100644
--- a/lib/tst_cgroup.c
+++ b/lib/tst_cgroup.c
@@ -1161,6 +1161,39 @@ void safe_cgroup_scanf(const char *const file, const int lineno,
 		 file_name, buf, fmt, ret, conv_cnt);
 }
 
+void safe_cgroup_lines_scanf(const char *const file, const int lineno,
+			     const struct tst_cgroup_group *const cg,
+			     const char *const file_name,
+			     const char *const fmt, ...)
+{
+	va_list va;
+	char buf[BUFSIZ];
+	ssize_t len = safe_cgroup_read(file, lineno,
+				       cg, file_name, buf, sizeof(buf));
+	const int conv_cnt = tst_count_scanf_conversions(fmt);
+	int ret = 0;
+	char *line;
+
+	if (len < 1)
+		return;
+
+	line = strtok(buf, "\n");
+	while (line && ret != conv_cnt) {
+		va_start(va, fmt);
+		ret = vsscanf(line, fmt, va);
+		va_end(va);
+
+		line = strtok(NULL, "\n");
+	}
+
+	if (conv_cnt == ret)
+		return;
+
+	tst_brk_(file, lineno, TBROK,
+		 "'%s': vsscanf('%s', '%s', ..): Less conversions than expected: %d != %d",
+		 file_name, buf, fmt, ret, conv_cnt);
+}
+
 int safe_cgroup_occursin(const char *const file, const int lineno,
 			 const struct tst_cgroup_group *const cg,
 			 const char *const file_name,
-- 
2.34.0


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

[LTP] [PATCH 2/3] API/cgroup: Add memory.stat

[Richard Palethorpe via ltp]

Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
---
 lib/tst_cgroup.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lib/tst_cgroup.c b/lib/tst_cgroup.c
index 961596256..cda1d39f7 100644
--- a/lib/tst_cgroup.c
+++ b/lib/tst_cgroup.c
@@ -153,6 +153,7 @@ static const struct cgroup_file cgroup_ctrl_files[] = {
 static const struct cgroup_file memory_ctrl_files[] = {
 	{ "memory.current", "memory.usage_in_bytes", CTRL_MEMORY },
 	{ "memory.max", "memory.limit_in_bytes", CTRL_MEMORY },
+	{ "memory.stat", "memory.stat", CTRL_MEMORY },
 	{ "memory.swappiness", "memory.swappiness", CTRL_MEMORY },
 	{ "memory.swap.current", "memory.memsw.usage_in_bytes", CTRL_MEMORY },
 	{ "memory.swap.max", "memory.memsw.limit_in_bytes", CTRL_MEMORY },
-- 
2.34.0


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

Re: [LTP] [PATCH 2/3] API/cgroup: Add memory.stat

[Cyril Hrubis]

Hi!
Reviewed-by: Cyril Hrubis <chrubis@suse.cz>

-- 
Cyril Hrubis
chrubis@suse.cz

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

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

[Richard Palethorpe via ltp]

Conversion of the second kself test.

Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
---
 runtest/controllers                           |   1 +
 testcases/kernel/controllers/memcg/.gitignore |   1 +
 .../kernel/controllers/memcg/memcontrol02.c   | 151 ++++++++++++++++++
 3 files changed, 153 insertions(+)
 create mode 100644 testcases/kernel/controllers/memcg/memcontrol02.c

diff --git a/runtest/controllers b/runtest/controllers
index 2b41a94d3..09e0107e4 100644
--- a/runtest/controllers
+++ b/runtest/controllers
@@ -18,6 +18,7 @@ memcg_control		memcg_control_test.sh
 
 # kselftest ports
 memcontrol01 memcontrol01
+memcontrol02 memcontrol02
 
 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);
+
+	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

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

[Cyril Hrubis]

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

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

[Richard Palethorpe]

Hello,

Cyril Hrubis <chrubis@suse.cz> writes:

>> +
>> +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.

Possibly it should just be TST_EXP(bool_expr, fmt, ...). That would be like
practically every other testing framework. OTOH LTP is somewhat special
as we usually are checking the return value of a syscall. So I should
probably leave these macros alone in this case.

>
> 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:

Frankly that is poor naming. One would expect tst_assert to be similar
to assert from assert.h.

>
> 	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.
>

I think it would be simpler to just create a general assert_expr
macro. The above function won't neatly handle loading multiple values
from multiple files. Nor will it handle transforming values.

We could implement SQL queries for sys files, like osquery, that would
be neat!

-- 
Thank you,
Richard.

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

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

[Cyril Hrubis]

Hi!
> > 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.
> 
> Possibly it should just be TST_EXP(bool_expr, fmt, ...). That would be like
> practically every other testing framework. OTOH LTP is somewhat special
> as we usually are checking the return value of a syscall. So I should
> probably leave these macros alone in this case.

Sounds reasonably.

> > 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:
> 
> Frankly that is poor naming. One would expect tst_assert to be similar
> to assert from assert.h.

Feel free to propose rename if you have a better idea.

> >
> > 	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.
> >
> 
> I think it would be simpler to just create a general assert_expr
> macro. The above function won't neatly handle loading multiple values
> from multiple files. Nor will it handle transforming values.

Yes, that is one of the limitations.

> We could implement SQL queries for sys files, like osquery, that would
> be neat!

Hmm, we do allready have boolean parser in LTP library, maybe we can
just reuse that, as long as we add a code that parses specified sysfs
files into variables that are passed to the parser it should work
reasonably well for us.

-- 
Cyril Hrubis
chrubis@suse.cz

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

Re: [LTP] [PATCH 1/3] API/cgroup: Add safe_cgroup_lines_scanf

[Cyril Hrubis]

Hi!
> +void safe_cgroup_lines_scanf(const char *const file, const int lineno,
> +			     const struct tst_cgroup_group *const cg,
> +			     const char *const file_name,
> +			     const char *const fmt, ...)
> +{
> +	va_list va;
> +	char buf[BUFSIZ];
> +	ssize_t len = safe_cgroup_read(file, lineno,
> +				       cg, file_name, buf, sizeof(buf));
> +	const int conv_cnt = tst_count_scanf_conversions(fmt);
> +	int ret = 0;
> +	char *line;
> +
> +	if (len < 1)
> +		return;
> +
> +	line = strtok(buf, "\n");

It may be safer to use strtok_r() other than that:

Reviewed-by: Cyril Hrubis <chrubis@suse.cz>

-- 
Cyril Hrubis
chrubis@suse.cz

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

[LTP] [PATCH v2 1/5] API/cgroup: Add safe_cgroup_lines_scanf

[Richard Palethorpe via ltp]

Similar to file_lines_scanf. Allows us to read a particular key-value
pair from a controller file. Can replace kselftest's cg_read_key_*
when converting tests.

Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
Reviewed-by: Cyril Hrubis <chrubis@suse.cz>
---

V2:
* Use all filesystems
* Add new EXP macro
* Switch to strtok_r
* Increase the margin of error for exfat and ext234
* Write to the file instead of reading to populate the page cache
  (allows it to work on tmpfs)

 include/tst_cgroup.h | 12 +++++++++++-
 lib/tst_cgroup.c     | 33 +++++++++++++++++++++++++++++++++
 2 files changed, 44 insertions(+), 1 deletion(-)

diff --git a/include/tst_cgroup.h b/include/tst_cgroup.h
index 632050e86..561216296 100644
--- a/include/tst_cgroup.h
+++ b/include/tst_cgroup.h
@@ -188,9 +188,19 @@ void safe_cgroup_printf(const char *const file, const int lineno,
 void safe_cgroup_scanf(const char *file, const int lineno,
 		       const struct tst_cgroup_group *const cg,
 		       const char *const file_name,
-		       const char *fmt, ...)
+		       const char *const fmt, ...)
 		       __attribute__ ((format (scanf, 5, 6), nonnull));
 
+#define SAFE_CGROUP_LINES_SCANF(cg, file_name, fmt, ...)		\
+	safe_cgroup_lines_scanf(__FILE__, __LINE__,			\
+				(cg), (file_name), (fmt), __VA_ARGS__)
+
+void safe_cgroup_lines_scanf(const char *const file, const int lineno,
+			     const struct tst_cgroup_group *const cg,
+			     const char *const file_name,
+			     const char *const fmt, ...)
+			__attribute__ ((format (scanf, 5, 6), nonnull));
+
 #define SAFE_CGROUP_OCCURSIN(cg, file_name, needle)		\
 	safe_cgroup_occursin(__FILE__, __LINE__,		\
 			     (cg), (file_name), (needle))
diff --git a/lib/tst_cgroup.c b/lib/tst_cgroup.c
index c08ff2f20..c78f28112 100644
--- a/lib/tst_cgroup.c
+++ b/lib/tst_cgroup.c
@@ -1161,6 +1161,39 @@ void safe_cgroup_scanf(const char *const file, const int lineno,
 		 file_name, buf, fmt, ret, conv_cnt);
 }
 
+void safe_cgroup_lines_scanf(const char *const file, const int lineno,
+			     const struct tst_cgroup_group *const cg,
+			     const char *const file_name,
+			     const char *const fmt, ...)
+{
+	va_list va;
+	char buf[BUFSIZ];
+	ssize_t len = safe_cgroup_read(file, lineno,
+				       cg, file_name, buf, sizeof(buf));
+	const int conv_cnt = tst_count_scanf_conversions(fmt);
+	int ret = 0;
+	char *line, *buf_ptr;
+
+	if (len < 1)
+		return;
+
+	line = strtok_r(buf, "\n", &buf_ptr);
+	while (line && ret != conv_cnt) {
+		va_start(va, fmt);
+		ret = vsscanf(line, fmt, va);
+		va_end(va);
+
+		line = strtok_r(NULL, "\n", &buf_ptr);
+	}
+
+	if (conv_cnt == ret)
+		return;
+
+	tst_brk_(file, lineno, TBROK,
+		 "'%s': vsscanf('%s', '%s', ..): Less conversions than expected: %d != %d",
+		 file_name, buf, fmt, ret, conv_cnt);
+}
+
 int safe_cgroup_occursin(const char *const file, const int lineno,
 			 const struct tst_cgroup_group *const cg,
 			 const char *const file_name,
-- 
2.34.0


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

[LTP] [PATCH v2 2/5] API/cgroup: Add memory.stat

[Richard Palethorpe via ltp]

Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
Reviewed-by: Cyril Hrubis <chrubis@suse.cz>
---
 lib/tst_cgroup.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lib/tst_cgroup.c b/lib/tst_cgroup.c
index c78f28112..2ef599d9e 100644
--- a/lib/tst_cgroup.c
+++ b/lib/tst_cgroup.c
@@ -153,6 +153,7 @@ static const struct cgroup_file cgroup_ctrl_files[] = {
 static const struct cgroup_file memory_ctrl_files[] = {
 	{ "memory.current", "memory.usage_in_bytes", CTRL_MEMORY },
 	{ "memory.max", "memory.limit_in_bytes", CTRL_MEMORY },
+	{ "memory.stat", "memory.stat", CTRL_MEMORY },
 	{ "memory.swappiness", "memory.swappiness", CTRL_MEMORY },
 	{ "memory.swap.current", "memory.memsw.usage_in_bytes", CTRL_MEMORY },
 	{ "memory.swap.max", "memory.memsw.limit_in_bytes", CTRL_MEMORY },
-- 
2.34.0


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

[LTP] [PATCH v2 3/5] API/fs: Add exfat magic

[Richard Palethorpe via ltp]

Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
---
 include/tst_fs.h  | 1 +
 lib/tst_fs_type.c | 2 ++
 2 files changed, 3 insertions(+)

diff --git a/include/tst_fs.h b/include/tst_fs.h
index 36d4b46f0..efcdff608 100644
--- a/include/tst_fs.h
+++ b/include/tst_fs.h
@@ -30,6 +30,7 @@
 #define TST_EXOFS_MAGIC    0x5DF5
 #define TST_OVERLAYFS_MAGIC 0x794c7630
 #define TST_FUSE_MAGIC     0x65735546
+#define TST_EXFAT_MAGIC    0x2011BAB0UL
 
 enum {
 	TST_BYTES = 1,
diff --git a/lib/tst_fs_type.c b/lib/tst_fs_type.c
index 8475f4c78..9de80224b 100644
--- a/lib/tst_fs_type.c
+++ b/lib/tst_fs_type.c
@@ -86,6 +86,8 @@ const char *tst_fs_type_name(long f_type)
 		return "overlayfs";
 	case TST_FUSE_MAGIC:
 		return "fuse";
+	case TST_EXFAT_MAGIC:
+		return "exfat";
 	default:
 		return "unknown";
 	}
-- 
2.34.0


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

[LTP] [PATCH v2 4/5] API: Add TST_EXP_EXPR macro

[Richard Palethorpe via ltp]

Simplifies evaluating an arbitrary expression or statement which is
not a syscall.

Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
---
 include/tst_test_macros.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/tst_test_macros.h b/include/tst_test_macros.h
index 5dea1aabd..ec8c38523 100644
--- a/include/tst_test_macros.h
+++ b/include/tst_test_macros.h
@@ -195,4 +195,7 @@ extern void *TST_RET_PTR;
 
 #define TST_EXP_FAIL2(SCALL, ERRNO, ...) TST_EXP_FAIL_(TST_RET >= 0, SCALL, #SCALL, ERRNO, ##__VA_ARGS__)
 
+#define TST_EXP_EXPR(EXPR, FMT, ...)						\
+	tst_res_(__FILE__, __LINE__, (EXPR) ? TPASS : TFAIL, "Expect: " FMT, ##__VA_ARGS__);
+
 #endif	/* TST_TEST_MACROS_H__ */
-- 
2.34.0


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

[LTP] [PATCH v2 5/5] cgroup: Add memcontrol02

[Richard Palethorpe via ltp]

This test appears to compare the overall "current" counter with anon
and file (page cache) memory counters. The file test assumes much more
memory will be consumed by the page cache. This is certainly true on
XFS and BTRFS where little or no memory outside the page cache is
used. However on ext4, ntfs and especially exfat, we can see more
memory being used.

This seems to be related to fs/buffer.c and buffer_head usage. exfat
in particular allocates a lot of memory. This is possibly due to the
buffer_head being allocated in cont_write_begin:

 Children      Self       Samples         bytes_alloc  Parent symbol
 ........  ........  ............  ..................  .................

    97.66%    97.66%        102401                 168  exfat_write_begin
            |
            ---0xf7eec549
               entry_SYSENTER_compat_after_hwframe
               do_fast_syscall_32
               __noinstr_text_start
               ksys_write
               vfs_write
               new_sync_write
               generic_file_write_iter
               __generic_file_write_iter
               generic_perform_write
               exfat_write_begin
               cont_write_begin
               __block_write_begin_int
               create_page_buffers
               create_empty_buffers
               alloc_page_buffers
               alloc_buffer_head
               kmem_cache_alloc
               kmem_cache_alloc

     0.20%     0.20%           205                 584  exfat_write_begin
     0.06%     0.06%            64                 168  exfat_fill_super
     0.05%     0.05%            49                 256  exfat_fill_super
     0.00%     0.00%             4                 584  exfat_fill_super
     0.00%     0.00%             1                 256  exfat_create
     0.00%     0.00%             1                1528  exfat_create
     0.00%     0.00%             1                1528  exfat_fill_super
     0.00%     0.00%             1                 312  exfat_fill_super

We can see (using slub_debug=U,buffer_head) that these buffer_head
objects are not freed until the file is unlinked:

 Outputting '/sys/kernel/debug/slab/buffer_head/alloc_traces' ...
 102480 alloc_buffer_head+0x1b/0x90 age=0/268/2629529 pid=248-430 cpus=0,2

 Outputting '/sys/kernel/debug/slab/buffer_head/free_traces' ...
 102400 <not-available> age=4297543659 pid=0 cpus=0
     80 free_buffer_head+0x21/0x60 age=147/266217/2629538 pid=337-427 cpus=1-2

ext4 and ntfs also use some of the "buffer" code, but don't seem to
allocate quite as much. Although ext4 begins to fail when slub debug
is enabled due to the extra memory debugging uses.

In any case, it appears that the CGroup code is correct, so I have
increased the error margin when exfat or ext234 is detected.

Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
---
 runtest/controllers                           |   1 +
 testcases/kernel/controllers/memcg/.gitignore |   1 +
 .../kernel/controllers/memcg/memcontrol02.c   | 182 ++++++++++++++++++
 3 files changed, 184 insertions(+)
 create mode 100644 testcases/kernel/controllers/memcg/memcontrol02.c

diff --git a/runtest/controllers b/runtest/controllers
index 2b41a94d3..09e0107e4 100644
--- a/runtest/controllers
+++ b/runtest/controllers
@@ -18,6 +18,7 @@ memcg_control		memcg_control_test.sh
 
 # kselftest ports
 memcontrol01 memcontrol01
+memcontrol02 memcontrol02
 
 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..a6d2f7268
--- /dev/null
+++ b/testcases/kernel/controllers/memcg/memcontrol02.c
@@ -0,0 +1,182 @@
+// SPDX-License-Identifier: GPL-2.0
+/*\
+ *
+ * [Description]
+ *
+ * Conversion of second kself test in cgroup/test_memcontrol.c.
+ *
+ * Original description:
+ * "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 differs from the
+ * kselftest in the following ways:
+ *
+ * . It supports V1.
+ * . It writes instead of reads to fill the page cache. Because no
+ *   pages were allocated on tmpfs.
+ * . It runs on most filesystems available
+ * . On EXFAT and extN we change the margine of error between all and file
+ *   memory to 50%. Because these allocate non-page-cache memory during writes.
+ */
+#define _GNU_SOURCE
+
+#include <stdlib.h>
+#include <stdio.h>
+
+#include "tst_test.h"
+#include "tst_cgroup.h"
+
+#define TMPDIR "mntdir"
+#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;
+static int file_to_all_error = 10;
+
+/*
+ * 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_EXPR(current >= size,
+		     "(memory.current=%zd) >= (size=%zd)", current, size);
+
+	SAFE_CGROUP_LINES_SCANF(cg_child, "memory.stat", anon_key_fmt, &anon);
+
+	TST_EXP_EXPR(anon > 0, "(memory.stat.anon=%zd) > 0", anon);
+	TST_EXP_EXPR(values_close(size, current, 3),
+		     "(size=%zd) ~= (memory.stat.anon=%zd)", size, current);
+	TST_EXP_EXPR(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];
+	size_t i;
+
+	for (i = 0; i < size; i += sizeof(buf))
+		SAFE_WRITE(1, 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";
+
+	TEST(open(TMPDIR"/tmpfile", O_RDWR | O_CREAT));
+
+	if (TST_RET < 0) {
+		if (TST_ERR == EOPNOTSUPP)
+			tst_brk(TCONF, "O_TMPFILE not supported by FS");
+
+		tst_brk(TBROK | TTERRNO,
+			"open(%s, O_TMPFILE | O_RDWR | O_EXCL", TMPDIR"/.");
+	}
+	fd = TST_RET;
+
+	SAFE_CGROUP_SCANF(cg_child, "memory.current", "%zu", &current);
+	tst_res(TINFO, "Created temp file: memory.current=%zu", current);
+
+	alloc_pagecache(fd, size);
+
+	SAFE_CGROUP_SCANF(cg_child, "memory.current", "%zu", &current);
+	TST_EXP_EXPR(current >= size,
+			 "(memory.current=%zu) >= (size=%zu)", current, size);
+
+	SAFE_CGROUP_LINES_SCANF(cg_child, "memory.stat", file_key_fmt, &file);
+	TST_EXP_EXPR(file > 0, "(memory.stat.file=%zd) > 0", file);
+
+	TST_EXP_EXPR(values_close(file, current, file_to_all_error),
+			 "(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_EXPR(current == 0, "(current=%zu) == 0", current);
+
+	if (!SAFE_FORK()) {
+		SAFE_CGROUP_PRINTF(cg_child, "cgroup.procs", "%d", getpid());
+
+		SAFE_CGROUP_SCANF(cg_child, "memory.current", "%zu", &current);
+		tst_res(TINFO, "Added proc to memcg: memory.current=%zu",
+			current);
+
+		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;
+
+	switch (tst_fs_type(TMPDIR)) {
+	case TST_EXFAT_MAGIC:
+	case TST_EXT234_MAGIC:
+		file_to_all_error = 50;
+		break;
+	}
+}
+
+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,
+	.mount_device = 1,
+	.dev_min_size = 256,
+	.mntpoint = TMPDIR,
+	.all_filesystems = 1,
+	.forks_child = 1,
+};
-- 
2.34.0


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

Re: [LTP] [PATCH v2 5/5] cgroup: Add memcontrol02

[Petr Vorel]

Hi Richie,

> +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";
> +
> +	TEST(open(TMPDIR"/tmpfile", O_RDWR | O_CREAT));
FYI open() needs to have also mode:
TEST(open(TMPDIR"/tmpfile", O_RDWR | O_CREAT, 0600));

To fix error:

Although man open(2) suggest it could work:

The mode argument must be supplied if O_CREAT or O_TMPFILE is specified in flags; if it is not supplied, some arbitrary bytes from the stack will be applied as the file mode.

<fcntl.h> in glibc actually check for it and require it:

In function ‘open’,
    inlined from ‘alloc_pagecache_50M_check’ at memcontrol02.c:92:2,
    inlined from ‘test_memcg_current’ at memcontrol02.c:140:4:
/usr/include/bits/fcntl2.h:50:11: error: call to ‘__open_missing_mode’ declared with attribute error: open with O_CREAT or O_TMPFILE in second argument needs 3 arguments
   50 |           __open_missing_mode ();

Kind regards,
Petr

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

Re: [LTP] [PATCH v2 1/5] API/cgroup: Add safe_cgroup_lines_scanf

[Petr Vorel]

Hi Richie,

Reviewed-by: Petr Vorel <pvorel@suse.cz>

Kind regards,
Petr

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