[LTP] [PATCH 1/2] Add TST_SPIN_TEST() macro

[LTP] [PATCH 1/2] Add TST_SPIN_TEST() macro

[Martin Doucha]

The TST_RETRY_FUNC() macro requires a single return value that'll be considered
success. This cannot be used with system calls that e.g. return a new file
descriptor because the success value is somewhat unpredictable.

Add a new macro TST_SPIN_TEST that'll work mostly like TST_RETRY_FUNC(), except:
- Any negative return value means failure, any non-negative return value means
  success.
- The loop will fall through on timeout instead of callid tst_brk(). TST_RET
  and TST_ERR will be set to the values returned by the last FUNC call.

Signed-off-by: Martin Doucha <mdoucha@suse.cz>
---
 include/tst_common.h | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/include/tst_common.h b/include/tst_common.h
index a0c06a3f7..72e00ca81 100644
--- a/include/tst_common.h
+++ b/include/tst_common.h
@@ -55,6 +55,34 @@
 	ERET;								\
 })
 
+/**
+ * TST_SPIN_TEST() - Repeatedly retry a function with an increasing delay.
+ * @FUNC - The function which will be retried
+ *
+ * Same as TST_RETRY_FUNC() but any non-negative return value is accepted
+ * as success and tst_brk() will not be called on timeout.
+ */
+#define TST_SPIN_TEST(FUNC) \
+	TST_SPIN_TEST_EXP_BACKOFF(FUNC, 1, -1)
+
+#define TST_SPIN_TEST_EXP_BACKOFF(FUNC, MAX_DELAY, GOOD_ERRNO)	\
+({	unsigned int tst_delay_, tst_max_delay_;			\
+	tst_delay_ = 1;							\
+	tst_max_delay_ = tst_multiply_timeout(MAX_DELAY * 1000000);	\
+	for (;;) {							\
+		TEST(FUNC);						\
+		if (TST_RET >= 0 || (GOOD_ERRNO >= 0 && TST_ERR == GOOD_ERRNO))	\
+			break;						\
+		if (tst_delay_ < tst_max_delay_) {			\
+			usleep(tst_delay_);				\
+			tst_delay_ *= 2;				\
+		} else {						\
+			break;						\
+		}							\
+	}								\
+	TST_RET;							\
+})
+
 #define TST_BRK_SUPPORTS_ONLY_TCONF_TBROK(condition) \
 	do { ((void)sizeof(char[1 - 2 * !!(condition)])); } while (0)
 
-- 
2.24.1

[LTP] [PATCH 2/2] Fix BPF test program loading issues

[Martin Doucha]

BPF programs require locked memory which will be released asynchronously.
If a BPF program gets loaded too many times too quickly, memory allocation
may fail due to race condition with asynchronous cleanup.

Wrap the bpf() test calls in a retry loop to fix the issue.

Signed-off-by: Martin Doucha <mdoucha@suse.cz>
---
 testcases/kernel/syscalls/bpf/Makefile     |  3 +
 testcases/kernel/syscalls/bpf/bpf_common.c | 89 ++++++++++++++++++++
 testcases/kernel/syscalls/bpf/bpf_common.h | 42 ++--------
 testcases/kernel/syscalls/bpf/bpf_prog01.c | 97 +++++++---------------
 testcases/kernel/syscalls/bpf/bpf_prog02.c | 28 +------
 testcases/kernel/syscalls/bpf/bpf_prog03.c | 20 ++---
 6 files changed, 136 insertions(+), 143 deletions(-)
 create mode 100644 testcases/kernel/syscalls/bpf/bpf_common.c

diff --git a/testcases/kernel/syscalls/bpf/Makefile b/testcases/kernel/syscalls/bpf/Makefile
index 990c8c83c..2241bce9b 100644
--- a/testcases/kernel/syscalls/bpf/Makefile
+++ b/testcases/kernel/syscalls/bpf/Makefile
@@ -5,6 +5,9 @@ top_srcdir		?= ../../../..
 
 include $(top_srcdir)/include/mk/testcases.mk
 
+FILTER_OUT_MAKE_TARGETS	:= bpf_common
 CFLAGS			+= -D_GNU_SOURCE
 
 include $(top_srcdir)/include/mk/generic_leaf_target.mk
+
+$(MAKE_TARGETS): %: %.o bpf_common.o
diff --git a/testcases/kernel/syscalls/bpf/bpf_common.c b/testcases/kernel/syscalls/bpf/bpf_common.c
new file mode 100644
index 000000000..8e61b3a74
--- /dev/null
+++ b/testcases/kernel/syscalls/bpf/bpf_common.c
@@ -0,0 +1,89 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * Copyright (c) 2019-2020 Linux Test Project
+ */
+
+#define TST_NO_DEFAULT_MAIN
+#include "tst_test.h"
+#include "lapi/bpf.h"
+#include "bpf_common.h"
+
+void rlimit_bump_memlock(void)
+{
+	struct rlimit memlock_r;
+
+	SAFE_GETRLIMIT(RLIMIT_MEMLOCK, &memlock_r);
+	memlock_r.rlim_cur += BPF_MEMLOCK_ADD;
+	tst_res(TINFO, "Raising RLIMIT_MEMLOCK to %ld",
+		(long)memlock_r.rlim_cur);
+
+	if (memlock_r.rlim_cur <= memlock_r.rlim_max) {
+		SAFE_SETRLIMIT(RLIMIT_MEMLOCK, &memlock_r);
+	} else if ((geteuid() == 0)) {
+		memlock_r.rlim_max += BPF_MEMLOCK_ADD;
+		SAFE_SETRLIMIT(RLIMIT_MEMLOCK, &memlock_r);
+	} else {
+		tst_res(TINFO, "Can't raise RLIMIT_MEMLOCK, test may fail "
+			"due to lack of max locked memory");
+	}
+}
+
+int bpf_map_create(union bpf_attr *attr)
+{
+	TST_SPIN_TEST(bpf(BPF_MAP_CREATE, attr, sizeof(*attr)));
+	if (TST_RET == -1) {
+		if (TST_ERR == EPERM) {
+			tst_res(TCONF, "Hint: check also /proc/sys/kernel/unprivileged_bpf_disabled");
+			tst_brk(TCONF | TTERRNO,
+				"bpf() requires CAP_SYS_ADMIN on this system");
+		} else {
+			tst_brk(TBROK | TTERRNO, "Failed to create array map");
+		}
+	}
+
+	return TST_RET;
+}
+
+void prepare_bpf_prog_attr(union bpf_attr *attr, const struct bpf_insn *prog,
+	size_t prog_size, char *log_buf, size_t log_size)
+{
+	static struct bpf_insn *buf;
+	static size_t buf_size;
+	size_t prog_len = prog_size / sizeof(*prog);
+
+	/* all guarded buffers will be free()d automatically by LTP library */
+	if (!buf || prog_size > buf_size) {
+		buf = tst_alloc(prog_size);
+		buf_size = prog_size;
+	}
+
+	memcpy(buf, prog, prog_size);
+	memset(attr, 0, sizeof(*attr));
+	attr->prog_type = BPF_PROG_TYPE_SOCKET_FILTER;
+	attr->insns = ptr_to_u64(buf);
+	attr->insn_cnt = prog_len;
+	attr->license = ptr_to_u64("GPL");
+	attr->log_buf = ptr_to_u64(log_buf);
+	attr->log_size = log_size;
+	attr->log_level = 1;
+}
+
+int load_bpf_prog(union bpf_attr *attr, const char *log)
+{
+	TST_SPIN_TEST(bpf(BPF_PROG_LOAD, attr, sizeof(*attr)));
+
+	if (TST_RET >= 0) {
+		tst_res(TPASS, "Loaded program");
+	} else if (TST_RET == -1) {
+		if (log[0] != 0) {
+			tst_brk(TBROK | TTERRNO, "Failed verification: %s",
+				log);
+		} else {
+			tst_brk(TBROK | TTERRNO, "Failed to load program");
+		}
+	} else {
+		tst_brk(TBROK, "Invalid bpf() return value: %ld", TST_RET);
+	}
+
+	return TST_RET;
+}
diff --git a/testcases/kernel/syscalls/bpf/bpf_common.h b/testcases/kernel/syscalls/bpf/bpf_common.h
index f700bede2..fadb7b75a 100644
--- a/testcases/kernel/syscalls/bpf/bpf_common.h
+++ b/testcases/kernel/syscalls/bpf/bpf_common.h
@@ -1,6 +1,6 @@
 /* SPDX-License-Identifier: GPL-2.0-or-later */
 /*
- * Copyright (c) 2019 Linux Test Project
+ * Copyright (c) 2019-2020 Linux Test Project
  */
 
 #ifndef LTP_BPF_COMMON_H
@@ -8,40 +8,10 @@
 
 #define BPF_MEMLOCK_ADD (256*1024)
 
-void rlimit_bump_memlock(void)
-{
-	struct rlimit memlock_r;
-
-	SAFE_GETRLIMIT(RLIMIT_MEMLOCK, &memlock_r);
-	memlock_r.rlim_cur += BPF_MEMLOCK_ADD;
-	tst_res(TINFO, "Raising RLIMIT_MEMLOCK to %ld",
-		(long)memlock_r.rlim_cur);
-
-	if (memlock_r.rlim_cur <= memlock_r.rlim_max) {
-		SAFE_SETRLIMIT(RLIMIT_MEMLOCK, &memlock_r);
-	} else if ((geteuid() == 0)) {
-		memlock_r.rlim_max += BPF_MEMLOCK_ADD;
-		SAFE_SETRLIMIT(RLIMIT_MEMLOCK, &memlock_r);
-	} else {
-		tst_res(TINFO, "Can't raise RLIMIT_MEMLOCK, test may fail "
-			"due to lack of max locked memory");
-	}
-}
-
-int bpf_map_create(union bpf_attr *attr)
-{
-	TEST(bpf(BPF_MAP_CREATE, attr, sizeof(*attr)));
-	if (TST_RET == -1) {
-		if (TST_ERR == EPERM) {
-			tst_res(TCONF, "Hint: check also /proc/sys/kernel/unprivileged_bpf_disabled");
-			tst_brk(TCONF | TTERRNO,
-				"bpf() requires CAP_SYS_ADMIN on this system");
-		} else {
-			tst_brk(TBROK | TTERRNO, "Failed to create array map");
-		}
-	}
-
-	return TST_RET;
-}
+void rlimit_bump_memlock(void);
+int bpf_map_create(union bpf_attr *attr);
+void prepare_bpf_prog_attr(union bpf_attr *attr, const struct bpf_insn *prog,
+	size_t prog_size, char *log_buf, size_t log_size);
+int load_bpf_prog(union bpf_attr *attr, const char *log);
 
 #endif
diff --git a/testcases/kernel/syscalls/bpf/bpf_prog01.c b/testcases/kernel/syscalls/bpf/bpf_prog01.c
index 46a909fe2..70645c408 100644
--- a/testcases/kernel/syscalls/bpf/bpf_prog01.c
+++ b/testcases/kernel/syscalls/bpf/bpf_prog01.c
@@ -32,72 +32,47 @@
 const char MSG[] = "Ahoj!";
 static char *msg;
 
-/*
- * The following is a byte code template. We copy it to a guarded buffer and
- * substitute the runtime value of our map file descriptor.
- *
- * r0 - r10 = registers 0 to 10
- * r0 = return code
- * r1 - r5 = scratch registers, used for function arguments
- * r6 - r9 = registers preserved across function calls
- * fp/r10 = stack frame pointer
- */
-const struct bpf_insn PROG[] = {
-	/* Load the map FD into r1 (place holder) */
-	BPF_LD_MAP_FD(BPF_REG_1, 0),
-	/* Put (key = 0) on stack and key ptr into r2 */
-	BPF_MOV64_REG(BPF_REG_2, BPF_REG_10),   /* r2 = fp */
-	BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8),  /* r2 = r2 - 8 */
-	BPF_ST_MEM(BPF_DW, BPF_REG_2, 0, 0),    /* *r2 = 0 */
-	/* r0 = bpf_map_lookup_elem(r1, r2) */
-	BPF_EMIT_CALL(BPF_FUNC_map_lookup_elem),
-	/* if r0 == 0 goto exit */
-	BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 3),
-	/* Set map[0] = 1 */
-	BPF_MOV64_REG(BPF_REG_1, BPF_REG_0),     /* r1 = r0 */
-	BPF_ST_MEM(BPF_DW, BPF_REG_1, 0, 1),     /* *r1 = 1 */
-	BPF_MOV64_IMM(BPF_REG_0, 0),             /* r0 = 0 */
-	BPF_EXIT_INSN(),		         /* return r0 */
-};
-
-static struct bpf_insn *prog;
 static char *log;
 static union bpf_attr *attr;
 
 int load_prog(int fd)
 {
-	prog[0] = BPF_LD_MAP_FD(BPF_REG_1, fd);
-
-	memset(attr, 0, sizeof(*attr));
-	attr->prog_type = BPF_PROG_TYPE_SOCKET_FILTER;
-	attr->insns = ptr_to_u64(prog);
-	attr->insn_cnt = ARRAY_SIZE(PROG);
-	attr->license = ptr_to_u64("GPL");
-	attr->log_buf = ptr_to_u64(log);
-	attr->log_size = BUFSIZ;
-	attr->log_level = 1;
-
-	TEST(bpf(BPF_PROG_LOAD, attr, sizeof(*attr)));
-	if (TST_RET == -1) {
-		if (log[0] != 0) {
-			tst_brk(TFAIL | TTERRNO,
-				"Failed verification: %s",
-				log);
-		} else {
-			tst_brk(TFAIL | TTERRNO, "Failed to load program");
-		}
-	} else {
-		tst_res(TPASS, "Loaded program");
-	}
-
-	return TST_RET;
+	/*
+	 * The following is a byte code template. We copy it to a guarded buffer and
+	 * substitute the runtime value of our map file descriptor.
+	 *
+	 * r0 - r10 = registers 0 to 10
+	 * r0 = return code
+	 * r1 - r5 = scratch registers, used for function arguments
+	 * r6 - r9 = registers preserved across function calls
+	 * fp/r10 = stack frame pointer
+	 */
+	struct bpf_insn PROG[] = {
+		/* Load the map FD into r1 (place holder) */
+		BPF_LD_MAP_FD(BPF_REG_1, fd),
+		/* Put (key = 0) on stack and key ptr into r2 */
+		BPF_MOV64_REG(BPF_REG_2, BPF_REG_10),   /* r2 = fp */
+		BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8),  /* r2 = r2 - 8 */
+		BPF_ST_MEM(BPF_DW, BPF_REG_2, 0, 0),    /* *r2 = 0 */
+		/* r0 = bpf_map_lookup_elem(r1, r2) */
+		BPF_EMIT_CALL(BPF_FUNC_map_lookup_elem),
+		/* if r0 == 0 goto exit */
+		BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 3),
+		/* Set map[0] = 1 */
+		BPF_MOV64_REG(BPF_REG_1, BPF_REG_0),     /* r1 = r0 */
+		BPF_ST_MEM(BPF_DW, BPF_REG_1, 0, 1),     /* *r1 = 1 */
+		BPF_MOV64_IMM(BPF_REG_0, 0),             /* r0 = 0 */
+		BPF_EXIT_INSN(),		         /* return r0 */
+	};
+
+	prepare_bpf_prog_attr(attr, PROG, sizeof(PROG), log, BUFSIZ);
+	return load_bpf_prog(attr, log);
 }
 
 void setup(void)
 {
 	rlimit_bump_memlock();
 
-	memcpy(prog, PROG, sizeof(PROG));
 	memcpy(msg, MSG, sizeof(MSG));
 }
 
@@ -114,16 +89,7 @@ void run(void)
 	attr->value_size = 8;
 	attr->max_entries = 1;
 
-	TEST(bpf(BPF_MAP_CREATE, attr, sizeof(*attr)));
-	if (TST_RET == -1) {
-		if (TST_ERR == EPERM) {
-			tst_brk(TCONF | TTERRNO,
-				"bpf() requires CAP_SYS_ADMIN on this system");
-		} else {
-			tst_brk(TBROK | TTERRNO, "Failed to create array map");
-		}
-	}
-	map_fd = TST_RET;
+	map_fd = bpf_map_create(attr);
 
 	prog_fd = load_prog(map_fd);
 
@@ -161,7 +127,6 @@ static struct tst_test test = {
 	.min_kver = "3.19",
 	.bufs = (struct tst_buffers []) {
 		{&log, .size = BUFSIZ},
-		{&prog, .size = sizeof(PROG)},
 		{&attr, .size = sizeof(*attr)},
 		{&msg, .size = sizeof(MSG)},
 		{},
diff --git a/testcases/kernel/syscalls/bpf/bpf_prog02.c b/testcases/kernel/syscalls/bpf/bpf_prog02.c
index acff1884a..eb783ce3e 100644
--- a/testcases/kernel/syscalls/bpf/bpf_prog02.c
+++ b/testcases/kernel/syscalls/bpf/bpf_prog02.c
@@ -37,7 +37,6 @@ static union bpf_attr *attr;
 
 static int load_prog(int fd)
 {
-	static struct bpf_insn *prog;
 	struct bpf_insn insn[] = {
 		BPF_MOV64_IMM(BPF_REG_6, 1),            /* 0: r6 = 1 */
 
@@ -67,31 +66,8 @@ static int load_prog(int fd)
 		BPF_EXIT_INSN(),		        /* 26: return r0 */
 	};
 
-	if (!prog)
-		prog = tst_alloc(sizeof(insn));
-	memcpy(prog, insn, sizeof(insn));
-
-	memset(attr, 0, sizeof(*attr));
-	attr->prog_type = BPF_PROG_TYPE_SOCKET_FILTER;
-	attr->insns = ptr_to_u64(prog);
-	attr->insn_cnt = ARRAY_SIZE(insn);
-	attr->license = ptr_to_u64("GPL");
-	attr->log_buf = ptr_to_u64(log);
-	attr->log_size = BUFSIZ;
-	attr->log_level = 1;
-
-	TEST(bpf(BPF_PROG_LOAD, attr, sizeof(*attr)));
-	if (TST_RET == -1) {
-		if (log[0] != 0) {
-			tst_res(TINFO, "Verification log:");
-			fputs(log, stderr);
-			tst_brk(TBROK | TTERRNO, "Failed verification");
-		} else {
-			tst_brk(TBROK | TTERRNO, "Failed to load program");
-		}
-	}
-
-	return TST_RET;
+	prepare_bpf_prog_attr(attr, insn, sizeof(insn), log, BUFSIZ);
+	return load_bpf_prog(attr, log);
 }
 
 static void setup(void)
diff --git a/testcases/kernel/syscalls/bpf/bpf_prog03.c b/testcases/kernel/syscalls/bpf/bpf_prog03.c
index d79815961..3dd9a174d 100644
--- a/testcases/kernel/syscalls/bpf/bpf_prog03.c
+++ b/testcases/kernel/syscalls/bpf/bpf_prog03.c
@@ -42,7 +42,6 @@ static union bpf_attr *attr;
 
 static int load_prog(int fd)
 {
-	static struct bpf_insn *prog;
 	struct bpf_insn insn[] = {
 		BPF_LD_MAP_FD(BPF_REG_1, fd),
 
@@ -85,25 +84,16 @@ static int load_prog(int fd)
 		BPF_EXIT_INSN()
 	};
 
-	if (!prog)
-		prog = tst_alloc(sizeof(insn));
-	memcpy(prog, insn, sizeof(insn));
-
-	memset(attr, 0, sizeof(*attr));
-	attr->prog_type = BPF_PROG_TYPE_SOCKET_FILTER;
-	attr->insns = ptr_to_u64(prog);
-	attr->insn_cnt = ARRAY_SIZE(insn);
-	attr->license = ptr_to_u64("GPL");
-	attr->log_buf = ptr_to_u64(log);
-	attr->log_size = LOG_SIZE;
-	attr->log_level = 1;
-
-	TEST(bpf(BPF_PROG_LOAD, attr, sizeof(*attr)));
+	prepare_bpf_prog_attr(attr, insn, sizeof(insn), log, LOG_SIZE);
+	TST_SPIN_TEST_EXP_BACKOFF(bpf(BPF_PROG_LOAD, attr, sizeof(*attr)), 1,
+		EACCES);
 	if (TST_RET == -1) {
 		if (log[0] != 0)
 			tst_res(TPASS | TTERRNO, "Failed verification");
 		else
 			tst_brk(TBROK | TTERRNO, "Failed to load program");
+	} else if (TST_RET < 0) {
+		tst_brk(TBROK, "Invalid bpf() return value %ld", TST_RET);
 	} else {
 		tst_res(TINFO, "Verification log:");
 		fputs(log, stderr);
-- 
2.24.1

[LTP] [PATCH 2/2] Fix BPF test program loading issues

[Richard Palethorpe]

Hello,

This looks like a considerable improvement, but a few comments below.

Martin Doucha <mdoucha@suse.cz> writes:

> BPF programs require locked memory which will be released asynchronously.
> If a BPF program gets loaded too many times too quickly, memory allocation
> may fail due to race condition with asynchronous cleanup.
>
> Wrap the bpf() test calls in a retry loop to fix the issue.
>
> Signed-off-by: Martin Doucha <mdoucha@suse.cz>
> ---
>  testcases/kernel/syscalls/bpf/Makefile     |  3 +
>  testcases/kernel/syscalls/bpf/bpf_common.c | 89 ++++++++++++++++++++
>  testcases/kernel/syscalls/bpf/bpf_common.h | 42 ++--------
>  testcases/kernel/syscalls/bpf/bpf_prog01.c | 97 +++++++---------------
>  testcases/kernel/syscalls/bpf/bpf_prog02.c | 28 +------
>  testcases/kernel/syscalls/bpf/bpf_prog03.c | 20 ++---
>  6 files changed, 136 insertions(+), 143 deletions(-)
>  create mode 100644 testcases/kernel/syscalls/bpf/bpf_common.c
>
> diff --git a/testcases/kernel/syscalls/bpf/Makefile b/testcases/kernel/syscalls/bpf/Makefile
> index 990c8c83c..2241bce9b 100644
> --- a/testcases/kernel/syscalls/bpf/Makefile
> +++ b/testcases/kernel/syscalls/bpf/Makefile
> @@ -5,6 +5,9 @@ top_srcdir		?= ../../../..
>  
>  include $(top_srcdir)/include/mk/testcases.mk
>  
> +FILTER_OUT_MAKE_TARGETS	:= bpf_common
>  CFLAGS			+= -D_GNU_SOURCE
>  
>  include $(top_srcdir)/include/mk/generic_leaf_target.mk
> +
> +$(MAKE_TARGETS): %: %.o bpf_common.o
> diff --git a/testcases/kernel/syscalls/bpf/bpf_common.c b/testcases/kernel/syscalls/bpf/bpf_common.c
> new file mode 100644
> index 000000000..8e61b3a74
> --- /dev/null
> +++ b/testcases/kernel/syscalls/bpf/bpf_common.c
> @@ -0,0 +1,89 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (c) 2019-2020 Linux Test Project
> + */
> +
> +#define TST_NO_DEFAULT_MAIN
> +#include "tst_test.h"
> +#include "lapi/bpf.h"
> +#include "bpf_common.h"
> +
> +void rlimit_bump_memlock(void)
> +{
> +	struct rlimit memlock_r;
> +
> +	SAFE_GETRLIMIT(RLIMIT_MEMLOCK, &memlock_r);
> +	memlock_r.rlim_cur += BPF_MEMLOCK_ADD;
> +	tst_res(TINFO, "Raising RLIMIT_MEMLOCK to %ld",
> +		(long)memlock_r.rlim_cur);
> +
> +	if (memlock_r.rlim_cur <= memlock_r.rlim_max) {
> +		SAFE_SETRLIMIT(RLIMIT_MEMLOCK, &memlock_r);
> +	} else if ((geteuid() == 0)) {
> +		memlock_r.rlim_max += BPF_MEMLOCK_ADD;
> +		SAFE_SETRLIMIT(RLIMIT_MEMLOCK, &memlock_r);
> +	} else {
> +		tst_res(TINFO, "Can't raise RLIMIT_MEMLOCK, test may fail "
> +			"due to lack of max locked memory");
> +	}
> +}
> +
> +int bpf_map_create(union bpf_attr *attr)
> +{
> +	TST_SPIN_TEST(bpf(BPF_MAP_CREATE, attr, sizeof(*attr)));

We should use exponential backoff on all of these. Unless you have some
reason to think it will cause problems?

> +	if (TST_RET == -1) {
> +		if (TST_ERR == EPERM) {
> +			tst_res(TCONF, "Hint: check also /proc/sys/kernel/unprivileged_bpf_disabled");
> +			tst_brk(TCONF | TTERRNO,
> +				"bpf() requires CAP_SYS_ADMIN on this system");
> +		} else {
> +			tst_brk(TBROK | TTERRNO, "Failed to create array map");
> +		}
> +	}
> +
> +	return TST_RET;
> +}
> +
> +void prepare_bpf_prog_attr(union bpf_attr *attr, const struct
> bpf_insn *prog,

How about just init_bpf_prog_attr?

> +	size_t prog_size, char *log_buf, size_t log_size)
> +{
> +	static struct bpf_insn *buf;
> +	static size_t buf_size;
> +	size_t prog_len = prog_size / sizeof(*prog);
> +
> +	/* all guarded buffers will be free()d automatically by LTP library */
> +	if (!buf || prog_size > buf_size) {
> +		buf = tst_alloc(prog_size);
> +		buf_size = prog_size;
> +	}
> +
> +	memcpy(buf, prog, prog_size);
> +	memset(attr, 0, sizeof(*attr));
> +	attr->prog_type = BPF_PROG_TYPE_SOCKET_FILTER;
> +	attr->insns = ptr_to_u64(buf);
> +	attr->insn_cnt = prog_len;
> +	attr->license = ptr_to_u64("GPL");
> +	attr->log_buf = ptr_to_u64(log_buf);
> +	attr->log_size = log_size;
> +	attr->log_level = 1;
> +}
> +
> +int load_bpf_prog(union bpf_attr *attr, const char *log)

Probably best to always put bpf at the begining so that someone can
simply search "bpf_" to get all the library functions related to that.

> +{
> +	TST_SPIN_TEST(bpf(BPF_PROG_LOAD, attr, sizeof(*attr)));

Same here for exponential backoff.

-- 
Thank you,
Richard.

[LTP] [PATCH 2/2] Fix BPF test program loading issues

[Cyril Hrubis]

Hi!
> diff --git a/testcases/kernel/syscalls/bpf/bpf_common.c b/testcases/kernel/syscalls/bpf/bpf_common.c
> new file mode 100644
> index 000000000..8e61b3a74
> --- /dev/null
> +++ b/testcases/kernel/syscalls/bpf/bpf_common.c
> @@ -0,0 +1,89 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (c) 2019-2020 Linux Test Project
> + */
> +
> +#define TST_NO_DEFAULT_MAIN
> +#include "tst_test.h"
> +#include "lapi/bpf.h"
> +#include "bpf_common.h"
> +
> +void rlimit_bump_memlock(void)
> +{
> +	struct rlimit memlock_r;
> +
> +	SAFE_GETRLIMIT(RLIMIT_MEMLOCK, &memlock_r);
> +	memlock_r.rlim_cur += BPF_MEMLOCK_ADD;
> +	tst_res(TINFO, "Raising RLIMIT_MEMLOCK to %ld",
> +		(long)memlock_r.rlim_cur);
> +
> +	if (memlock_r.rlim_cur <= memlock_r.rlim_max) {
> +		SAFE_SETRLIMIT(RLIMIT_MEMLOCK, &memlock_r);
> +	} else if ((geteuid() == 0)) {
> +		memlock_r.rlim_max += BPF_MEMLOCK_ADD;
> +		SAFE_SETRLIMIT(RLIMIT_MEMLOCK, &memlock_r);
> +	} else {
> +		tst_res(TINFO, "Can't raise RLIMIT_MEMLOCK, test may fail "
> +			"due to lack of max locked memory");
> +	}
> +}
> +
> +int bpf_map_create(union bpf_attr *attr)
> +{
> +	TST_SPIN_TEST(bpf(BPF_MAP_CREATE, attr, sizeof(*attr)));
> +	if (TST_RET == -1) {
> +		if (TST_ERR == EPERM) {
> +			tst_res(TCONF, "Hint: check also /proc/sys/kernel/unprivileged_bpf_disabled");
> +			tst_brk(TCONF | TTERRNO,
> +				"bpf() requires CAP_SYS_ADMIN on this system");
> +		} else {
> +			tst_brk(TBROK | TTERRNO, "Failed to create array map");
> +		}
> +	}
> +
> +	return TST_RET;
> +}
> +
> +void prepare_bpf_prog_attr(union bpf_attr *attr, const struct bpf_insn *prog,
> +	size_t prog_size, char *log_buf, size_t log_size)
> +{
> +	static struct bpf_insn *buf;
> +	static size_t buf_size;
> +	size_t prog_len = prog_size / sizeof(*prog);
> +
> +	/* all guarded buffers will be free()d automatically by LTP library */
> +	if (!buf || prog_size > buf_size) {
> +		buf = tst_alloc(prog_size);
> +		buf_size = prog_size;
> +	}
> +
> +	memcpy(buf, prog, prog_size);
> +	memset(attr, 0, sizeof(*attr));
> +	attr->prog_type = BPF_PROG_TYPE_SOCKET_FILTER;
> +	attr->insns = ptr_to_u64(buf);
> +	attr->insn_cnt = prog_len;
> +	attr->license = ptr_to_u64("GPL");
> +	attr->log_buf = ptr_to_u64(log_buf);
> +	attr->log_size = log_size;
> +	attr->log_level = 1;
> +}
> +
> +int load_bpf_prog(union bpf_attr *attr, const char *log)
> +{
> +	TST_SPIN_TEST(bpf(BPF_PROG_LOAD, attr, sizeof(*attr)));
> +
> +	if (TST_RET >= 0) {
> +		tst_res(TPASS, "Loaded program");
> +	} else if (TST_RET == -1) {
> +		if (log[0] != 0) {
> +			tst_brk(TBROK | TTERRNO, "Failed verification: %s",
> +				log);
> +		} else {
> +			tst_brk(TBROK | TTERRNO, "Failed to load program");
> +		}

There is absolutely no need for else branches when we do tst_brk()

> +	} else {
> +		tst_brk(TBROK, "Invalid bpf() return value: %ld", TST_RET);
> +	}


This whole mess could be written easily as:

int bpf_load_prog(...)
{
	...

	if (TST_RET >= 0) {
		tst_res(TPASS, ...);
		return TST_RET;
	}

	if (log[0] != 0)
		tst_brk(TBROK | TERRNO, "Failed verification ...);

	tst_brk(TBROK | TERRNO, "Failed to load program bpf() = %ld", ret);
}

> +	return TST_RET;
> +}
> diff --git a/testcases/kernel/syscalls/bpf/bpf_common.h b/testcases/kernel/syscalls/bpf/bpf_common.h

Why can't we keep the code in the header? I do not condsider this to be
improving anything at all.

> index f700bede2..fadb7b75a 100644
> --- a/testcases/kernel/syscalls/bpf/bpf_common.h
> +++ b/testcases/kernel/syscalls/bpf/bpf_common.h
> @@ -1,6 +1,6 @@
>  /* SPDX-License-Identifier: GPL-2.0-or-later */
>  /*
> - * Copyright (c) 2019 Linux Test Project
> + * Copyright (c) 2019-2020 Linux Test Project
>   */
>  
>  #ifndef LTP_BPF_COMMON_H
> @@ -8,40 +8,10 @@
>  
>  #define BPF_MEMLOCK_ADD (256*1024)
>  
> -void rlimit_bump_memlock(void)
> -{
> -	struct rlimit memlock_r;
> -
> -	SAFE_GETRLIMIT(RLIMIT_MEMLOCK, &memlock_r);
> -	memlock_r.rlim_cur += BPF_MEMLOCK_ADD;
> -	tst_res(TINFO, "Raising RLIMIT_MEMLOCK to %ld",
> -		(long)memlock_r.rlim_cur);
> -
> -	if (memlock_r.rlim_cur <= memlock_r.rlim_max) {
> -		SAFE_SETRLIMIT(RLIMIT_MEMLOCK, &memlock_r);
> -	} else if ((geteuid() == 0)) {
> -		memlock_r.rlim_max += BPF_MEMLOCK_ADD;
> -		SAFE_SETRLIMIT(RLIMIT_MEMLOCK, &memlock_r);
> -	} else {
> -		tst_res(TINFO, "Can't raise RLIMIT_MEMLOCK, test may fail "
> -			"due to lack of max locked memory");
> -	}
> -}
> -
> -int bpf_map_create(union bpf_attr *attr)
> -{
> -	TEST(bpf(BPF_MAP_CREATE, attr, sizeof(*attr)));
> -	if (TST_RET == -1) {
> -		if (TST_ERR == EPERM) {
> -			tst_res(TCONF, "Hint: check also /proc/sys/kernel/unprivileged_bpf_disabled");
> -			tst_brk(TCONF | TTERRNO,
> -				"bpf() requires CAP_SYS_ADMIN on this system");
> -		} else {
> -			tst_brk(TBROK | TTERRNO, "Failed to create array map");
> -		}
> -	}
> -
> -	return TST_RET;
> -}
> +void rlimit_bump_memlock(void);
> +int bpf_map_create(union bpf_attr *attr);
> +void prepare_bpf_prog_attr(union bpf_attr *attr, const struct bpf_insn *prog,
> +	size_t prog_size, char *log_buf, size_t log_size);
> +int load_bpf_prog(union bpf_attr *attr, const char *log);
>  
>  #endif
> diff --git a/testcases/kernel/syscalls/bpf/bpf_prog01.c b/testcases/kernel/syscalls/bpf/bpf_prog01.c
> index 46a909fe2..70645c408 100644
> --- a/testcases/kernel/syscalls/bpf/bpf_prog01.c
> +++ b/testcases/kernel/syscalls/bpf/bpf_prog01.c
> @@ -32,72 +32,47 @@
>  const char MSG[] = "Ahoj!";
>  static char *msg;
>  
> -/*
> - * The following is a byte code template. We copy it to a guarded buffer and
> - * substitute the runtime value of our map file descriptor.
> - *
> - * r0 - r10 = registers 0 to 10
> - * r0 = return code
> - * r1 - r5 = scratch registers, used for function arguments
> - * r6 - r9 = registers preserved across function calls
> - * fp/r10 = stack frame pointer
> - */
> -const struct bpf_insn PROG[] = {
> -	/* Load the map FD into r1 (place holder) */
> -	BPF_LD_MAP_FD(BPF_REG_1, 0),
> -	/* Put (key = 0) on stack and key ptr into r2 */
> -	BPF_MOV64_REG(BPF_REG_2, BPF_REG_10),   /* r2 = fp */
> -	BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8),  /* r2 = r2 - 8 */
> -	BPF_ST_MEM(BPF_DW, BPF_REG_2, 0, 0),    /* *r2 = 0 */
> -	/* r0 = bpf_map_lookup_elem(r1, r2) */
> -	BPF_EMIT_CALL(BPF_FUNC_map_lookup_elem),
> -	/* if r0 == 0 goto exit */
> -	BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 3),
> -	/* Set map[0] = 1 */
> -	BPF_MOV64_REG(BPF_REG_1, BPF_REG_0),     /* r1 = r0 */
> -	BPF_ST_MEM(BPF_DW, BPF_REG_1, 0, 1),     /* *r1 = 1 */
> -	BPF_MOV64_IMM(BPF_REG_0, 0),             /* r0 = 0 */
> -	BPF_EXIT_INSN(),		         /* return r0 */
> -};
> -
> -static struct bpf_insn *prog;
>  static char *log;
>  static union bpf_attr *attr;
>  
>  int load_prog(int fd)
>  {
> -	prog[0] = BPF_LD_MAP_FD(BPF_REG_1, fd);
> -
> -	memset(attr, 0, sizeof(*attr));
> -	attr->prog_type = BPF_PROG_TYPE_SOCKET_FILTER;
> -	attr->insns = ptr_to_u64(prog);
> -	attr->insn_cnt = ARRAY_SIZE(PROG);
> -	attr->license = ptr_to_u64("GPL");
> -	attr->log_buf = ptr_to_u64(log);
> -	attr->log_size = BUFSIZ;
> -	attr->log_level = 1;
> -
> -	TEST(bpf(BPF_PROG_LOAD, attr, sizeof(*attr)));
> -	if (TST_RET == -1) {
> -		if (log[0] != 0) {
> -			tst_brk(TFAIL | TTERRNO,
> -				"Failed verification: %s",
> -				log);
> -		} else {
> -			tst_brk(TFAIL | TTERRNO, "Failed to load program");
> -		}
> -	} else {
> -		tst_res(TPASS, "Loaded program");
> -	}
> -
> -	return TST_RET;
> +	/*
> +	 * The following is a byte code template. We copy it to a guarded buffer and
> +	 * substitute the runtime value of our map file descriptor.
> +	 *
> +	 * r0 - r10 = registers 0 to 10
> +	 * r0 = return code
> +	 * r1 - r5 = scratch registers, used for function arguments
> +	 * r6 - r9 = registers preserved across function calls
> +	 * fp/r10 = stack frame pointer
> +	 */
> +	struct bpf_insn PROG[] = {
> +		/* Load the map FD into r1 (place holder) */
> +		BPF_LD_MAP_FD(BPF_REG_1, fd),
> +		/* Put (key = 0) on stack and key ptr into r2 */
> +		BPF_MOV64_REG(BPF_REG_2, BPF_REG_10),   /* r2 = fp */
> +		BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8),  /* r2 = r2 - 8 */
> +		BPF_ST_MEM(BPF_DW, BPF_REG_2, 0, 0),    /* *r2 = 0 */
> +		/* r0 = bpf_map_lookup_elem(r1, r2) */
> +		BPF_EMIT_CALL(BPF_FUNC_map_lookup_elem),
> +		/* if r0 == 0 goto exit */
> +		BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 3),
> +		/* Set map[0] = 1 */
> +		BPF_MOV64_REG(BPF_REG_1, BPF_REG_0),     /* r1 = r0 */
> +		BPF_ST_MEM(BPF_DW, BPF_REG_1, 0, 1),     /* *r1 = 1 */
> +		BPF_MOV64_IMM(BPF_REG_0, 0),             /* r0 = 0 */
> +		BPF_EXIT_INSN(),		         /* return r0 */
> +	};
> +
> +	prepare_bpf_prog_attr(attr, PROG, sizeof(PROG), log, BUFSIZ);
> +	return load_bpf_prog(attr, log);
>  }
>  
>  void setup(void)
>  {
>  	rlimit_bump_memlock();
>  
> -	memcpy(prog, PROG, sizeof(PROG));
>  	memcpy(msg, MSG, sizeof(MSG));
>  }
>  
> @@ -114,16 +89,7 @@ void run(void)
>  	attr->value_size = 8;
>  	attr->max_entries = 1;
>  
> -	TEST(bpf(BPF_MAP_CREATE, attr, sizeof(*attr)));
> -	if (TST_RET == -1) {
> -		if (TST_ERR == EPERM) {
> -			tst_brk(TCONF | TTERRNO,
> -				"bpf() requires CAP_SYS_ADMIN on this system");
> -		} else {
> -			tst_brk(TBROK | TTERRNO, "Failed to create array map");
> -		}
> -	}
> -	map_fd = TST_RET;
> +	map_fd = bpf_map_create(attr);
>  
>  	prog_fd = load_prog(map_fd);
>  
> @@ -161,7 +127,6 @@ static struct tst_test test = {
>  	.min_kver = "3.19",
>  	.bufs = (struct tst_buffers []) {
>  		{&log, .size = BUFSIZ},
> -		{&prog, .size = sizeof(PROG)},
>  		{&attr, .size = sizeof(*attr)},
>  		{&msg, .size = sizeof(MSG)},
>  		{},
> diff --git a/testcases/kernel/syscalls/bpf/bpf_prog02.c b/testcases/kernel/syscalls/bpf/bpf_prog02.c
> index acff1884a..eb783ce3e 100644
> --- a/testcases/kernel/syscalls/bpf/bpf_prog02.c
> +++ b/testcases/kernel/syscalls/bpf/bpf_prog02.c
> @@ -37,7 +37,6 @@ static union bpf_attr *attr;
>  
>  static int load_prog(int fd)
>  {
> -	static struct bpf_insn *prog;
>  	struct bpf_insn insn[] = {
>  		BPF_MOV64_IMM(BPF_REG_6, 1),            /* 0: r6 = 1 */
>  
> @@ -67,31 +66,8 @@ static int load_prog(int fd)
>  		BPF_EXIT_INSN(),		        /* 26: return r0 */
>  	};
>  
> -	if (!prog)
> -		prog = tst_alloc(sizeof(insn));
> -	memcpy(prog, insn, sizeof(insn));
> -
> -	memset(attr, 0, sizeof(*attr));
> -	attr->prog_type = BPF_PROG_TYPE_SOCKET_FILTER;
> -	attr->insns = ptr_to_u64(prog);
> -	attr->insn_cnt = ARRAY_SIZE(insn);
> -	attr->license = ptr_to_u64("GPL");
> -	attr->log_buf = ptr_to_u64(log);
> -	attr->log_size = BUFSIZ;
> -	attr->log_level = 1;
> -
> -	TEST(bpf(BPF_PROG_LOAD, attr, sizeof(*attr)));
> -	if (TST_RET == -1) {
> -		if (log[0] != 0) {
> -			tst_res(TINFO, "Verification log:");
> -			fputs(log, stderr);
> -			tst_brk(TBROK | TTERRNO, "Failed verification");
> -		} else {
> -			tst_brk(TBROK | TTERRNO, "Failed to load program");
> -		}
> -	}
> -
> -	return TST_RET;
> +	prepare_bpf_prog_attr(attr, insn, sizeof(insn), log, BUFSIZ);
> +	return load_bpf_prog(attr, log);
>  }
>  
>  static void setup(void)
> diff --git a/testcases/kernel/syscalls/bpf/bpf_prog03.c b/testcases/kernel/syscalls/bpf/bpf_prog03.c
> index d79815961..3dd9a174d 100644
> --- a/testcases/kernel/syscalls/bpf/bpf_prog03.c
> +++ b/testcases/kernel/syscalls/bpf/bpf_prog03.c
> @@ -42,7 +42,6 @@ static union bpf_attr *attr;
>  
>  static int load_prog(int fd)
>  {
> -	static struct bpf_insn *prog;
>  	struct bpf_insn insn[] = {
>  		BPF_LD_MAP_FD(BPF_REG_1, fd),
>  
> @@ -85,25 +84,16 @@ static int load_prog(int fd)
>  		BPF_EXIT_INSN()
>  	};
>  
> -	if (!prog)
> -		prog = tst_alloc(sizeof(insn));
> -	memcpy(prog, insn, sizeof(insn));
> -
> -	memset(attr, 0, sizeof(*attr));
> -	attr->prog_type = BPF_PROG_TYPE_SOCKET_FILTER;
> -	attr->insns = ptr_to_u64(prog);
> -	attr->insn_cnt = ARRAY_SIZE(insn);
> -	attr->license = ptr_to_u64("GPL");
> -	attr->log_buf = ptr_to_u64(log);
> -	attr->log_size = LOG_SIZE;
> -	attr->log_level = 1;
> -
> -	TEST(bpf(BPF_PROG_LOAD, attr, sizeof(*attr)));
> +	prepare_bpf_prog_attr(attr, insn, sizeof(insn), log, LOG_SIZE);
> +	TST_SPIN_TEST_EXP_BACKOFF(bpf(BPF_PROG_LOAD, attr, sizeof(*attr)), 1,
> +		EACCES);
>  	if (TST_RET == -1) {
>  		if (log[0] != 0)
>  			tst_res(TPASS | TTERRNO, "Failed verification");
>  		else
>  			tst_brk(TBROK | TTERRNO, "Failed to load program");
> +	} else if (TST_RET < 0) {
> +		tst_brk(TBROK, "Invalid bpf() return value %ld", TST_RET);
>  	} else {
>  		tst_res(TINFO, "Verification log:");
>  		fputs(log, stderr);
> -- 
> 2.24.1
> 
> 
> -- 
> Mailing list info: https://lists.linux.it/listinfo/ltp

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH 2/2] Fix BPF test program loading issues

[Martin Doucha]

On 2/5/20 3:31 PM, Cyril Hrubis wrote:
>> diff --git a/testcases/kernel/syscalls/bpf/bpf_common.h b/testcases/kernel/syscalls/bpf/bpf_common.h
> 
> Why can't we keep the code in the header? I do not condsider this to be
> improving anything at all.

Because executable code doesn't belong in header files unless necessary.
Header files are as much API documentation for developers as they are
code for compilers. And header files full of executable code make
terrible API documentation.

I'll implement the rest of your suggestions and resubmit.

-- 
Martin Doucha   mdoucha@suse.cz
QA Engineer for Software Maintenance
SUSE LINUX, s.r.o.
CORSO IIa
Krizikova 148/34
186 00 Prague 8
Czech Republic

[LTP] [PATCH 2/2] Fix BPF test program loading issues

[Cyril Hrubis]

Hi!
> > Why can't we keep the code in the header? I do not condsider this to be
> > improving anything at all.
> 
> Because executable code doesn't belong in header files unless necessary.
> Header files are as much API documentation for developers as they are
> code for compilers. And header files full of executable code make
> terrible API documentation.

That's only true if we are building and external interface for a
library, here we are just avoiding copy&paste by the simpliest means
available.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH 2/2] Fix BPF test program loading issues

[Martin Doucha]

On 2/5/20 4:50 PM, Cyril Hrubis wrote:
> That's only true if we are building and external interface for a
> library, here we are just avoiding copy&paste by the simpliest means
> available.

I am building external interface for a library. The library is called
bpf_common. But if you still disagree with splitting the executable code
into a separate file to make the header more readable for developers of
future BPF tests, I'll gladly unassign myself from this task and go work
on something else.

-- 
Martin Doucha   mdoucha@suse.cz
QA Engineer for Software Maintenance
SUSE LINUX, s.r.o.
CORSO IIa
Krizikova 148/34
186 00 Prague 8
Czech Republic

[LTP] [PATCH 2/2] Fix BPF test program loading issues

[Cyril Hrubis]

Hi!
> > That's only true if we are building and external interface for a
> > library, here we are just avoiding copy&paste by the simpliest means
> > available.
> 
> I am building external interface for a library. The library is called
> bpf_common. But if you still disagree with splitting the executable code
> into a separate file to make the header more readable for developers of
> future BPF tests, I'll gladly unassign myself from this task and go work
> on something else.

Can we please discuss things calmly and rationally? If you want to give
up on your patch that's fine, however if you want to continue to discuss
technical details, let's do it without emotions, okay?

Getting back to the technical point of the discussion, I still do not
consider that these three functions are complex enough to be split into
header and C source, but I do not have such strong opinion about that.

So if you really think that it should be separated like that at least
put the change that moves the code into a separate patch, since that is
unrelated change to introduction of the new function.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH 2/2] Fix BPF test program loading issues

[Richard Palethorpe]

Hello,

Cyril Hrubis <chrubis@suse.cz> writes:

> Hi!
>> > That's only true if we are building and external interface for a
>> > library, here we are just avoiding copy&paste by the simpliest means
>> > available.
>>
>> I am building external interface for a library. The library is called
>> bpf_common. But if you still disagree with splitting the executable code
>> into a separate file to make the header more readable for developers of
>> future BPF tests, I'll gladly unassign myself from this task and go work
>> on something else.
>
> Can we please discuss things calmly and rationally? If you want to give
> up on your patch that's fine, however if you want to continue to discuss
> technical details, let's do it without emotions, okay?

Honestly this is a style issue, so we can exchange one or two opinions,
but then just decide Cyril is right (because he has survived as
maintainer for X years with similar ideas about style) and move on to
things where the universe proves you right or wrong in the time it takes
to compile and run your code.

>
> Getting back to the technical point of the discussion, I still do not
> consider that these three functions are complex enough to be split into
> header and C source, but I do not have such strong opinion about that.
>
> So if you really think that it should be separated like that at least
> put the change that moves the code into a separate patch, since that is
> unrelated change to introduction of the new function.
>
> --
> Cyril Hrubis
> chrubis@suse.cz


--
Thank you,
Richard.

[LTP] [PATCH 2/2] Fix BPF test program loading issues

[Martin Doucha]

On 2/6/20 12:02 PM, Richard Palethorpe wrote:
> Hello,
> 
> Cyril Hrubis <chrubis@suse.cz> writes:
>> Can we please discuss things calmly and rationally? If you want to give
>> up on your patch that's fine, however if you want to continue to discuss
>> technical details, let's do it without emotions, okay?
> 
> Honestly this is a style issue, so we can exchange one or two opinions,
> but then just decide Cyril is right (because he has survived as
> maintainer for X years with similar ideas about style) and move on to
> things where the universe proves you right or wrong in the time it takes
> to compile and run your code.

Do I really need to remind you all that we've had a bug caused by this
exact lack of basic code hygiene right before the last release?

http://lists.linux.it/pipermail/ltp/2020-January/015099.html

-- 
Martin Doucha   mdoucha@suse.cz
QA Engineer for Software Maintenance
SUSE LINUX, s.r.o.
CORSO IIa
Krizikova 148/34
186 00 Prague 8
Czech Republic

[LTP] [PATCH 2/2] Fix BPF test program loading issues

[Richard Palethorpe]

Hi,

Martin Doucha <mdoucha@suse.cz> writes:

> On 2/6/20 12:02 PM, Richard Palethorpe wrote:
>> Hello,
>> 
>> Cyril Hrubis <chrubis@suse.cz> writes:
>>> Can we please discuss things calmly and rationally? If you want to give
>>> up on your patch that's fine, however if you want to continue to discuss
>>> technical details, let's do it without emotions, okay?
>> 
>> Honestly this is a style issue, so we can exchange one or two opinions,
>> but then just decide Cyril is right (because he has survived as
>> maintainer for X years with similar ideas about style) and move on to
>> things where the universe proves you right or wrong in the time it takes
>> to compile and run your code.
>
> Do I really need to remind you all that we've had a bug caused by this
> exact lack of basic code hygiene right before the last release?
>
> http://lists.linux.it/pipermail/ltp/2020-January/015099.html

Yes, how is this the exact same issue?

There is a big difference between showing a concrete example and
simply asserting something.

-- 
Thank you,
Richard.

[LTP] [PATCH 2/2] Fix BPF test program loading issues

[Richard Palethorpe]

Hi,

Richard Palethorpe <rpalethorpe@suse.de> writes:

> Hi,
>
> Martin Doucha <mdoucha@suse.cz> writes:
>
>> On 2/6/20 12:02 PM, Richard Palethorpe wrote:
>>> Hello,
>>>
>>> Cyril Hrubis <chrubis@suse.cz> writes:
>>>> Can we please discuss things calmly and rationally? If you want to give
>>>> up on your patch that's fine, however if you want to continue to discuss
>>>> technical details, let's do it without emotions, okay?
>>>
>>> Honestly this is a style issue, so we can exchange one or two opinions,
>>> but then just decide Cyril is right (because he has survived as
>>> maintainer for X years with similar ideas about style) and move on to
>>> things where the universe proves you right or wrong in the time it takes
>>> to compile and run your code.
>>
>> Do I really need to remind you all that we've had a bug caused by this
>> exact lack of basic code hygiene right before the last release?
>>
>> http://lists.linux.it/pipermail/ltp/2020-January/015099.html
>
> Yes, how is this the exact same issue?
>
> There is a big difference between showing a concrete example and
> simply asserting something.

OK, I see why it is, it prevents a class of bugs by avoiding "namespace
pollution". This is a concrete argument which has a history of causing
problems.

OTOH, At least with compilers which don't have link time optimisation
(everything except Clang?), there is an advantage to including stuff
inline. Hence why fzsync is entirely in the header, although I don't
think it matters much either way for LTP.

So perhaps it should be the default to seperate headers from
implementation...

I still just defer to Cyril though. He deals with most of these
problems...

--
Thank you,
Richard.

[LTP] [PATCH 2/2] Fix BPF test program loading issues

[Martin Doucha]

On 2/6/20 1:36 PM, Richard Palethorpe wrote:
> OTOH, At least with compilers which don't have link time optimisation
> (everything except Clang?), there is an advantage to including stuff
> inline. Hence why fzsync is entirely in the header, although I don't
> think it matters much either way for LTP.

GCC does have link-time optimisation.
CFLAGS += -flto
CXXFLAGS += -flto
LDFLAGS += -flto

The only drawback is that object files will get bigger and linking the
test programs will take significantly longer. But since the performance
overhead of actual LTP code is mostly trivial compared to time spent in
system calls and waiting for I/O operations, I don't think we'll gain
anything by turning these optimizations on.

-- 
Martin Doucha   mdoucha@suse.cz
QA Engineer for Software Maintenance
SUSE LINUX, s.r.o.
CORSO IIa
Krizikova 148/34
186 00 Prague 8
Czech Republic

[LTP] [PATCH 1/2] Add TST_SPIN_TEST() macro

[Richard Palethorpe]

Hi,

Martin Doucha <mdoucha@suse.cz> writes:

> The TST_RETRY_FUNC() macro requires a single return value that'll be considered
> success. This cannot be used with system calls that e.g. return a new file
> descriptor because the success value is somewhat unpredictable.
>
> Add a new macro TST_SPIN_TEST that'll work mostly like TST_RETRY_FUNC(), except:
> - Any negative return value means failure, any non-negative return value means
>   success.
> - The loop will fall through on timeout instead of callid tst_brk(). TST_RET
>   and TST_ERR will be set to the values returned by the last FUNC call.
>
> Signed-off-by: Martin Doucha <mdoucha@suse.cz>
> ---
>  include/tst_common.h | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
>
> diff --git a/include/tst_common.h b/include/tst_common.h
> index a0c06a3f7..72e00ca81 100644
> --- a/include/tst_common.h
> +++ b/include/tst_common.h
> @@ -55,6 +55,34 @@
>  	ERET;								\
>  })
>
> +/**
> + * TST_SPIN_TEST() - Repeatedly retry a function with an increasing delay.
> + * @FUNC - The function which will be retried
> + *
> + * Same as TST_RETRY_FUNC() but any non-negative return value is accepted
> + * as success and tst_brk() will not be called on timeout.
> + */
> +#define TST_SPIN_TEST(FUNC) \
> +	TST_SPIN_TEST_EXP_BACKOFF(FUNC, 1, -1)

Ah, so this is actually always exponential. Ignore my comment on other
patch.

> +
> +#define TST_SPIN_TEST_EXP_BACKOFF(FUNC, MAX_DELAY, GOOD_ERRNO)	\
> +({	unsigned int tst_delay_, tst_max_delay_;			\
> +	tst_delay_ = 1;							\
> +	tst_max_delay_ = tst_multiply_timeout(MAX_DELAY * 1000000);	\
> +	for (;;) {							\
> +		TEST(FUNC);						\
> +		if (TST_RET >= 0 || (GOOD_ERRNO >= 0 && TST_ERR == GOOD_ERRNO))	\
> +			break;						\
> +		if (tst_delay_ < tst_max_delay_) {			\
> +			usleep(tst_delay_);				\
> +			tst_delay_ *= 2;				\
> +		} else {						\
> +			break;						\
> +		}							\
> +	}								\
> +	TST_RET;							\
> +})
> +
>  #define TST_BRK_SUPPORTS_ONLY_TCONF_TBROK(condition) \
>  	do { ((void)sizeof(char[1 - 2 * !!(condition)])); } while (0)
>
> --
> 2.24.1


--
Thank you,
Richard.

[LTP] [PATCH 1/2] Add TST_SPIN_TEST() macro

[Cyril Hrubis]

Hi!
> Signed-off-by: Martin Doucha <mdoucha@suse.cz>
> ---
>  include/tst_common.h | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/include/tst_common.h b/include/tst_common.h
> index a0c06a3f7..72e00ca81 100644
> --- a/include/tst_common.h
> +++ b/include/tst_common.h
> @@ -55,6 +55,34 @@
>  	ERET;								\
>  })
>  
> +/**
> + * TST_SPIN_TEST() - Repeatedly retry a function with an increasing delay.
> + * @FUNC - The function which will be retried
> + *
> + * Same as TST_RETRY_FUNC() but any non-negative return value is accepted
> + * as success and tst_brk() will not be called on timeout.
> + */
> +#define TST_SPIN_TEST(FUNC) \
> +	TST_SPIN_TEST_EXP_BACKOFF(FUNC, 1, -1)
> +
> +#define TST_SPIN_TEST_EXP_BACKOFF(FUNC, MAX_DELAY, GOOD_ERRNO)	\
> +({	unsigned int tst_delay_, tst_max_delay_;			\
> +	tst_delay_ = 1;							\
> +	tst_max_delay_ = tst_multiply_timeout(MAX_DELAY * 1000000);	\
> +	for (;;) {							\
> +		TEST(FUNC);						\
> +		if (TST_RET >= 0 || (GOOD_ERRNO >= 0 && TST_ERR == GOOD_ERRNO))	\
> +			break;						\
> +		if (tst_delay_ < tst_max_delay_) {			\
> +			usleep(tst_delay_);				\
> +			tst_delay_ *= 2;				\
> +		} else {						\
> +			break;						\
> +		}							\
> +	}								\
> +	TST_RET;							\
> +})

This looks like we will end up adding more specialized variants over
time, I do wonder if we can make one generic implementation. It would
probably make more sense to pass a function that converts the FUNC
output into boolean instead.

Something as:

diff --git a/include/tst_common.h b/include/tst_common.h
index a0c06a3f7..b7c644d0d 100644
--- a/include/tst_common.h
+++ b/include/tst_common.h
@@ -34,25 +34,26 @@
  * (the total time sleeping will be approximately one second as well). When the
  * delay exceeds one second tst_brk() is called.
  */
-#define TST_RETRY_FUNC(FUNC, ERET) \
-	TST_RETRY_FN_EXP_BACKOFF(FUNC, ERET, 1)
+#define TST_RETRY_FUNC(FUNC, ECHCK) \
+	TST_RETRY_FN_EXP_BACKOFF(FUNC, ECHCK, 1)
 
-#define TST_RETRY_FN_EXP_BACKOFF(FUNC, ERET, MAX_DELAY)	\
+#define TST_RETRY_FN_EXP_BACKOFF(FUNC, ECHCK, MAX_DELAY)		\
 ({	unsigned int tst_delay_, tst_max_delay_;			\
 	tst_delay_ = 1;							\
 	tst_max_delay_ = tst_multiply_timeout(MAX_DELAY * 1000000);	\
+	typeof(FUNC) tst_ret_;                                          \
 	for (;;) {							\
-		typeof(FUNC) tst_ret_ = FUNC;				\
-		if (tst_ret_ == ERET)					\
+		tst_ret_ = FUNC;					\
+		if (ECHCK(tst_ret_))					\
 			break;						\
 		if (tst_delay_ < tst_max_delay_) {			\
 			usleep(tst_delay_);				\
 			tst_delay_ *= 2;				\
 		} else {						\
-			tst_brk(TBROK, #FUNC" timed out");		\
+			break;                                          \
 		}							\
 	}								\
-	ERET;								\
+	tst_ret_;							\
 })
 
 #define TST_BRK_SUPPORTS_ONLY_TCONF_TBROK(condition) \
diff --git a/testcases/kernel/syscalls/tgkill/tgkill03.c b/testcases/kernel/syscalls/tgkill/tgkill03.c
index 593a21726..a303d0c9c 100644
--- a/testcases/kernel/syscalls/tgkill/tgkill03.c
+++ b/testcases/kernel/syscalls/tgkill/tgkill03.c
@@ -39,11 +39,14 @@ static void *defunct_thread_func(void *arg)
 	return arg;
 }
 
+#define HAS_FAILED(x) ((x) == -1)
+
 static void setup(void)
 {
 	sigset_t sigusr1;
 	pthread_t defunct_thread;
 	char defunct_tid_path[PATH_MAX];
+	int ret;
 
 	sigemptyset(&sigusr1);
 	sigaddset(&sigusr1, SIGUSR1);
@@ -59,7 +62,9 @@ static void setup(void)
 	SAFE_PTHREAD_CREATE(&defunct_thread, NULL, defunct_thread_func, NULL);
 	SAFE_PTHREAD_JOIN(defunct_thread, NULL);
 	sprintf(defunct_tid_path, "/proc/%d/task/%d", getpid(), defunct_tid);
-	TST_RETRY_FN_EXP_BACKOFF(access(defunct_tid_path, R_OK), -1, 15);
+	ret = TST_RETRY_FN_EXP_BACKOFF(access(defunct_tid_path, R_OK), HAS_FAILED, 1);
+	if (!HAS_FAILED(ret))
+		tst_brk(TBROK, "Timeout %s still exists", defunct_tid_path);
 }
 
 static void cleanup(void)

Also I do not like that we are using the TEST() macro inside of an
macro, that may lead to unexpected consequencies. The TST_RET and
TST_ERR should not change unless user used TEST() macro explicitely.

-- 
Cyril Hrubis
chrubis@suse.cz