* [PATCH bpf-next 0/9] Revamp test_progs as a test running framework
@ 2019-07-26 20:37 Andrii Nakryiko
2019-07-26 20:37 ` [PATCH bpf-next 1/9] selftests/bpf: prevent headers to be compiled as C code Andrii Nakryiko
` (8 more replies)
0 siblings, 9 replies; 28+ messages in thread
From: Andrii Nakryiko @ 2019-07-26 20:37 UTC (permalink / raw)
To: bpf, netdev, ast, daniel; +Cc: andrii.nakryiko, kernel-team, Andrii Nakryiko
This patch set makes a number of changes to test_progs selftest, which is
a collection of many other tests (and sometimes sub-tests as well), to provide
better testing experience and allow to start convering many individual test
programs under selftests/bpf into a single and convenient test runner.
Patch #1 fixes issue with Makefile, which makes prog_tests/test.h compiled as
a C code. This fix allows to change how test.h is generated, providing ability
to have more control on what and how tests are run.
Patch #2 changes how test.h is auto-generated, which allows to have test
definitions, instead of just running test functions. This gives ability to do
more complicated test run policies.
Patch #3 adds `-t <test-name>` and `-n <test-num>` selectors to run only
subset of tests.
Patch #4 adds libbpf_swap_print() API allowing to temporarily replace current
print callback and then set it back. This is necessary for some tests that
want more control over libbpf logging.
Patch #5 sets up and takes over libbpf logging from individual tests to
test_prog runner, adding -vv verbosity to capture debug output from libbpf.
This is useful when debugging failing tests.
Patch #6 furthers test output management and buffers it by default, emitting
log output only if test fails. This give succinct and clean default test
output. It's possible to bypass this behavior with -v flag, which will turn
off test output buffering.
Patch #7 adds support for sub-tests. It also enhances -t and -n selectors to
both support ability to specify sub-test selectors, as well as enhancing
number selector to accept sets of test, instead of just individual test
number.
Patch #8 converts bpf_verif_scale.c test to use sub-test APIs.
Patch #9 converts send_signal.c tests to use sub-test APIs.
Andrii Nakryiko (9):
selftests/bpf: prevent headers to be compiled as C code
selftests/bpf: revamp test_progs to allow more control
selftests/bpf: add test selectors by number and name to test_progs
libbpf: add libbpf_swap_print to get previous print func
selftest/bpf: centralize libbpf logging management for test_progs
selftests/bpf: abstract away test log output
selftests/bpf: add sub-tests support for test_progs
selftests/bpf: convert bpf_verif_scale.c to sub-tests API
selftests/bpf: convert send_signal.c to use subtests
tools/lib/bpf/libbpf.c | 8 +
tools/lib/bpf/libbpf.h | 1 +
tools/lib/bpf/libbpf.map | 5 +
tools/testing/selftests/bpf/Makefile | 14 +-
.../selftests/bpf/prog_tests/bpf_obj_id.c | 6 +-
.../bpf/prog_tests/bpf_verif_scale.c | 90 +++--
.../bpf/prog_tests/get_stack_raw_tp.c | 4 +-
.../selftests/bpf/prog_tests/l4lb_all.c | 2 +-
.../selftests/bpf/prog_tests/map_lock.c | 10 +-
.../bpf/prog_tests/reference_tracking.c | 15 +-
.../selftests/bpf/prog_tests/send_signal.c | 17 +-
.../selftests/bpf/prog_tests/spinlock.c | 2 +-
.../bpf/prog_tests/stacktrace_build_id.c | 4 +-
.../bpf/prog_tests/stacktrace_build_id_nmi.c | 4 +-
.../selftests/bpf/prog_tests/xdp_noinline.c | 3 +-
tools/testing/selftests/bpf/test_progs.c | 373 +++++++++++++++++-
tools/testing/selftests/bpf/test_progs.h | 45 ++-
17 files changed, 501 insertions(+), 102 deletions(-)
--
2.17.1
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH bpf-next 1/9] selftests/bpf: prevent headers to be compiled as C code
2019-07-26 20:37 [PATCH bpf-next 0/9] Revamp test_progs as a test running framework Andrii Nakryiko
@ 2019-07-26 20:37 ` Andrii Nakryiko
2019-07-26 21:21 ` Stanislav Fomichev
2019-07-26 20:37 ` [PATCH bpf-next 2/9] selftests/bpf: revamp test_progs to allow more control Andrii Nakryiko
` (7 subsequent siblings)
8 siblings, 1 reply; 28+ messages in thread
From: Andrii Nakryiko @ 2019-07-26 20:37 UTC (permalink / raw)
To: bpf, netdev, ast, daniel; +Cc: andrii.nakryiko, kernel-team, Andrii Nakryiko
Apprently listing header as a normal dependency for a binary output
makes it go through compilation as if it was C code. This currently
works without a problem, but in subsequent commits causes problems for
differently generated test.h for test_progs. Marking those headers as
order-only dependency solves the issue.
Signed-off-by: Andrii Nakryiko <andriin@fb.com>
---
tools/testing/selftests/bpf/Makefile | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index 11c9c62c3362..bb66cc4a7f34 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -235,7 +235,7 @@ PROG_TESTS_H := $(PROG_TESTS_DIR)/tests.h
PROG_TESTS_FILES := $(wildcard prog_tests/*.c)
test_progs.c: $(PROG_TESTS_H)
$(OUTPUT)/test_progs: CFLAGS += $(TEST_PROGS_CFLAGS)
-$(OUTPUT)/test_progs: test_progs.c $(PROG_TESTS_H) $(PROG_TESTS_FILES)
+$(OUTPUT)/test_progs: test_progs.c $(PROG_TESTS_FILES) | $(PROG_TESTS_H)
$(PROG_TESTS_H): $(PROG_TESTS_FILES) | $(PROG_TESTS_DIR)
$(shell ( cd prog_tests/; \
echo '/* Generated header, do not edit */'; \
@@ -256,7 +256,7 @@ MAP_TESTS_H := $(MAP_TESTS_DIR)/tests.h
MAP_TESTS_FILES := $(wildcard map_tests/*.c)
test_maps.c: $(MAP_TESTS_H)
$(OUTPUT)/test_maps: CFLAGS += $(TEST_MAPS_CFLAGS)
-$(OUTPUT)/test_maps: test_maps.c $(MAP_TESTS_H) $(MAP_TESTS_FILES)
+$(OUTPUT)/test_maps: test_maps.c $(MAP_TESTS_FILES) | $(MAP_TESTS_H)
$(MAP_TESTS_H): $(MAP_TESTS_FILES) | $(MAP_TESTS_DIR)
$(shell ( cd map_tests/; \
echo '/* Generated header, do not edit */'; \
@@ -277,7 +277,7 @@ VERIFIER_TESTS_H := $(VERIFIER_TESTS_DIR)/tests.h
VERIFIER_TEST_FILES := $(wildcard verifier/*.c)
test_verifier.c: $(VERIFIER_TESTS_H)
$(OUTPUT)/test_verifier: CFLAGS += $(TEST_VERIFIER_CFLAGS)
-$(OUTPUT)/test_verifier: test_verifier.c $(VERIFIER_TESTS_H)
+$(OUTPUT)/test_verifier: test_verifier.c | $(VERIFIER_TEST_FILES) $(VERIFIER_TESTS_H)
$(VERIFIER_TESTS_H): $(VERIFIER_TEST_FILES) | $(VERIFIER_TESTS_DIR)
$(shell ( cd verifier/; \
echo '/* Generated header, do not edit */'; \
--
2.17.1
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH bpf-next 2/9] selftests/bpf: revamp test_progs to allow more control
2019-07-26 20:37 [PATCH bpf-next 0/9] Revamp test_progs as a test running framework Andrii Nakryiko
2019-07-26 20:37 ` [PATCH bpf-next 1/9] selftests/bpf: prevent headers to be compiled as C code Andrii Nakryiko
@ 2019-07-26 20:37 ` Andrii Nakryiko
2019-07-26 20:37 ` [PATCH bpf-next 3/9] selftests/bpf: add test selectors by number and name to test_progs Andrii Nakryiko
` (6 subsequent siblings)
8 siblings, 0 replies; 28+ messages in thread
From: Andrii Nakryiko @ 2019-07-26 20:37 UTC (permalink / raw)
To: bpf, netdev, ast, daniel; +Cc: andrii.nakryiko, kernel-team, Andrii Nakryiko
Refactor test_progs to allow better control on what's being run.
Also use argp to do argument parsing, so that it's easier to keep adding
more options.
Signed-off-by: Andrii Nakryiko <andriin@fb.com>
---
tools/testing/selftests/bpf/Makefile | 8 +--
tools/testing/selftests/bpf/test_progs.c | 84 +++++++++++++++++++++---
2 files changed, 77 insertions(+), 15 deletions(-)
diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index bb66cc4a7f34..3bd0f4a0336a 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -239,14 +239,8 @@ $(OUTPUT)/test_progs: test_progs.c $(PROG_TESTS_FILES) | $(PROG_TESTS_H)
$(PROG_TESTS_H): $(PROG_TESTS_FILES) | $(PROG_TESTS_DIR)
$(shell ( cd prog_tests/; \
echo '/* Generated header, do not edit */'; \
- echo '#ifdef DECLARE'; \
ls *.c 2> /dev/null | \
- sed -e 's@\([^\.]*\)\.c@extern void test_\1(void);@'; \
- echo '#endif'; \
- echo '#ifdef CALL'; \
- ls *.c 2> /dev/null | \
- sed -e 's@\([^\.]*\)\.c@test_\1();@'; \
- echo '#endif' \
+ sed -e 's@\([^\.]*\)\.c@DEFINE_TEST(\1)@'; \
) > $(PROG_TESTS_H))
MAP_TESTS_DIR = $(OUTPUT)/map_tests
diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
index dae0819b1141..eea88ba59225 100644
--- a/tools/testing/selftests/bpf/test_progs.c
+++ b/tools/testing/selftests/bpf/test_progs.c
@@ -3,6 +3,7 @@
*/
#include "test_progs.h"
#include "bpf_rlimit.h"
+#include <argp.h>
int error_cnt, pass_cnt;
bool jit_enabled;
@@ -156,22 +157,89 @@ void *spin_lock_thread(void *arg)
pthread_exit(arg);
}
-#define DECLARE
+/* extern declarations for test funcs */
+#define DEFINE_TEST(name) extern void test_##name();
#include <prog_tests/tests.h>
-#undef DECLARE
+#undef DEFINE_TEST
-int main(int ac, char **av)
+struct prog_test_def {
+ const char *test_name;
+ void (*run_test)(void);
+};
+
+static struct prog_test_def prog_test_defs[] = {
+#define DEFINE_TEST(name) { \
+ .test_name = #name, \
+ .run_test = &test_##name, \
+},
+#include <prog_tests/tests.h>
+#undef DEFINE_TEST
+};
+
+const char *argp_program_version = "test_progs 0.1";
+const char *argp_program_bug_address = "<bpf@vger.kernel.org>";
+const char argp_program_doc[] = "BPF selftests test runner";
+
+enum ARG_KEYS {
+ ARG_VERIFIER_STATS = 's',
+};
+
+static const struct argp_option opts[] = {
+ { "verifier-stats", ARG_VERIFIER_STATS, NULL, 0,
+ "Output verifier statistics", },
+ {},
+};
+
+struct test_env {
+ bool verifier_stats;
+};
+
+static struct test_env env = {};
+
+static error_t parse_arg(int key, char *arg, struct argp_state *state)
{
+ struct test_env *env = state->input;
+
+ switch (key) {
+ case ARG_VERIFIER_STATS:
+ env->verifier_stats = true;
+ break;
+ case ARGP_KEY_ARG:
+ argp_usage(state);
+ break;
+ case ARGP_KEY_END:
+ break;
+ default:
+ return ARGP_ERR_UNKNOWN;
+ }
+ return 0;
+}
+
+
+int main(int argc, char **argv)
+{
+ static const struct argp argp = {
+ .options = opts,
+ .parser = parse_arg,
+ .doc = argp_program_doc,
+ };
+ const struct prog_test_def *def;
+ int err, i;
+
+ err = argp_parse(&argp, argc, argv, 0, NULL, &env);
+ if (err)
+ return err;
+
srand(time(NULL));
jit_enabled = is_jit_enabled();
- if (ac == 2 && strcmp(av[1], "-s") == 0)
- verifier_stats = true;
+ verifier_stats = env.verifier_stats;
-#define CALL
-#include <prog_tests/tests.h>
-#undef CALL
+ for (i = 0; i < ARRAY_SIZE(prog_test_defs); i++) {
+ def = &prog_test_defs[i];
+ def->run_test();
+ }
printf("Summary: %d PASSED, %d FAILED\n", pass_cnt, error_cnt);
return error_cnt ? EXIT_FAILURE : EXIT_SUCCESS;
--
2.17.1
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH bpf-next 3/9] selftests/bpf: add test selectors by number and name to test_progs
2019-07-26 20:37 [PATCH bpf-next 0/9] Revamp test_progs as a test running framework Andrii Nakryiko
2019-07-26 20:37 ` [PATCH bpf-next 1/9] selftests/bpf: prevent headers to be compiled as C code Andrii Nakryiko
2019-07-26 20:37 ` [PATCH bpf-next 2/9] selftests/bpf: revamp test_progs to allow more control Andrii Nakryiko
@ 2019-07-26 20:37 ` Andrii Nakryiko
2019-07-26 21:25 ` Stanislav Fomichev
2019-07-26 20:37 ` [PATCH bpf-next 4/9] libbpf: add libbpf_swap_print to get previous print func Andrii Nakryiko
` (5 subsequent siblings)
8 siblings, 1 reply; 28+ messages in thread
From: Andrii Nakryiko @ 2019-07-26 20:37 UTC (permalink / raw)
To: bpf, netdev, ast, daniel; +Cc: andrii.nakryiko, kernel-team, Andrii Nakryiko
Add ability to specify either test number or test name substring to
narrow down a set of test to run.
Usage:
sudo ./test_progs -n 1
sudo ./test_progs -t attach_probe
Signed-off-by: Andrii Nakryiko <andriin@fb.com>
---
tools/testing/selftests/bpf/test_progs.c | 43 +++++++++++++++++++++---
1 file changed, 39 insertions(+), 4 deletions(-)
diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
index eea88ba59225..6e04b9f83777 100644
--- a/tools/testing/selftests/bpf/test_progs.c
+++ b/tools/testing/selftests/bpf/test_progs.c
@@ -4,6 +4,7 @@
#include "test_progs.h"
#include "bpf_rlimit.h"
#include <argp.h>
+#include <string.h>
int error_cnt, pass_cnt;
bool jit_enabled;
@@ -164,6 +165,7 @@ void *spin_lock_thread(void *arg)
struct prog_test_def {
const char *test_name;
+ int test_num;
void (*run_test)(void);
};
@@ -181,26 +183,49 @@ const char *argp_program_bug_address = "<bpf@vger.kernel.org>";
const char argp_program_doc[] = "BPF selftests test runner";
enum ARG_KEYS {
+ ARG_TEST_NUM = 'n',
+ ARG_TEST_NAME = 't',
ARG_VERIFIER_STATS = 's',
};
static const struct argp_option opts[] = {
+ { "num", ARG_TEST_NUM, "NUM", 0,
+ "Run test number NUM only " },
+ { "name", ARG_TEST_NAME, "NAME", 0,
+ "Run tests with names containing NAME" },
{ "verifier-stats", ARG_VERIFIER_STATS, NULL, 0,
"Output verifier statistics", },
{},
};
struct test_env {
+ int test_num_selector;
+ const char *test_name_selector;
bool verifier_stats;
};
-static struct test_env env = {};
+static struct test_env env = {
+ .test_num_selector = -1,
+};
static error_t parse_arg(int key, char *arg, struct argp_state *state)
{
struct test_env *env = state->input;
switch (key) {
+ case ARG_TEST_NUM: {
+ int test_num;
+
+ errno = 0;
+ test_num = strtol(arg, NULL, 10);
+ if (errno)
+ return -errno;
+ env->test_num_selector = test_num;
+ break;
+ }
+ case ARG_TEST_NAME:
+ env->test_name_selector = arg;
+ break;
case ARG_VERIFIER_STATS:
env->verifier_stats = true;
break;
@@ -223,7 +248,7 @@ int main(int argc, char **argv)
.parser = parse_arg,
.doc = argp_program_doc,
};
- const struct prog_test_def *def;
+ struct prog_test_def *test;
int err, i;
err = argp_parse(&argp, argc, argv, 0, NULL, &env);
@@ -237,8 +262,18 @@ int main(int argc, char **argv)
verifier_stats = env.verifier_stats;
for (i = 0; i < ARRAY_SIZE(prog_test_defs); i++) {
- def = &prog_test_defs[i];
- def->run_test();
+ test = &prog_test_defs[i];
+
+ test->test_num = i + 1;
+
+ if (env.test_num_selector >= 0 &&
+ test->test_num != env.test_num_selector)
+ continue;
+ if (env.test_name_selector &&
+ !strstr(test->test_name, env.test_name_selector))
+ continue;
+
+ test->run_test();
}
printf("Summary: %d PASSED, %d FAILED\n", pass_cnt, error_cnt);
--
2.17.1
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH bpf-next 4/9] libbpf: add libbpf_swap_print to get previous print func
2019-07-26 20:37 [PATCH bpf-next 0/9] Revamp test_progs as a test running framework Andrii Nakryiko
` (2 preceding siblings ...)
2019-07-26 20:37 ` [PATCH bpf-next 3/9] selftests/bpf: add test selectors by number and name to test_progs Andrii Nakryiko
@ 2019-07-26 20:37 ` Andrii Nakryiko
2019-07-26 21:28 ` Stanislav Fomichev
2019-07-26 20:37 ` [PATCH bpf-next 5/9] selftest/bpf: centralize libbpf logging management for test_progs Andrii Nakryiko
` (4 subsequent siblings)
8 siblings, 1 reply; 28+ messages in thread
From: Andrii Nakryiko @ 2019-07-26 20:37 UTC (permalink / raw)
To: bpf, netdev, ast, daniel; +Cc: andrii.nakryiko, kernel-team, Andrii Nakryiko
libbpf_swap_print allows to restore previously set print function.
This is useful when running many independent test with one default print
function, but overriding log verbosity for particular subset of tests.
Signed-off-by: Andrii Nakryiko <andriin@fb.com>
---
tools/lib/bpf/libbpf.c | 8 ++++++++
tools/lib/bpf/libbpf.h | 1 +
tools/lib/bpf/libbpf.map | 5 +++++
3 files changed, 14 insertions(+)
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 8741c39adb1c..0c254b6c9685 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -79,6 +79,14 @@ void libbpf_set_print(libbpf_print_fn_t fn)
__libbpf_pr = fn;
}
+libbpf_print_fn_t libbpf_swap_print(libbpf_print_fn_t fn)
+{
+ libbpf_print_fn_t old_print_fn = __libbpf_pr;
+
+ __libbpf_pr = fn;
+ return old_print_fn;
+}
+
__printf(2, 3)
void libbpf_print(enum libbpf_print_level level, const char *format, ...)
{
diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
index 5cbf459ece0b..4e0aa893571f 100644
--- a/tools/lib/bpf/libbpf.h
+++ b/tools/lib/bpf/libbpf.h
@@ -58,6 +58,7 @@ typedef int (*libbpf_print_fn_t)(enum libbpf_print_level level,
const char *, va_list ap);
LIBBPF_API void libbpf_set_print(libbpf_print_fn_t fn);
+LIBBPF_API libbpf_print_fn_t libbpf_swap_print(libbpf_print_fn_t fn);
/* Hide internal to user */
struct bpf_object;
diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
index f9d316e873d8..e211c38ddc43 100644
--- a/tools/lib/bpf/libbpf.map
+++ b/tools/lib/bpf/libbpf.map
@@ -184,3 +184,8 @@ LIBBPF_0.0.4 {
perf_buffer__new_raw;
perf_buffer__poll;
} LIBBPF_0.0.3;
+
+LIBBPF_0.0.5 {
+ global:
+ libbpf_swap_print;
+} LIBBPF_0.0.4;
--
2.17.1
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH bpf-next 5/9] selftest/bpf: centralize libbpf logging management for test_progs
2019-07-26 20:37 [PATCH bpf-next 0/9] Revamp test_progs as a test running framework Andrii Nakryiko
` (3 preceding siblings ...)
2019-07-26 20:37 ` [PATCH bpf-next 4/9] libbpf: add libbpf_swap_print to get previous print func Andrii Nakryiko
@ 2019-07-26 20:37 ` Andrii Nakryiko
2019-07-26 20:37 ` [PATCH bpf-next 6/9] selftests/bpf: abstract away test log output Andrii Nakryiko
` (3 subsequent siblings)
8 siblings, 0 replies; 28+ messages in thread
From: Andrii Nakryiko @ 2019-07-26 20:37 UTC (permalink / raw)
To: bpf, netdev, ast, daniel; +Cc: andrii.nakryiko, kernel-team, Andrii Nakryiko
Make test_progs test runner own libbpf logging. Also introduce two
levels of verbosity: -v and -vv. First one will be used in subsequent
patches to enable test log output always. Second one increases verbosity
level of libbpf logging further to include debug output as well.
Signed-off-by: Andrii Nakryiko <andriin@fb.com>
---
.../bpf/prog_tests/bpf_verif_scale.c | 6 +++-
.../bpf/prog_tests/reference_tracking.c | 15 +++-------
tools/testing/selftests/bpf/test_progs.c | 29 +++++++++++++++++++
3 files changed, 38 insertions(+), 12 deletions(-)
diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_verif_scale.c b/tools/testing/selftests/bpf/prog_tests/bpf_verif_scale.c
index e1b55261526f..2c4d9ef099b4 100644
--- a/tools/testing/selftests/bpf/prog_tests/bpf_verif_scale.c
+++ b/tools/testing/selftests/bpf/prog_tests/bpf_verif_scale.c
@@ -70,10 +70,11 @@ void test_bpf_verif_scale(void)
const char *cg_sysctl[] = {
"./test_sysctl_loop1.o", "./test_sysctl_loop2.o",
};
+ libbpf_print_fn_t old_print_fn = NULL;
int err, i;
if (verifier_stats)
- libbpf_set_print(libbpf_debug_print);
+ old_print_fn = libbpf_swap_print(libbpf_debug_print);
err = check_load("./loop3.o", BPF_PROG_TYPE_RAW_TRACEPOINT);
printf("test_scale:loop3:%s\n", err ? (error_cnt--, "OK") : "FAIL");
@@ -97,4 +98,7 @@ void test_bpf_verif_scale(void)
err = check_load("./test_seg6_loop.o", BPF_PROG_TYPE_LWT_SEG6LOCAL);
printf("test_scale:test_seg6_loop:%s\n", err ? "FAIL" : "OK");
+
+ if (verifier_stats)
+ libbpf_set_print(old_print_fn);
}
diff --git a/tools/testing/selftests/bpf/prog_tests/reference_tracking.c b/tools/testing/selftests/bpf/prog_tests/reference_tracking.c
index 5633be43828f..c9a6ef809bd1 100644
--- a/tools/testing/selftests/bpf/prog_tests/reference_tracking.c
+++ b/tools/testing/selftests/bpf/prog_tests/reference_tracking.c
@@ -1,15 +1,6 @@
// SPDX-License-Identifier: GPL-2.0
#include <test_progs.h>
-static int libbpf_debug_print(enum libbpf_print_level level,
- const char *format, va_list args)
-{
- if (level == LIBBPF_DEBUG)
- return 0;
-
- return vfprintf(stderr, format, args);
-}
-
void test_reference_tracking(void)
{
const char *file = "./test_sk_lookup_kern.o";
@@ -36,9 +27,11 @@ void test_reference_tracking(void)
/* Expect verifier failure if test name has 'fail' */
if (strstr(title, "fail") != NULL) {
- libbpf_set_print(NULL);
+ libbpf_print_fn_t old_print_fn;
+
+ old_print_fn = libbpf_swap_print(NULL);
err = !bpf_program__load(prog, "GPL", 0);
- libbpf_set_print(libbpf_debug_print);
+ libbpf_set_print(old_print_fn);
} else {
err = bpf_program__load(prog, "GPL", 0);
}
diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
index 6e04b9f83777..94b6951b90b3 100644
--- a/tools/testing/selftests/bpf/test_progs.c
+++ b/tools/testing/selftests/bpf/test_progs.c
@@ -186,6 +186,8 @@ enum ARG_KEYS {
ARG_TEST_NUM = 'n',
ARG_TEST_NAME = 't',
ARG_VERIFIER_STATS = 's',
+
+ ARG_VERBOSE = 'v',
};
static const struct argp_option opts[] = {
@@ -195,6 +197,8 @@ static const struct argp_option opts[] = {
"Run tests with names containing NAME" },
{ "verifier-stats", ARG_VERIFIER_STATS, NULL, 0,
"Output verifier statistics", },
+ { "verbose", ARG_VERBOSE, "LEVEL", OPTION_ARG_OPTIONAL,
+ "Verbose output (use -vv for extra verbose output)" },
{},
};
@@ -202,12 +206,22 @@ struct test_env {
int test_num_selector;
const char *test_name_selector;
bool verifier_stats;
+ bool verbose;
+ bool very_verbose;
};
static struct test_env env = {
.test_num_selector = -1,
};
+static int libbpf_print_fn(enum libbpf_print_level level,
+ const char *format, va_list args)
+{
+ if (!env.very_verbose && level == LIBBPF_DEBUG)
+ return 0;
+ return vfprintf(stderr, format, args);
+}
+
static error_t parse_arg(int key, char *arg, struct argp_state *state)
{
struct test_env *env = state->input;
@@ -229,6 +243,19 @@ static error_t parse_arg(int key, char *arg, struct argp_state *state)
case ARG_VERIFIER_STATS:
env->verifier_stats = true;
break;
+ case ARG_VERBOSE:
+ if (arg) {
+ if (strcmp(arg, "v") == 0) {
+ env->very_verbose = true;
+ } else {
+ fprintf(stderr,
+ "Unrecognized verbosity setting ('%s'), only -v and -vv are supported\n",
+ arg);
+ return -EINVAL;
+ }
+ }
+ env->verbose = true;
+ break;
case ARGP_KEY_ARG:
argp_usage(state);
break;
@@ -255,6 +282,8 @@ int main(int argc, char **argv)
if (err)
return err;
+ libbpf_set_print(libbpf_print_fn);
+
srand(time(NULL));
jit_enabled = is_jit_enabled();
--
2.17.1
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH bpf-next 6/9] selftests/bpf: abstract away test log output
2019-07-26 20:37 [PATCH bpf-next 0/9] Revamp test_progs as a test running framework Andrii Nakryiko
` (4 preceding siblings ...)
2019-07-26 20:37 ` [PATCH bpf-next 5/9] selftest/bpf: centralize libbpf logging management for test_progs Andrii Nakryiko
@ 2019-07-26 20:37 ` Andrii Nakryiko
2019-07-26 21:31 ` Stanislav Fomichev
2019-07-26 20:37 ` [PATCH bpf-next 7/9] selftests/bpf: add sub-tests support for test_progs Andrii Nakryiko
` (2 subsequent siblings)
8 siblings, 1 reply; 28+ messages in thread
From: Andrii Nakryiko @ 2019-07-26 20:37 UTC (permalink / raw)
To: bpf, netdev, ast, daniel; +Cc: andrii.nakryiko, kernel-team, Andrii Nakryiko
This patch changes how test output is printed out. By default, if test
had no errors, the only output will be a single line with test number,
name, and verdict at the end, e.g.:
#31 xdp:OK
If test had any errors, all log output captured during test execution
will be output after test completes.
It's possible to force output of log with `-v` (`--verbose`) option, in
which case output won't be buffered and will be output immediately.
To support this, individual tests are required to use helper methods for
logging: `test__printf()` and `test__vprintf()`.
Signed-off-by: Andrii Nakryiko <andriin@fb.com>
---
.../selftests/bpf/prog_tests/bpf_obj_id.c | 6 +-
.../bpf/prog_tests/bpf_verif_scale.c | 31 ++--
.../bpf/prog_tests/get_stack_raw_tp.c | 4 +-
.../selftests/bpf/prog_tests/l4lb_all.c | 2 +-
.../selftests/bpf/prog_tests/map_lock.c | 10 +-
.../selftests/bpf/prog_tests/send_signal.c | 8 +-
.../selftests/bpf/prog_tests/spinlock.c | 2 +-
.../bpf/prog_tests/stacktrace_build_id.c | 4 +-
.../bpf/prog_tests/stacktrace_build_id_nmi.c | 4 +-
.../selftests/bpf/prog_tests/xdp_noinline.c | 3 +-
tools/testing/selftests/bpf/test_progs.c | 135 +++++++++++++-----
tools/testing/selftests/bpf/test_progs.h | 37 ++++-
12 files changed, 173 insertions(+), 73 deletions(-)
diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_obj_id.c b/tools/testing/selftests/bpf/prog_tests/bpf_obj_id.c
index cb827383db4d..fb5840a62548 100644
--- a/tools/testing/selftests/bpf/prog_tests/bpf_obj_id.c
+++ b/tools/testing/selftests/bpf/prog_tests/bpf_obj_id.c
@@ -106,8 +106,8 @@ void test_bpf_obj_id(void)
if (CHECK(err ||
prog_infos[i].type != BPF_PROG_TYPE_SOCKET_FILTER ||
info_len != sizeof(struct bpf_prog_info) ||
- (jit_enabled && !prog_infos[i].jited_prog_len) ||
- (jit_enabled &&
+ (env.jit_enabled && !prog_infos[i].jited_prog_len) ||
+ (env.jit_enabled &&
!memcmp(jited_insns, zeros, sizeof(zeros))) ||
!prog_infos[i].xlated_prog_len ||
!memcmp(xlated_insns, zeros, sizeof(zeros)) ||
@@ -121,7 +121,7 @@ void test_bpf_obj_id(void)
err, errno, i,
prog_infos[i].type, BPF_PROG_TYPE_SOCKET_FILTER,
info_len, sizeof(struct bpf_prog_info),
- jit_enabled,
+ env.jit_enabled,
prog_infos[i].jited_prog_len,
prog_infos[i].xlated_prog_len,
!!memcmp(jited_insns, zeros, sizeof(zeros)),
diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_verif_scale.c b/tools/testing/selftests/bpf/prog_tests/bpf_verif_scale.c
index 2c4d9ef099b4..95f012c417fe 100644
--- a/tools/testing/selftests/bpf/prog_tests/bpf_verif_scale.c
+++ b/tools/testing/selftests/bpf/prog_tests/bpf_verif_scale.c
@@ -4,12 +4,15 @@
static int libbpf_debug_print(enum libbpf_print_level level,
const char *format, va_list args)
{
- if (level != LIBBPF_DEBUG)
- return vfprintf(stderr, format, args);
+ if (level != LIBBPF_DEBUG) {
+ test__vprintf(format, args);
+ return 0;
+ }
if (!strstr(format, "verifier log"))
return 0;
- return vfprintf(stderr, "%s", args);
+ test__vprintf("%s", args);
+ return 0;
}
static int check_load(const char *file, enum bpf_prog_type type)
@@ -73,32 +76,38 @@ void test_bpf_verif_scale(void)
libbpf_print_fn_t old_print_fn = NULL;
int err, i;
- if (verifier_stats)
+ if (env.verifier_stats) {
+ test__force_log();
old_print_fn = libbpf_swap_print(libbpf_debug_print);
+ }
err = check_load("./loop3.o", BPF_PROG_TYPE_RAW_TRACEPOINT);
- printf("test_scale:loop3:%s\n", err ? (error_cnt--, "OK") : "FAIL");
+ test__printf("test_scale:loop3:%s\n",
+ err ? (error_cnt--, "OK") : "FAIL");
for (i = 0; i < ARRAY_SIZE(sched_cls); i++) {
err = check_load(sched_cls[i], BPF_PROG_TYPE_SCHED_CLS);
- printf("test_scale:%s:%s\n", sched_cls[i], err ? "FAIL" : "OK");
+ test__printf("test_scale:%s:%s\n", sched_cls[i],
+ err ? "FAIL" : "OK");
}
for (i = 0; i < ARRAY_SIZE(raw_tp); i++) {
err = check_load(raw_tp[i], BPF_PROG_TYPE_RAW_TRACEPOINT);
- printf("test_scale:%s:%s\n", raw_tp[i], err ? "FAIL" : "OK");
+ test__printf("test_scale:%s:%s\n", raw_tp[i],
+ err ? "FAIL" : "OK");
}
for (i = 0; i < ARRAY_SIZE(cg_sysctl); i++) {
err = check_load(cg_sysctl[i], BPF_PROG_TYPE_CGROUP_SYSCTL);
- printf("test_scale:%s:%s\n", cg_sysctl[i], err ? "FAIL" : "OK");
+ test__printf("test_scale:%s:%s\n", cg_sysctl[i],
+ err ? "FAIL" : "OK");
}
err = check_load("./test_xdp_loop.o", BPF_PROG_TYPE_XDP);
- printf("test_scale:test_xdp_loop:%s\n", err ? "FAIL" : "OK");
+ test__printf("test_scale:test_xdp_loop:%s\n", err ? "FAIL" : "OK");
err = check_load("./test_seg6_loop.o", BPF_PROG_TYPE_LWT_SEG6LOCAL);
- printf("test_scale:test_seg6_loop:%s\n", err ? "FAIL" : "OK");
+ test__printf("test_scale:test_seg6_loop:%s\n", err ? "FAIL" : "OK");
- if (verifier_stats)
+ if (env.verifier_stats)
libbpf_set_print(old_print_fn);
}
diff --git a/tools/testing/selftests/bpf/prog_tests/get_stack_raw_tp.c b/tools/testing/selftests/bpf/prog_tests/get_stack_raw_tp.c
index 9d73a8f932ac..3d59b3c841fe 100644
--- a/tools/testing/selftests/bpf/prog_tests/get_stack_raw_tp.c
+++ b/tools/testing/selftests/bpf/prog_tests/get_stack_raw_tp.c
@@ -41,7 +41,7 @@ static void get_stack_print_output(void *ctx, int cpu, void *data, __u32 size)
* just assume it is good if the stack is not empty.
* This could be improved in the future.
*/
- if (jit_enabled) {
+ if (env.jit_enabled) {
found = num_stack > 0;
} else {
for (i = 0; i < num_stack; i++) {
@@ -58,7 +58,7 @@ static void get_stack_print_output(void *ctx, int cpu, void *data, __u32 size)
}
} else {
num_stack = e->kern_stack_size / sizeof(__u64);
- if (jit_enabled) {
+ if (env.jit_enabled) {
good_kern_stack = num_stack > 0;
} else {
for (i = 0; i < num_stack; i++) {
diff --git a/tools/testing/selftests/bpf/prog_tests/l4lb_all.c b/tools/testing/selftests/bpf/prog_tests/l4lb_all.c
index 20ddca830e68..5ce572c03a5f 100644
--- a/tools/testing/selftests/bpf/prog_tests/l4lb_all.c
+++ b/tools/testing/selftests/bpf/prog_tests/l4lb_all.c
@@ -74,7 +74,7 @@ static void test_l4lb(const char *file)
}
if (bytes != MAGIC_BYTES * NUM_ITER * 2 || pkts != NUM_ITER * 2) {
error_cnt++;
- printf("test_l4lb:FAIL:stats %lld %lld\n", bytes, pkts);
+ test__printf("test_l4lb:FAIL:stats %lld %lld\n", bytes, pkts);
}
out:
bpf_object__close(obj);
diff --git a/tools/testing/selftests/bpf/prog_tests/map_lock.c b/tools/testing/selftests/bpf/prog_tests/map_lock.c
index ee99368c595c..2e78217ed3fd 100644
--- a/tools/testing/selftests/bpf/prog_tests/map_lock.c
+++ b/tools/testing/selftests/bpf/prog_tests/map_lock.c
@@ -9,12 +9,12 @@ static void *parallel_map_access(void *arg)
for (i = 0; i < 10000; i++) {
err = bpf_map_lookup_elem_flags(map_fd, &key, vars, BPF_F_LOCK);
if (err) {
- printf("lookup failed\n");
+ test__printf("lookup failed\n");
error_cnt++;
goto out;
}
if (vars[0] != 0) {
- printf("lookup #%d var[0]=%d\n", i, vars[0]);
+ test__printf("lookup #%d var[0]=%d\n", i, vars[0]);
error_cnt++;
goto out;
}
@@ -22,8 +22,8 @@ static void *parallel_map_access(void *arg)
for (j = 2; j < 17; j++) {
if (vars[j] == rnd)
continue;
- printf("lookup #%d var[1]=%d var[%d]=%d\n",
- i, rnd, j, vars[j]);
+ test__printf("lookup #%d var[1]=%d var[%d]=%d\n",
+ i, rnd, j, vars[j]);
error_cnt++;
goto out;
}
@@ -43,7 +43,7 @@ void test_map_lock(void)
err = bpf_prog_load(file, BPF_PROG_TYPE_CGROUP_SKB, &obj, &prog_fd);
if (err) {
- printf("test_map_lock:bpf_prog_load errno %d\n", errno);
+ test__printf("test_map_lock:bpf_prog_load errno %d\n", errno);
goto close_prog;
}
map_fd[0] = bpf_find_map(__func__, obj, "hash_map");
diff --git a/tools/testing/selftests/bpf/prog_tests/send_signal.c b/tools/testing/selftests/bpf/prog_tests/send_signal.c
index 54218ee3c004..d950f4558897 100644
--- a/tools/testing/selftests/bpf/prog_tests/send_signal.c
+++ b/tools/testing/selftests/bpf/prog_tests/send_signal.c
@@ -202,8 +202,8 @@ static int test_send_signal_nmi(void)
-1 /* cpu */, -1 /* group_fd */, 0 /* flags */);
if (pmu_fd == -1) {
if (errno == ENOENT) {
- printf("%s:SKIP:no PERF_COUNT_HW_CPU_CYCLES\n",
- __func__);
+ test__printf("%s:SKIP:no PERF_COUNT_HW_CPU_CYCLES\n",
+ __func__);
return 0;
}
/* Let the test fail with a more informative message */
@@ -222,8 +222,4 @@ void test_send_signal(void)
ret |= test_send_signal_tracepoint();
ret |= test_send_signal_perf();
ret |= test_send_signal_nmi();
- if (!ret)
- printf("test_send_signal:OK\n");
- else
- printf("test_send_signal:FAIL\n");
}
diff --git a/tools/testing/selftests/bpf/prog_tests/spinlock.c b/tools/testing/selftests/bpf/prog_tests/spinlock.c
index 114ebe6a438e..deb2db5b85b0 100644
--- a/tools/testing/selftests/bpf/prog_tests/spinlock.c
+++ b/tools/testing/selftests/bpf/prog_tests/spinlock.c
@@ -12,7 +12,7 @@ void test_spinlock(void)
err = bpf_prog_load(file, BPF_PROG_TYPE_CGROUP_SKB, &obj, &prog_fd);
if (err) {
- printf("test_spin_lock:bpf_prog_load errno %d\n", errno);
+ test__printf("test_spin_lock:bpf_prog_load errno %d\n", errno);
goto close_prog;
}
for (i = 0; i < 4; i++)
diff --git a/tools/testing/selftests/bpf/prog_tests/stacktrace_build_id.c b/tools/testing/selftests/bpf/prog_tests/stacktrace_build_id.c
index ac44fda84833..356d2c017a9c 100644
--- a/tools/testing/selftests/bpf/prog_tests/stacktrace_build_id.c
+++ b/tools/testing/selftests/bpf/prog_tests/stacktrace_build_id.c
@@ -109,8 +109,8 @@ void test_stacktrace_build_id(void)
if (build_id_matches < 1 && retry--) {
bpf_link__destroy(link);
bpf_object__close(obj);
- printf("%s:WARN:Didn't find expected build ID from the map, retrying\n",
- __func__);
+ test__printf("%s:WARN:Didn't find expected build ID from the map, retrying\n",
+ __func__);
goto retry;
}
diff --git a/tools/testing/selftests/bpf/prog_tests/stacktrace_build_id_nmi.c b/tools/testing/selftests/bpf/prog_tests/stacktrace_build_id_nmi.c
index 9557b7dfb782..f44f2c159714 100644
--- a/tools/testing/selftests/bpf/prog_tests/stacktrace_build_id_nmi.c
+++ b/tools/testing/selftests/bpf/prog_tests/stacktrace_build_id_nmi.c
@@ -140,8 +140,8 @@ void test_stacktrace_build_id_nmi(void)
if (build_id_matches < 1 && retry--) {
bpf_link__destroy(link);
bpf_object__close(obj);
- printf("%s:WARN:Didn't find expected build ID from the map, retrying\n",
- __func__);
+ test__printf("%s:WARN:Didn't find expected build ID from the map, retrying\n",
+ __func__);
goto retry;
}
diff --git a/tools/testing/selftests/bpf/prog_tests/xdp_noinline.c b/tools/testing/selftests/bpf/prog_tests/xdp_noinline.c
index 09e6b46f5515..b5404494b8aa 100644
--- a/tools/testing/selftests/bpf/prog_tests/xdp_noinline.c
+++ b/tools/testing/selftests/bpf/prog_tests/xdp_noinline.c
@@ -75,7 +75,8 @@ void test_xdp_noinline(void)
}
if (bytes != MAGIC_BYTES * NUM_ITER * 2 || pkts != NUM_ITER * 2) {
error_cnt++;
- printf("test_xdp_noinline:FAIL:stats %lld %lld\n", bytes, pkts);
+ test__printf("test_xdp_noinline:FAIL:stats %lld %lld\n",
+ bytes, pkts);
}
out:
bpf_object__close(obj);
diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
index 94b6951b90b3..3cf3ebda1d31 100644
--- a/tools/testing/selftests/bpf/test_progs.c
+++ b/tools/testing/selftests/bpf/test_progs.c
@@ -6,9 +6,75 @@
#include <argp.h>
#include <string.h>
+/* defined in test_progs.h */
+struct test_env env = {
+ .test_num_selector = -1,
+};
int error_cnt, pass_cnt;
-bool jit_enabled;
-bool verifier_stats = false;
+
+struct prog_test_def {
+ const char *test_name;
+ int test_num;
+ void (*run_test)(void);
+ bool force_log;
+ int pass_cnt;
+ int error_cnt;
+ bool tested;
+};
+
+void test__force_log() {
+ env.test->force_log = true;
+}
+
+void test__vprintf(const char *fmt, va_list args)
+{
+ size_t rem_sz;
+ int ret;
+
+ if (env.verbose || (env.test && env.test->force_log)) {
+ vfprintf(stderr, fmt, args);
+ return;
+ }
+
+try_again:
+ rem_sz = env.log_cap - env.log_cnt;
+ if (rem_sz) {
+ ret = vsnprintf(env.log_buf + env.log_cnt, rem_sz, fmt, args);
+ if (ret < 0) {
+ fprintf(stderr, "failed to log message w/ fmt '%s'\n", fmt);
+ return;
+ }
+ }
+
+ if (ret >= rem_sz) {
+ size_t new_sz = env.log_cap * 3 / 2;
+ char *new_buf;
+
+ if (new_sz < 4096)
+ new_sz = 4096;
+
+ new_buf = realloc(env.log_buf, new_sz);
+ if (!new_buf) {
+ fprintf(stderr, "failed to realloc log buffer: %d\n",
+ errno);
+ return;
+ }
+ env.log_buf = new_buf;
+ env.log_cap = new_sz;
+ goto try_again;
+ }
+
+ env.log_cnt += ret + 1;
+}
+
+void test__printf(const char *fmt, ...)
+{
+ va_list args;
+
+ va_start(args, fmt);
+ test__vprintf(fmt, args);
+ va_end(args);
+}
struct ipv4_packet pkt_v4 = {
.eth.h_proto = __bpf_constant_htons(ETH_P_IP),
@@ -163,20 +229,15 @@ void *spin_lock_thread(void *arg)
#include <prog_tests/tests.h>
#undef DEFINE_TEST
-struct prog_test_def {
- const char *test_name;
- int test_num;
- void (*run_test)(void);
-};
-
static struct prog_test_def prog_test_defs[] = {
-#define DEFINE_TEST(name) { \
- .test_name = #name, \
- .run_test = &test_##name, \
+#define DEFINE_TEST(name) { \
+ .test_name = #name, \
+ .run_test = &test_##name, \
},
#include <prog_tests/tests.h>
#undef DEFINE_TEST
};
+const int prog_test_cnt = ARRAY_SIZE(prog_test_defs);
const char *argp_program_version = "test_progs 0.1";
const char *argp_program_bug_address = "<bpf@vger.kernel.org>";
@@ -186,7 +247,6 @@ enum ARG_KEYS {
ARG_TEST_NUM = 'n',
ARG_TEST_NAME = 't',
ARG_VERIFIER_STATS = 's',
-
ARG_VERBOSE = 'v',
};
@@ -202,24 +262,13 @@ static const struct argp_option opts[] = {
{},
};
-struct test_env {
- int test_num_selector;
- const char *test_name_selector;
- bool verifier_stats;
- bool verbose;
- bool very_verbose;
-};
-
-static struct test_env env = {
- .test_num_selector = -1,
-};
-
static int libbpf_print_fn(enum libbpf_print_level level,
const char *format, va_list args)
{
if (!env.very_verbose && level == LIBBPF_DEBUG)
return 0;
- return vfprintf(stderr, format, args);
+ test__vprintf(format, args);
+ return 0;
}
static error_t parse_arg(int key, char *arg, struct argp_state *state)
@@ -267,7 +316,6 @@ static error_t parse_arg(int key, char *arg, struct argp_state *state)
return 0;
}
-
int main(int argc, char **argv)
{
static const struct argp argp = {
@@ -275,7 +323,6 @@ int main(int argc, char **argv)
.parser = parse_arg,
.doc = argp_program_doc,
};
- struct prog_test_def *test;
int err, i;
err = argp_parse(&argp, argc, argv, 0, NULL, &env);
@@ -286,13 +333,14 @@ int main(int argc, char **argv)
srand(time(NULL));
- jit_enabled = is_jit_enabled();
+ env.jit_enabled = is_jit_enabled();
- verifier_stats = env.verifier_stats;
-
- for (i = 0; i < ARRAY_SIZE(prog_test_defs); i++) {
- test = &prog_test_defs[i];
+ for (i = 0; i < prog_test_cnt; i++) {
+ struct prog_test_def *test = &prog_test_defs[i];
+ int old_pass_cnt = pass_cnt;
+ int old_error_cnt = error_cnt;
+ env.test = test;
test->test_num = i + 1;
if (env.test_num_selector >= 0 &&
@@ -303,8 +351,29 @@ int main(int argc, char **argv)
continue;
test->run_test();
+ test->tested = true;
+ test->pass_cnt = pass_cnt - old_pass_cnt;
+ test->error_cnt = error_cnt - old_error_cnt;
+ if (test->error_cnt)
+ env.fail_cnt++;
+ else
+ env.succ_cnt++;
+
+ if (env.verbose || test->force_log || test->error_cnt) {
+ if (env.log_cnt) {
+ fprintf(stdout, "%s", env.log_buf);
+ if (env.log_buf[env.log_cnt - 1] != '\n')
+ fprintf(stdout, "\n");
+ }
+ }
+ env.log_cnt = 0;
+
+ printf("#%d %s:%s\n", test->test_num, test->test_name,
+ test->error_cnt ? "FAIL" : "OK");
}
+ printf("Summary: %d PASSED, %d FAILED\n", env.succ_cnt, env.fail_cnt);
+
+ free(env.log_buf);
- printf("Summary: %d PASSED, %d FAILED\n", pass_cnt, error_cnt);
return error_cnt ? EXIT_FAILURE : EXIT_SUCCESS;
}
diff --git a/tools/testing/selftests/bpf/test_progs.h b/tools/testing/selftests/bpf/test_progs.h
index 49e0f7d85643..62f55a4231e9 100644
--- a/tools/testing/selftests/bpf/test_progs.h
+++ b/tools/testing/selftests/bpf/test_progs.h
@@ -38,9 +38,33 @@ typedef __u16 __sum16;
#include "trace_helpers.h"
#include "flow_dissector_load.h"
-extern int error_cnt, pass_cnt;
-extern bool jit_enabled;
-extern bool verifier_stats;
+struct prog_test_def;
+
+struct test_env {
+ int test_num_selector;
+ const char *test_name_selector;
+ bool verifier_stats;
+ bool verbose;
+ bool very_verbose;
+
+ bool jit_enabled;
+
+ struct prog_test_def *test;
+ char *log_buf;
+ size_t log_cnt;
+ size_t log_cap;
+
+ int succ_cnt;
+ int fail_cnt;
+};
+
+extern int error_cnt;
+extern int pass_cnt;
+extern struct test_env env;
+
+extern void test__printf(const char *fmt, ...);
+extern void test__vprintf(const char *fmt, va_list args);
+extern void test__force_log();
#define MAGIC_BYTES 123
@@ -64,11 +88,12 @@ extern struct ipv6_packet pkt_v6;
int __ret = !!(condition); \
if (__ret) { \
error_cnt++; \
- printf("%s:FAIL:%s ", __func__, tag); \
- printf(format); \
+ test__printf("%s:FAIL:%s ", __func__, tag); \
+ test__printf(format); \
} else { \
pass_cnt++; \
- printf("%s:PASS:%s %d nsec\n", __func__, tag, duration);\
+ test__printf("%s:PASS:%s %d nsec\n", \
+ __func__, tag, duration); \
} \
__ret; \
})
--
2.17.1
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH bpf-next 7/9] selftests/bpf: add sub-tests support for test_progs
2019-07-26 20:37 [PATCH bpf-next 0/9] Revamp test_progs as a test running framework Andrii Nakryiko
` (5 preceding siblings ...)
2019-07-26 20:37 ` [PATCH bpf-next 6/9] selftests/bpf: abstract away test log output Andrii Nakryiko
@ 2019-07-26 20:37 ` Andrii Nakryiko
2019-07-26 20:37 ` [PATCH bpf-next 8/9] selftests/bpf: convert bpf_verif_scale.c to sub-tests API Andrii Nakryiko
2019-07-26 20:37 ` [PATCH bpf-next 9/9] selftests/bpf: convert send_signal.c to use subtests Andrii Nakryiko
8 siblings, 0 replies; 28+ messages in thread
From: Andrii Nakryiko @ 2019-07-26 20:37 UTC (permalink / raw)
To: bpf, netdev, ast, daniel; +Cc: andrii.nakryiko, kernel-team, Andrii Nakryiko
Allow tests to have their own set of sub-tests. Also add ability to do
test/subtest selection using `-t <test-name>/<subtest-name>` and `-n
<test-nums-set>/<subtest-nums-set>`, as an extension of existing -t/-n
selector options. For the <test-num-set> format: it's a comma-separated
list of either individual test numbers (1-based), or range of test
numbers. E.g., all of the following are valid sets of test numbers:
- 10
- 1,2,3
- 1-3
- 5-10,1,3-4
'/<subtest' part is optional, but has the same format. E.g., to select
test #3 and its sub-tests #10 through #15, use: -t 3/10-15.
Similarly, to select tests by name, use `-t verif/strobe`:
$ sudo ./test_progs -t verif/strobe
#3/12 strobemeta.o:OK
#3/13 strobemeta_nounroll1.o:OK
#3/14 strobemeta_nounroll2.o:OK
#3 bpf_verif_scale:OK
Summary: 1/3 PASSED, 0 FAILED
Example of using subtest API is in the next patch, converting
bpf_verif_scale.c tests to use sub-tests.
Signed-off-by: Andrii Nakryiko <andriin@fb.com>
---
tools/testing/selftests/bpf/test_progs.c | 198 ++++++++++++++++++++---
tools/testing/selftests/bpf/test_progs.h | 16 +-
2 files changed, 185 insertions(+), 29 deletions(-)
diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
index 3cf3ebda1d31..7a2db48b6fd1 100644
--- a/tools/testing/selftests/bpf/test_progs.c
+++ b/tools/testing/selftests/bpf/test_progs.c
@@ -7,9 +7,7 @@
#include <string.h>
/* defined in test_progs.h */
-struct test_env env = {
- .test_num_selector = -1,
-};
+struct test_env env;
int error_cnt, pass_cnt;
struct prog_test_def {
@@ -20,8 +18,82 @@ struct prog_test_def {
int pass_cnt;
int error_cnt;
bool tested;
+
+ const char *subtest_name;
+ int subtest_num;
+
+ /* store counts before subtest started */
+ int old_pass_cnt;
+ int old_error_cnt;
};
+static bool should_run(struct test_selector *sel, int num, const char *name)
+{
+ if (sel->name && sel->name[0] && !strstr(name, sel->name))
+ return false;
+
+ if (!sel->num_set)
+ return true;
+
+ return num < sel->num_set_len && sel->num_set[num];
+}
+
+static void dump_test_log(const struct prog_test_def *test, bool failed)
+{
+ if (env.verbose || test->force_log || failed) {
+ if (env.log_cnt) {
+ fprintf(stdout, "%s", env.log_buf);
+ if (env.log_buf[env.log_cnt - 1] != '\n')
+ fprintf(stdout, "\n");
+ }
+ env.log_cnt = 0;
+ }
+}
+
+void test__end_subtest()
+{
+ struct prog_test_def *test = env.test;
+ int sub_error_cnt = error_cnt - test->old_error_cnt;
+
+ if (sub_error_cnt)
+ env.fail_cnt++;
+ else
+ env.sub_succ_cnt++;
+
+ dump_test_log(test, sub_error_cnt);
+
+ printf("#%d/%d %s:%s\n",
+ test->test_num, test->subtest_num,
+ test->subtest_name, sub_error_cnt ? "FAIL" : "OK");
+}
+
+bool test__start_subtest(const char *name)
+{
+ struct prog_test_def *test = env.test;
+
+ if (test->subtest_name) {
+ test__end_subtest();
+ test->subtest_name = NULL;
+ }
+
+ test->subtest_num++;
+
+ if (!name || !name[0]) {
+ fprintf(stderr, "Subtest #%d didn't provide sub-test name!\n",
+ test->subtest_num);
+ return false;
+ }
+
+ if (!should_run(&env.subtest_selector, test->subtest_num, name))
+ return false;
+
+ test->subtest_name = name;
+ env.test->old_pass_cnt = pass_cnt;
+ env.test->old_error_cnt = error_cnt;
+
+ return true;
+}
+
void test__force_log() {
env.test->force_log = true;
}
@@ -271,24 +343,103 @@ static int libbpf_print_fn(enum libbpf_print_level level,
return 0;
}
+int parse_num_list(const char *s, struct test_selector *sel)
+{
+ int i, set_len = 0, num, start = 0, end = -1;
+ bool *set = NULL, *tmp, parsing_end = false;
+ char *next;
+
+ while (s[0]) {
+ errno = 0;
+ num = strtol(s, &next, 10);
+ if (errno)
+ return -errno;
+
+ if (parsing_end)
+ end = num;
+ else
+ start = num;
+
+ if (!parsing_end && *next == '-') {
+ s = next + 1;
+ parsing_end = true;
+ continue;
+ } else if (*next == ',') {
+ parsing_end = false;
+ s = next + 1;
+ end = num;
+ } else if (*next == '\0') {
+ parsing_end = false;
+ s = next;
+ end = num;
+ } else {
+ return -EINVAL;
+ }
+
+ if (start > end)
+ return -EINVAL;
+
+ if (end + 1 > set_len) {
+ set_len = end + 1;
+ tmp = realloc(set, set_len);
+ if (!tmp) {
+ free(set);
+ return -ENOMEM;
+ }
+ set = tmp;
+ }
+ for (i = start; i <= end; i++) {
+ set[i] = true;
+ }
+
+ }
+
+ if (!set)
+ return -EINVAL;
+
+ sel->num_set = set;
+ sel->num_set_len = set_len;
+
+ return 0;
+}
+
static error_t parse_arg(int key, char *arg, struct argp_state *state)
{
struct test_env *env = state->input;
switch (key) {
case ARG_TEST_NUM: {
- int test_num;
+ char *subtest_str = strchr(arg, '/');
- errno = 0;
- test_num = strtol(arg, NULL, 10);
- if (errno)
- return -errno;
- env->test_num_selector = test_num;
+ if (subtest_str) {
+ *subtest_str = '\0';
+ if (parse_num_list(subtest_str + 1,
+ &env->subtest_selector)) {
+ fprintf(stderr,
+ "Failed to parse subtest numbers.\n");
+ return -EINVAL;
+ }
+ }
+ if (parse_num_list(arg, &env->test_selector)) {
+ fprintf(stderr, "Failed to parse test numbers.\n");
+ return -EINVAL;
+ }
break;
}
- case ARG_TEST_NAME:
- env->test_name_selector = arg;
+ case ARG_TEST_NAME: {
+ char *subtest_str = strchr(arg, '/');
+
+ if (subtest_str) {
+ *subtest_str = '\0';
+ env->subtest_selector.name = strdup(subtest_str + 1);
+ if (!env->subtest_selector.name)
+ return -ENOMEM;
+ }
+ env->test_selector.name = strdup(arg);
+ if (!env->test_selector.name)
+ return -ENOMEM;
break;
+ }
case ARG_VERIFIER_STATS:
env->verifier_stats = true;
break;
@@ -343,14 +494,15 @@ int main(int argc, char **argv)
env.test = test;
test->test_num = i + 1;
- if (env.test_num_selector >= 0 &&
- test->test_num != env.test_num_selector)
- continue;
- if (env.test_name_selector &&
- !strstr(test->test_name, env.test_name_selector))
+ if (!should_run(&env.test_selector,
+ test->test_num, test->test_name))
continue;
test->run_test();
+ /* ensure last sub-test is finalized properly */
+ if (test->subtest_name)
+ test__end_subtest();
+
test->tested = true;
test->pass_cnt = pass_cnt - old_pass_cnt;
test->error_cnt = error_cnt - old_error_cnt;
@@ -359,21 +511,17 @@ int main(int argc, char **argv)
else
env.succ_cnt++;
- if (env.verbose || test->force_log || test->error_cnt) {
- if (env.log_cnt) {
- fprintf(stdout, "%s", env.log_buf);
- if (env.log_buf[env.log_cnt - 1] != '\n')
- fprintf(stdout, "\n");
- }
- }
- env.log_cnt = 0;
+ dump_test_log(test, test->error_cnt);
printf("#%d %s:%s\n", test->test_num, test->test_name,
test->error_cnt ? "FAIL" : "OK");
}
- printf("Summary: %d PASSED, %d FAILED\n", env.succ_cnt, env.fail_cnt);
+ printf("Summary: %d/%d PASSED, %d FAILED\n",
+ env.succ_cnt, env.sub_succ_cnt, env.fail_cnt);
free(env.log_buf);
+ free(env.test_selector.num_set);
+ free(env.subtest_selector.num_set);
return error_cnt ? EXIT_FAILURE : EXIT_SUCCESS;
}
diff --git a/tools/testing/selftests/bpf/test_progs.h b/tools/testing/selftests/bpf/test_progs.h
index 62f55a4231e9..afd14962456f 100644
--- a/tools/testing/selftests/bpf/test_progs.h
+++ b/tools/testing/selftests/bpf/test_progs.h
@@ -40,9 +40,15 @@ typedef __u16 __sum16;
struct prog_test_def;
+struct test_selector {
+ const char *name;
+ bool *num_set;
+ int num_set_len;
+};
+
struct test_env {
- int test_num_selector;
- const char *test_name_selector;
+ struct test_selector test_selector;
+ struct test_selector subtest_selector;
bool verifier_stats;
bool verbose;
bool very_verbose;
@@ -54,8 +60,9 @@ struct test_env {
size_t log_cnt;
size_t log_cap;
- int succ_cnt;
- int fail_cnt;
+ int succ_cnt; /* successful tests */
+ int sub_succ_cnt; /* successful sub-tests */
+ int fail_cnt; /* total failed tests + sub-tests */
};
extern int error_cnt;
@@ -65,6 +72,7 @@ extern struct test_env env;
extern void test__printf(const char *fmt, ...);
extern void test__vprintf(const char *fmt, va_list args);
extern void test__force_log();
+extern bool test__start_subtest(const char *name);
#define MAGIC_BYTES 123
--
2.17.1
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH bpf-next 8/9] selftests/bpf: convert bpf_verif_scale.c to sub-tests API
2019-07-26 20:37 [PATCH bpf-next 0/9] Revamp test_progs as a test running framework Andrii Nakryiko
` (6 preceding siblings ...)
2019-07-26 20:37 ` [PATCH bpf-next 7/9] selftests/bpf: add sub-tests support for test_progs Andrii Nakryiko
@ 2019-07-26 20:37 ` Andrii Nakryiko
2019-07-26 20:37 ` [PATCH bpf-next 9/9] selftests/bpf: convert send_signal.c to use subtests Andrii Nakryiko
8 siblings, 0 replies; 28+ messages in thread
From: Andrii Nakryiko @ 2019-07-26 20:37 UTC (permalink / raw)
To: bpf, netdev, ast, daniel; +Cc: andrii.nakryiko, kernel-team, Andrii Nakryiko
Expose each BPF verifier scale test as individual sub-test to allow
independent results output and test selection.
Test run results now look like this:
$ sudo ./test_progs -t verif/
#3/1 loop3.o:OK
#3/2 test_verif_scale1.o:OK
#3/3 test_verif_scale2.o:OK
#3/4 test_verif_scale3.o:OK
#3/5 pyperf50.o:OK
#3/6 pyperf100.o:OK
#3/7 pyperf180.o:OK
#3/8 pyperf600.o:OK
#3/9 pyperf600_nounroll.o:OK
#3/10 loop1.o:OK
#3/11 loop2.o:OK
#3/12 strobemeta.o:OK
#3/13 strobemeta_nounroll1.o:OK
#3/14 strobemeta_nounroll2.o:OK
#3/15 test_sysctl_loop1.o:OK
#3/16 test_sysctl_loop2.o:OK
#3/17 test_xdp_loop.o:OK
#3/18 test_seg6_loop.o:OK
#3 bpf_verif_scale:OK
Signed-off-by: Andrii Nakryiko <andriin@fb.com>
---
.../bpf/prog_tests/bpf_verif_scale.c | 77 ++++++++++---------
1 file changed, 40 insertions(+), 37 deletions(-)
diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_verif_scale.c b/tools/testing/selftests/bpf/prog_tests/bpf_verif_scale.c
index 95f012c417fe..a77c52395a2f 100644
--- a/tools/testing/selftests/bpf/prog_tests/bpf_verif_scale.c
+++ b/tools/testing/selftests/bpf/prog_tests/bpf_verif_scale.c
@@ -33,14 +33,25 @@ static int check_load(const char *file, enum bpf_prog_type type)
return err;
}
+struct scale_test_def {
+ const char *file;
+ enum bpf_prog_type attach_type;
+ bool fails;
+};
+
void test_bpf_verif_scale(void)
{
- const char *sched_cls[] = {
- "./test_verif_scale1.o", "./test_verif_scale2.o", "./test_verif_scale3.o",
- };
- const char *raw_tp[] = {
+ struct scale_test_def tests[] = {
+ { "loop3.o", BPF_PROG_TYPE_RAW_TRACEPOINT, true /* fails */ },
+
+ { "test_verif_scale1.o", BPF_PROG_TYPE_SCHED_CLS },
+ { "test_verif_scale2.o", BPF_PROG_TYPE_SCHED_CLS },
+ { "test_verif_scale3.o", BPF_PROG_TYPE_SCHED_CLS },
+
/* full unroll by llvm */
- "./pyperf50.o", "./pyperf100.o", "./pyperf180.o",
+ { "pyperf50.o", BPF_PROG_TYPE_RAW_TRACEPOINT },
+ { "pyperf100.o", BPF_PROG_TYPE_RAW_TRACEPOINT },
+ { "pyperf180.o", BPF_PROG_TYPE_RAW_TRACEPOINT },
/* partial unroll. llvm will unroll loop ~150 times.
* C loop count -> 600.
@@ -48,7 +59,7 @@ void test_bpf_verif_scale(void)
* 16k insns in loop body.
* Total of 5 such loops. Total program size ~82k insns.
*/
- "./pyperf600.o",
+ { "pyperf600.o", BPF_PROG_TYPE_RAW_TRACEPOINT },
/* no unroll at all.
* C loop count -> 600.
@@ -56,22 +67,26 @@ void test_bpf_verif_scale(void)
* ~110 insns in loop body.
* Total of 5 such loops. Total program size ~1500 insns.
*/
- "./pyperf600_nounroll.o",
+ { "pyperf600_nounroll.o", BPF_PROG_TYPE_RAW_TRACEPOINT },
- "./loop1.o", "./loop2.o",
+ { "loop1.o", BPF_PROG_TYPE_RAW_TRACEPOINT },
+ { "loop2.o", BPF_PROG_TYPE_RAW_TRACEPOINT },
/* partial unroll. 19k insn in a loop.
* Total program size 20.8k insn.
* ~350k processed_insns
*/
- "./strobemeta.o",
+ { "strobemeta.o", BPF_PROG_TYPE_RAW_TRACEPOINT },
/* no unroll, tiny loops */
- "./strobemeta_nounroll1.o",
- "./strobemeta_nounroll2.o",
- };
- const char *cg_sysctl[] = {
- "./test_sysctl_loop1.o", "./test_sysctl_loop2.o",
+ { "strobemeta_nounroll1.o", BPF_PROG_TYPE_RAW_TRACEPOINT },
+ { "strobemeta_nounroll2.o", BPF_PROG_TYPE_RAW_TRACEPOINT },
+
+ { "test_sysctl_loop1.o", BPF_PROG_TYPE_CGROUP_SYSCTL },
+ { "test_sysctl_loop2.o", BPF_PROG_TYPE_CGROUP_SYSCTL },
+
+ { "test_xdp_loop.o", BPF_PROG_TYPE_XDP },
+ { "test_seg6_loop.o", BPF_PROG_TYPE_LWT_SEG6LOCAL },
};
libbpf_print_fn_t old_print_fn = NULL;
int err, i;
@@ -81,33 +96,21 @@ void test_bpf_verif_scale(void)
old_print_fn = libbpf_swap_print(libbpf_debug_print);
}
- err = check_load("./loop3.o", BPF_PROG_TYPE_RAW_TRACEPOINT);
- test__printf("test_scale:loop3:%s\n",
- err ? (error_cnt--, "OK") : "FAIL");
+ for (i = 0; i < ARRAY_SIZE(tests); i++) {
+ const struct scale_test_def *test = &tests[i];
- for (i = 0; i < ARRAY_SIZE(sched_cls); i++) {
- err = check_load(sched_cls[i], BPF_PROG_TYPE_SCHED_CLS);
- test__printf("test_scale:%s:%s\n", sched_cls[i],
- err ? "FAIL" : "OK");
- }
+ if (!test__start_subtest(test->file))
+ continue;
- for (i = 0; i < ARRAY_SIZE(raw_tp); i++) {
- err = check_load(raw_tp[i], BPF_PROG_TYPE_RAW_TRACEPOINT);
- test__printf("test_scale:%s:%s\n", raw_tp[i],
- err ? "FAIL" : "OK");
+ err = check_load(test->file, test->attach_type);
+ if (test->fails) { /* expected to fail */
+ if (err)
+ error_cnt--;
+ else
+ error_cnt++;
+ }
}
- for (i = 0; i < ARRAY_SIZE(cg_sysctl); i++) {
- err = check_load(cg_sysctl[i], BPF_PROG_TYPE_CGROUP_SYSCTL);
- test__printf("test_scale:%s:%s\n", cg_sysctl[i],
- err ? "FAIL" : "OK");
- }
- err = check_load("./test_xdp_loop.o", BPF_PROG_TYPE_XDP);
- test__printf("test_scale:test_xdp_loop:%s\n", err ? "FAIL" : "OK");
-
- err = check_load("./test_seg6_loop.o", BPF_PROG_TYPE_LWT_SEG6LOCAL);
- test__printf("test_scale:test_seg6_loop:%s\n", err ? "FAIL" : "OK");
-
if (env.verifier_stats)
libbpf_set_print(old_print_fn);
}
--
2.17.1
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH bpf-next 9/9] selftests/bpf: convert send_signal.c to use subtests
2019-07-26 20:37 [PATCH bpf-next 0/9] Revamp test_progs as a test running framework Andrii Nakryiko
` (7 preceding siblings ...)
2019-07-26 20:37 ` [PATCH bpf-next 8/9] selftests/bpf: convert bpf_verif_scale.c to sub-tests API Andrii Nakryiko
@ 2019-07-26 20:37 ` Andrii Nakryiko
8 siblings, 0 replies; 28+ messages in thread
From: Andrii Nakryiko @ 2019-07-26 20:37 UTC (permalink / raw)
To: bpf, netdev, ast, daniel; +Cc: andrii.nakryiko, kernel-team, Andrii Nakryiko
Convert send_signal set of tests to be exposed as three sub-tests.
Signed-off-by: Andrii Nakryiko <andriin@fb.com>
---
tools/testing/selftests/bpf/prog_tests/send_signal.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/tools/testing/selftests/bpf/prog_tests/send_signal.c b/tools/testing/selftests/bpf/prog_tests/send_signal.c
index d950f4558897..461b423d0584 100644
--- a/tools/testing/selftests/bpf/prog_tests/send_signal.c
+++ b/tools/testing/selftests/bpf/prog_tests/send_signal.c
@@ -219,7 +219,10 @@ void test_send_signal(void)
{
int ret = 0;
- ret |= test_send_signal_tracepoint();
- ret |= test_send_signal_perf();
- ret |= test_send_signal_nmi();
+ if (test__start_subtest("send_signal_tracepoint"))
+ ret |= test_send_signal_tracepoint();
+ if (test__start_subtest("send_signal_perf"))
+ ret |= test_send_signal_perf();
+ if (test__start_subtest("send_signal_nmi"))
+ ret |= test_send_signal_nmi();
}
--
2.17.1
^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH bpf-next 1/9] selftests/bpf: prevent headers to be compiled as C code
2019-07-26 20:37 ` [PATCH bpf-next 1/9] selftests/bpf: prevent headers to be compiled as C code Andrii Nakryiko
@ 2019-07-26 21:21 ` Stanislav Fomichev
2019-07-26 21:42 ` Andrii Nakryiko
0 siblings, 1 reply; 28+ messages in thread
From: Stanislav Fomichev @ 2019-07-26 21:21 UTC (permalink / raw)
To: Andrii Nakryiko; +Cc: bpf, netdev, ast, daniel, andrii.nakryiko, kernel-team
On 07/26, Andrii Nakryiko wrote:
> Apprently listing header as a normal dependency for a binary output
> makes it go through compilation as if it was C code. This currently
> works without a problem, but in subsequent commits causes problems for
> differently generated test.h for test_progs. Marking those headers as
> order-only dependency solves the issue.
Are you sure it will not result in a situation where
test_progs/test_maps is not regenerated if tests.h is updated.
If I read the following doc correctly, order deps make sense for
directories only:
https://www.gnu.org/software/make/manual/html_node/Prerequisite-Types.html
Can you maybe double check it with:
* make
* add new prog_tests/test_something.c
* make
to see if the binary is regenerated with test_something.c?
Maybe fix the problem of header compilation by having '#ifndef
DECLARE_TEST #define DECLARE_TEST() #endif' in tests.h instead?
> Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> ---
> tools/testing/selftests/bpf/Makefile | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> index 11c9c62c3362..bb66cc4a7f34 100644
> --- a/tools/testing/selftests/bpf/Makefile
> +++ b/tools/testing/selftests/bpf/Makefile
> @@ -235,7 +235,7 @@ PROG_TESTS_H := $(PROG_TESTS_DIR)/tests.h
> PROG_TESTS_FILES := $(wildcard prog_tests/*.c)
> test_progs.c: $(PROG_TESTS_H)
> $(OUTPUT)/test_progs: CFLAGS += $(TEST_PROGS_CFLAGS)
> -$(OUTPUT)/test_progs: test_progs.c $(PROG_TESTS_H) $(PROG_TESTS_FILES)
> +$(OUTPUT)/test_progs: test_progs.c $(PROG_TESTS_FILES) | $(PROG_TESTS_H)
> $(PROG_TESTS_H): $(PROG_TESTS_FILES) | $(PROG_TESTS_DIR)
> $(shell ( cd prog_tests/; \
> echo '/* Generated header, do not edit */'; \
> @@ -256,7 +256,7 @@ MAP_TESTS_H := $(MAP_TESTS_DIR)/tests.h
> MAP_TESTS_FILES := $(wildcard map_tests/*.c)
> test_maps.c: $(MAP_TESTS_H)
> $(OUTPUT)/test_maps: CFLAGS += $(TEST_MAPS_CFLAGS)
> -$(OUTPUT)/test_maps: test_maps.c $(MAP_TESTS_H) $(MAP_TESTS_FILES)
> +$(OUTPUT)/test_maps: test_maps.c $(MAP_TESTS_FILES) | $(MAP_TESTS_H)
> $(MAP_TESTS_H): $(MAP_TESTS_FILES) | $(MAP_TESTS_DIR)
> $(shell ( cd map_tests/; \
> echo '/* Generated header, do not edit */'; \
> @@ -277,7 +277,7 @@ VERIFIER_TESTS_H := $(VERIFIER_TESTS_DIR)/tests.h
> VERIFIER_TEST_FILES := $(wildcard verifier/*.c)
> test_verifier.c: $(VERIFIER_TESTS_H)
> $(OUTPUT)/test_verifier: CFLAGS += $(TEST_VERIFIER_CFLAGS)
> -$(OUTPUT)/test_verifier: test_verifier.c $(VERIFIER_TESTS_H)
> +$(OUTPUT)/test_verifier: test_verifier.c | $(VERIFIER_TEST_FILES) $(VERIFIER_TESTS_H)
> $(VERIFIER_TESTS_H): $(VERIFIER_TEST_FILES) | $(VERIFIER_TESTS_DIR)
> $(shell ( cd verifier/; \
> echo '/* Generated header, do not edit */'; \
> --
> 2.17.1
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH bpf-next 3/9] selftests/bpf: add test selectors by number and name to test_progs
2019-07-26 20:37 ` [PATCH bpf-next 3/9] selftests/bpf: add test selectors by number and name to test_progs Andrii Nakryiko
@ 2019-07-26 21:25 ` Stanislav Fomichev
2019-07-26 21:45 ` Andrii Nakryiko
0 siblings, 1 reply; 28+ messages in thread
From: Stanislav Fomichev @ 2019-07-26 21:25 UTC (permalink / raw)
To: Andrii Nakryiko; +Cc: bpf, netdev, ast, daniel, andrii.nakryiko, kernel-team
On 07/26, Andrii Nakryiko wrote:
> Add ability to specify either test number or test name substring to
> narrow down a set of test to run.
>
> Usage:
> sudo ./test_progs -n 1
> sudo ./test_progs -t attach_probe
>
> Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> ---
> tools/testing/selftests/bpf/test_progs.c | 43 +++++++++++++++++++++---
> 1 file changed, 39 insertions(+), 4 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
> index eea88ba59225..6e04b9f83777 100644
> --- a/tools/testing/selftests/bpf/test_progs.c
> +++ b/tools/testing/selftests/bpf/test_progs.c
> @@ -4,6 +4,7 @@
> #include "test_progs.h"
> #include "bpf_rlimit.h"
> #include <argp.h>
> +#include <string.h>
>
> int error_cnt, pass_cnt;
> bool jit_enabled;
> @@ -164,6 +165,7 @@ void *spin_lock_thread(void *arg)
>
> struct prog_test_def {
> const char *test_name;
> + int test_num;
> void (*run_test)(void);
> };
>
> @@ -181,26 +183,49 @@ const char *argp_program_bug_address = "<bpf@vger.kernel.org>";
> const char argp_program_doc[] = "BPF selftests test runner";
>
> enum ARG_KEYS {
> + ARG_TEST_NUM = 'n',
> + ARG_TEST_NAME = 't',
> ARG_VERIFIER_STATS = 's',
> };
>
> static const struct argp_option opts[] = {
> + { "num", ARG_TEST_NUM, "NUM", 0,
> + "Run test number NUM only " },
> + { "name", ARG_TEST_NAME, "NAME", 0,
> + "Run tests with names containing NAME" },
> { "verifier-stats", ARG_VERIFIER_STATS, NULL, 0,
> "Output verifier statistics", },
> {},
> };
>
> struct test_env {
> + int test_num_selector;
> + const char *test_name_selector;
> bool verifier_stats;
> };
>
> -static struct test_env env = {};
> +static struct test_env env = {
> + .test_num_selector = -1,
> +};
>
> static error_t parse_arg(int key, char *arg, struct argp_state *state)
> {
> struct test_env *env = state->input;
>
> switch (key) {
[..]
> + case ARG_TEST_NUM: {
> + int test_num;
> +
> + errno = 0;
> + test_num = strtol(arg, NULL, 10);
> + if (errno)
> + return -errno;
> + env->test_num_selector = test_num;
> + break;
> + }
Do you think it's really useful? I agree about running by name (I
usually used grep -v in the Makefile :-), but I'm not sure about running
by number.
Or is the idea is that you can just copy-paste this number from the
test_progs output to rerun the tests? In this case, why not copy-paste
the name instead?
> + case ARG_TEST_NAME:
> + env->test_name_selector = arg;
> + break;
> case ARG_VERIFIER_STATS:
> env->verifier_stats = true;
> break;
> @@ -223,7 +248,7 @@ int main(int argc, char **argv)
> .parser = parse_arg,
> .doc = argp_program_doc,
> };
> - const struct prog_test_def *def;
> + struct prog_test_def *test;
> int err, i;
>
> err = argp_parse(&argp, argc, argv, 0, NULL, &env);
> @@ -237,8 +262,18 @@ int main(int argc, char **argv)
> verifier_stats = env.verifier_stats;
>
> for (i = 0; i < ARRAY_SIZE(prog_test_defs); i++) {
> - def = &prog_test_defs[i];
> - def->run_test();
> + test = &prog_test_defs[i];
> +
> + test->test_num = i + 1;
> +
> + if (env.test_num_selector >= 0 &&
> + test->test_num != env.test_num_selector)
> + continue;
> + if (env.test_name_selector &&
> + !strstr(test->test_name, env.test_name_selector))
> + continue;
> +
> + test->run_test();
> }
>
> printf("Summary: %d PASSED, %d FAILED\n", pass_cnt, error_cnt);
> --
> 2.17.1
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH bpf-next 4/9] libbpf: add libbpf_swap_print to get previous print func
2019-07-26 20:37 ` [PATCH bpf-next 4/9] libbpf: add libbpf_swap_print to get previous print func Andrii Nakryiko
@ 2019-07-26 21:28 ` Stanislav Fomichev
2019-07-26 21:47 ` Andrii Nakryiko
0 siblings, 1 reply; 28+ messages in thread
From: Stanislav Fomichev @ 2019-07-26 21:28 UTC (permalink / raw)
To: Andrii Nakryiko; +Cc: bpf, netdev, ast, daniel, andrii.nakryiko, kernel-team
On 07/26, Andrii Nakryiko wrote:
> libbpf_swap_print allows to restore previously set print function.
> This is useful when running many independent test with one default print
> function, but overriding log verbosity for particular subset of tests.
Can we change the return type of libbpf_set_print instead and return
the old function from it? Will it break ABI?
> Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> ---
> tools/lib/bpf/libbpf.c | 8 ++++++++
> tools/lib/bpf/libbpf.h | 1 +
> tools/lib/bpf/libbpf.map | 5 +++++
> 3 files changed, 14 insertions(+)
>
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 8741c39adb1c..0c254b6c9685 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -79,6 +79,14 @@ void libbpf_set_print(libbpf_print_fn_t fn)
> __libbpf_pr = fn;
> }
>
> +libbpf_print_fn_t libbpf_swap_print(libbpf_print_fn_t fn)
> +{
> + libbpf_print_fn_t old_print_fn = __libbpf_pr;
> +
> + __libbpf_pr = fn;
> + return old_print_fn;
> +}
> +
> __printf(2, 3)
> void libbpf_print(enum libbpf_print_level level, const char *format, ...)
> {
> diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> index 5cbf459ece0b..4e0aa893571f 100644
> --- a/tools/lib/bpf/libbpf.h
> +++ b/tools/lib/bpf/libbpf.h
> @@ -58,6 +58,7 @@ typedef int (*libbpf_print_fn_t)(enum libbpf_print_level level,
> const char *, va_list ap);
>
> LIBBPF_API void libbpf_set_print(libbpf_print_fn_t fn);
> +LIBBPF_API libbpf_print_fn_t libbpf_swap_print(libbpf_print_fn_t fn);
>
> /* Hide internal to user */
> struct bpf_object;
> diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
> index f9d316e873d8..e211c38ddc43 100644
> --- a/tools/lib/bpf/libbpf.map
> +++ b/tools/lib/bpf/libbpf.map
> @@ -184,3 +184,8 @@ LIBBPF_0.0.4 {
> perf_buffer__new_raw;
> perf_buffer__poll;
> } LIBBPF_0.0.3;
> +
> +LIBBPF_0.0.5 {
> + global:
> + libbpf_swap_print;
> +} LIBBPF_0.0.4;
> --
> 2.17.1
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH bpf-next 6/9] selftests/bpf: abstract away test log output
2019-07-26 20:37 ` [PATCH bpf-next 6/9] selftests/bpf: abstract away test log output Andrii Nakryiko
@ 2019-07-26 21:31 ` Stanislav Fomichev
2019-07-26 21:51 ` Andrii Nakryiko
0 siblings, 1 reply; 28+ messages in thread
From: Stanislav Fomichev @ 2019-07-26 21:31 UTC (permalink / raw)
To: Andrii Nakryiko; +Cc: bpf, netdev, ast, daniel, andrii.nakryiko, kernel-team
On 07/26, Andrii Nakryiko wrote:
> This patch changes how test output is printed out. By default, if test
> had no errors, the only output will be a single line with test number,
> name, and verdict at the end, e.g.:
>
> #31 xdp:OK
>
> If test had any errors, all log output captured during test execution
> will be output after test completes.
>
> It's possible to force output of log with `-v` (`--verbose`) option, in
> which case output won't be buffered and will be output immediately.
>
> To support this, individual tests are required to use helper methods for
> logging: `test__printf()` and `test__vprintf()`.
>
> Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> ---
> .../selftests/bpf/prog_tests/bpf_obj_id.c | 6 +-
> .../bpf/prog_tests/bpf_verif_scale.c | 31 ++--
> .../bpf/prog_tests/get_stack_raw_tp.c | 4 +-
> .../selftests/bpf/prog_tests/l4lb_all.c | 2 +-
> .../selftests/bpf/prog_tests/map_lock.c | 10 +-
> .../selftests/bpf/prog_tests/send_signal.c | 8 +-
> .../selftests/bpf/prog_tests/spinlock.c | 2 +-
> .../bpf/prog_tests/stacktrace_build_id.c | 4 +-
> .../bpf/prog_tests/stacktrace_build_id_nmi.c | 4 +-
> .../selftests/bpf/prog_tests/xdp_noinline.c | 3 +-
> tools/testing/selftests/bpf/test_progs.c | 135 +++++++++++++-----
> tools/testing/selftests/bpf/test_progs.h | 37 ++++-
> 12 files changed, 173 insertions(+), 73 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_obj_id.c b/tools/testing/selftests/bpf/prog_tests/bpf_obj_id.c
> index cb827383db4d..fb5840a62548 100644
> --- a/tools/testing/selftests/bpf/prog_tests/bpf_obj_id.c
> +++ b/tools/testing/selftests/bpf/prog_tests/bpf_obj_id.c
> @@ -106,8 +106,8 @@ void test_bpf_obj_id(void)
> if (CHECK(err ||
> prog_infos[i].type != BPF_PROG_TYPE_SOCKET_FILTER ||
> info_len != sizeof(struct bpf_prog_info) ||
> - (jit_enabled && !prog_infos[i].jited_prog_len) ||
> - (jit_enabled &&
> + (env.jit_enabled && !prog_infos[i].jited_prog_len) ||
> + (env.jit_enabled &&
> !memcmp(jited_insns, zeros, sizeof(zeros))) ||
> !prog_infos[i].xlated_prog_len ||
> !memcmp(xlated_insns, zeros, sizeof(zeros)) ||
> @@ -121,7 +121,7 @@ void test_bpf_obj_id(void)
> err, errno, i,
> prog_infos[i].type, BPF_PROG_TYPE_SOCKET_FILTER,
> info_len, sizeof(struct bpf_prog_info),
> - jit_enabled,
> + env.jit_enabled,
> prog_infos[i].jited_prog_len,
> prog_infos[i].xlated_prog_len,
> !!memcmp(jited_insns, zeros, sizeof(zeros)),
> diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_verif_scale.c b/tools/testing/selftests/bpf/prog_tests/bpf_verif_scale.c
> index 2c4d9ef099b4..95f012c417fe 100644
> --- a/tools/testing/selftests/bpf/prog_tests/bpf_verif_scale.c
> +++ b/tools/testing/selftests/bpf/prog_tests/bpf_verif_scale.c
> @@ -4,12 +4,15 @@
> static int libbpf_debug_print(enum libbpf_print_level level,
> const char *format, va_list args)
> {
> - if (level != LIBBPF_DEBUG)
> - return vfprintf(stderr, format, args);
> + if (level != LIBBPF_DEBUG) {
> + test__vprintf(format, args);
> + return 0;
> + }
>
> if (!strstr(format, "verifier log"))
> return 0;
> - return vfprintf(stderr, "%s", args);
> + test__vprintf("%s", args);
> + return 0;
> }
>
> static int check_load(const char *file, enum bpf_prog_type type)
> @@ -73,32 +76,38 @@ void test_bpf_verif_scale(void)
> libbpf_print_fn_t old_print_fn = NULL;
> int err, i;
>
> - if (verifier_stats)
> + if (env.verifier_stats) {
> + test__force_log();
> old_print_fn = libbpf_swap_print(libbpf_debug_print);
> + }
>
> err = check_load("./loop3.o", BPF_PROG_TYPE_RAW_TRACEPOINT);
> - printf("test_scale:loop3:%s\n", err ? (error_cnt--, "OK") : "FAIL");
> + test__printf("test_scale:loop3:%s\n",
> + err ? (error_cnt--, "OK") : "FAIL");
>
> for (i = 0; i < ARRAY_SIZE(sched_cls); i++) {
> err = check_load(sched_cls[i], BPF_PROG_TYPE_SCHED_CLS);
> - printf("test_scale:%s:%s\n", sched_cls[i], err ? "FAIL" : "OK");
> + test__printf("test_scale:%s:%s\n", sched_cls[i],
> + err ? "FAIL" : "OK");
> }
>
> for (i = 0; i < ARRAY_SIZE(raw_tp); i++) {
> err = check_load(raw_tp[i], BPF_PROG_TYPE_RAW_TRACEPOINT);
> - printf("test_scale:%s:%s\n", raw_tp[i], err ? "FAIL" : "OK");
> + test__printf("test_scale:%s:%s\n", raw_tp[i],
> + err ? "FAIL" : "OK");
> }
>
> for (i = 0; i < ARRAY_SIZE(cg_sysctl); i++) {
> err = check_load(cg_sysctl[i], BPF_PROG_TYPE_CGROUP_SYSCTL);
> - printf("test_scale:%s:%s\n", cg_sysctl[i], err ? "FAIL" : "OK");
> + test__printf("test_scale:%s:%s\n", cg_sysctl[i],
> + err ? "FAIL" : "OK");
> }
> err = check_load("./test_xdp_loop.o", BPF_PROG_TYPE_XDP);
> - printf("test_scale:test_xdp_loop:%s\n", err ? "FAIL" : "OK");
> + test__printf("test_scale:test_xdp_loop:%s\n", err ? "FAIL" : "OK");
>
> err = check_load("./test_seg6_loop.o", BPF_PROG_TYPE_LWT_SEG6LOCAL);
> - printf("test_scale:test_seg6_loop:%s\n", err ? "FAIL" : "OK");
> + test__printf("test_scale:test_seg6_loop:%s\n", err ? "FAIL" : "OK");
>
> - if (verifier_stats)
> + if (env.verifier_stats)
> libbpf_set_print(old_print_fn);
> }
> diff --git a/tools/testing/selftests/bpf/prog_tests/get_stack_raw_tp.c b/tools/testing/selftests/bpf/prog_tests/get_stack_raw_tp.c
> index 9d73a8f932ac..3d59b3c841fe 100644
> --- a/tools/testing/selftests/bpf/prog_tests/get_stack_raw_tp.c
> +++ b/tools/testing/selftests/bpf/prog_tests/get_stack_raw_tp.c
> @@ -41,7 +41,7 @@ static void get_stack_print_output(void *ctx, int cpu, void *data, __u32 size)
> * just assume it is good if the stack is not empty.
> * This could be improved in the future.
> */
> - if (jit_enabled) {
> + if (env.jit_enabled) {
> found = num_stack > 0;
> } else {
> for (i = 0; i < num_stack; i++) {
> @@ -58,7 +58,7 @@ static void get_stack_print_output(void *ctx, int cpu, void *data, __u32 size)
> }
> } else {
> num_stack = e->kern_stack_size / sizeof(__u64);
> - if (jit_enabled) {
> + if (env.jit_enabled) {
> good_kern_stack = num_stack > 0;
> } else {
> for (i = 0; i < num_stack; i++) {
> diff --git a/tools/testing/selftests/bpf/prog_tests/l4lb_all.c b/tools/testing/selftests/bpf/prog_tests/l4lb_all.c
> index 20ddca830e68..5ce572c03a5f 100644
> --- a/tools/testing/selftests/bpf/prog_tests/l4lb_all.c
> +++ b/tools/testing/selftests/bpf/prog_tests/l4lb_all.c
> @@ -74,7 +74,7 @@ static void test_l4lb(const char *file)
> }
> if (bytes != MAGIC_BYTES * NUM_ITER * 2 || pkts != NUM_ITER * 2) {
> error_cnt++;
> - printf("test_l4lb:FAIL:stats %lld %lld\n", bytes, pkts);
> + test__printf("test_l4lb:FAIL:stats %lld %lld\n", bytes, pkts);
#define printf(...) test__printf(...) in tests.h?
A bit ugly, but no need to retrain everyone to use new printf wrappers.
> }
> out:
> bpf_object__close(obj);
> diff --git a/tools/testing/selftests/bpf/prog_tests/map_lock.c b/tools/testing/selftests/bpf/prog_tests/map_lock.c
> index ee99368c595c..2e78217ed3fd 100644
> --- a/tools/testing/selftests/bpf/prog_tests/map_lock.c
> +++ b/tools/testing/selftests/bpf/prog_tests/map_lock.c
> @@ -9,12 +9,12 @@ static void *parallel_map_access(void *arg)
> for (i = 0; i < 10000; i++) {
> err = bpf_map_lookup_elem_flags(map_fd, &key, vars, BPF_F_LOCK);
> if (err) {
> - printf("lookup failed\n");
> + test__printf("lookup failed\n");
> error_cnt++;
> goto out;
> }
> if (vars[0] != 0) {
> - printf("lookup #%d var[0]=%d\n", i, vars[0]);
> + test__printf("lookup #%d var[0]=%d\n", i, vars[0]);
> error_cnt++;
> goto out;
> }
> @@ -22,8 +22,8 @@ static void *parallel_map_access(void *arg)
> for (j = 2; j < 17; j++) {
> if (vars[j] == rnd)
> continue;
> - printf("lookup #%d var[1]=%d var[%d]=%d\n",
> - i, rnd, j, vars[j]);
> + test__printf("lookup #%d var[1]=%d var[%d]=%d\n",
> + i, rnd, j, vars[j]);
> error_cnt++;
> goto out;
> }
> @@ -43,7 +43,7 @@ void test_map_lock(void)
>
> err = bpf_prog_load(file, BPF_PROG_TYPE_CGROUP_SKB, &obj, &prog_fd);
> if (err) {
> - printf("test_map_lock:bpf_prog_load errno %d\n", errno);
> + test__printf("test_map_lock:bpf_prog_load errno %d\n", errno);
> goto close_prog;
> }
> map_fd[0] = bpf_find_map(__func__, obj, "hash_map");
> diff --git a/tools/testing/selftests/bpf/prog_tests/send_signal.c b/tools/testing/selftests/bpf/prog_tests/send_signal.c
> index 54218ee3c004..d950f4558897 100644
> --- a/tools/testing/selftests/bpf/prog_tests/send_signal.c
> +++ b/tools/testing/selftests/bpf/prog_tests/send_signal.c
> @@ -202,8 +202,8 @@ static int test_send_signal_nmi(void)
> -1 /* cpu */, -1 /* group_fd */, 0 /* flags */);
> if (pmu_fd == -1) {
> if (errno == ENOENT) {
> - printf("%s:SKIP:no PERF_COUNT_HW_CPU_CYCLES\n",
> - __func__);
> + test__printf("%s:SKIP:no PERF_COUNT_HW_CPU_CYCLES\n",
> + __func__);
> return 0;
> }
> /* Let the test fail with a more informative message */
> @@ -222,8 +222,4 @@ void test_send_signal(void)
> ret |= test_send_signal_tracepoint();
> ret |= test_send_signal_perf();
> ret |= test_send_signal_nmi();
> - if (!ret)
> - printf("test_send_signal:OK\n");
> - else
> - printf("test_send_signal:FAIL\n");
> }
> diff --git a/tools/testing/selftests/bpf/prog_tests/spinlock.c b/tools/testing/selftests/bpf/prog_tests/spinlock.c
> index 114ebe6a438e..deb2db5b85b0 100644
> --- a/tools/testing/selftests/bpf/prog_tests/spinlock.c
> +++ b/tools/testing/selftests/bpf/prog_tests/spinlock.c
> @@ -12,7 +12,7 @@ void test_spinlock(void)
>
> err = bpf_prog_load(file, BPF_PROG_TYPE_CGROUP_SKB, &obj, &prog_fd);
> if (err) {
> - printf("test_spin_lock:bpf_prog_load errno %d\n", errno);
> + test__printf("test_spin_lock:bpf_prog_load errno %d\n", errno);
> goto close_prog;
> }
> for (i = 0; i < 4; i++)
> diff --git a/tools/testing/selftests/bpf/prog_tests/stacktrace_build_id.c b/tools/testing/selftests/bpf/prog_tests/stacktrace_build_id.c
> index ac44fda84833..356d2c017a9c 100644
> --- a/tools/testing/selftests/bpf/prog_tests/stacktrace_build_id.c
> +++ b/tools/testing/selftests/bpf/prog_tests/stacktrace_build_id.c
> @@ -109,8 +109,8 @@ void test_stacktrace_build_id(void)
> if (build_id_matches < 1 && retry--) {
> bpf_link__destroy(link);
> bpf_object__close(obj);
> - printf("%s:WARN:Didn't find expected build ID from the map, retrying\n",
> - __func__);
> + test__printf("%s:WARN:Didn't find expected build ID from the map, retrying\n",
> + __func__);
> goto retry;
> }
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/stacktrace_build_id_nmi.c b/tools/testing/selftests/bpf/prog_tests/stacktrace_build_id_nmi.c
> index 9557b7dfb782..f44f2c159714 100644
> --- a/tools/testing/selftests/bpf/prog_tests/stacktrace_build_id_nmi.c
> +++ b/tools/testing/selftests/bpf/prog_tests/stacktrace_build_id_nmi.c
> @@ -140,8 +140,8 @@ void test_stacktrace_build_id_nmi(void)
> if (build_id_matches < 1 && retry--) {
> bpf_link__destroy(link);
> bpf_object__close(obj);
> - printf("%s:WARN:Didn't find expected build ID from the map, retrying\n",
> - __func__);
> + test__printf("%s:WARN:Didn't find expected build ID from the map, retrying\n",
> + __func__);
> goto retry;
> }
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/xdp_noinline.c b/tools/testing/selftests/bpf/prog_tests/xdp_noinline.c
> index 09e6b46f5515..b5404494b8aa 100644
> --- a/tools/testing/selftests/bpf/prog_tests/xdp_noinline.c
> +++ b/tools/testing/selftests/bpf/prog_tests/xdp_noinline.c
> @@ -75,7 +75,8 @@ void test_xdp_noinline(void)
> }
> if (bytes != MAGIC_BYTES * NUM_ITER * 2 || pkts != NUM_ITER * 2) {
> error_cnt++;
> - printf("test_xdp_noinline:FAIL:stats %lld %lld\n", bytes, pkts);
> + test__printf("test_xdp_noinline:FAIL:stats %lld %lld\n",
> + bytes, pkts);
> }
> out:
> bpf_object__close(obj);
> diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
> index 94b6951b90b3..3cf3ebda1d31 100644
> --- a/tools/testing/selftests/bpf/test_progs.c
> +++ b/tools/testing/selftests/bpf/test_progs.c
> @@ -6,9 +6,75 @@
> #include <argp.h>
> #include <string.h>
>
> +/* defined in test_progs.h */
> +struct test_env env = {
> + .test_num_selector = -1,
> +};
> int error_cnt, pass_cnt;
> -bool jit_enabled;
> -bool verifier_stats = false;
> +
> +struct prog_test_def {
> + const char *test_name;
> + int test_num;
> + void (*run_test)(void);
> + bool force_log;
> + int pass_cnt;
> + int error_cnt;
> + bool tested;
> +};
> +
> +void test__force_log() {
> + env.test->force_log = true;
> +}
> +
> +void test__vprintf(const char *fmt, va_list args)
> +{
> + size_t rem_sz;
> + int ret;
> +
> + if (env.verbose || (env.test && env.test->force_log)) {
> + vfprintf(stderr, fmt, args);
> + return;
> + }
> +
> +try_again:
> + rem_sz = env.log_cap - env.log_cnt;
> + if (rem_sz) {
> + ret = vsnprintf(env.log_buf + env.log_cnt, rem_sz, fmt, args);
> + if (ret < 0) {
> + fprintf(stderr, "failed to log message w/ fmt '%s'\n", fmt);
> + return;
> + }
> + }
> +
> + if (ret >= rem_sz) {
> + size_t new_sz = env.log_cap * 3 / 2;
> + char *new_buf;
> +
> + if (new_sz < 4096)
> + new_sz = 4096;
> +
> + new_buf = realloc(env.log_buf, new_sz);
> + if (!new_buf) {
> + fprintf(stderr, "failed to realloc log buffer: %d\n",
> + errno);
> + return;
> + }
> + env.log_buf = new_buf;
> + env.log_cap = new_sz;
> + goto try_again;
> + }
> +
> + env.log_cnt += ret + 1;
> +}
> +
> +void test__printf(const char *fmt, ...)
> +{
> + va_list args;
> +
> + va_start(args, fmt);
> + test__vprintf(fmt, args);
> + va_end(args);
> +}
>
> struct ipv4_packet pkt_v4 = {
> .eth.h_proto = __bpf_constant_htons(ETH_P_IP),
> @@ -163,20 +229,15 @@ void *spin_lock_thread(void *arg)
> #include <prog_tests/tests.h>
> #undef DEFINE_TEST
>
> -struct prog_test_def {
> - const char *test_name;
> - int test_num;
> - void (*run_test)(void);
> -};
> -
> static struct prog_test_def prog_test_defs[] = {
> -#define DEFINE_TEST(name) { \
> - .test_name = #name, \
> - .run_test = &test_##name, \
> +#define DEFINE_TEST(name) { \
> + .test_name = #name, \
> + .run_test = &test_##name, \
> },
> #include <prog_tests/tests.h>
> #undef DEFINE_TEST
> };
> +const int prog_test_cnt = ARRAY_SIZE(prog_test_defs);
>
> const char *argp_program_version = "test_progs 0.1";
> const char *argp_program_bug_address = "<bpf@vger.kernel.org>";
> @@ -186,7 +247,6 @@ enum ARG_KEYS {
> ARG_TEST_NUM = 'n',
> ARG_TEST_NAME = 't',
> ARG_VERIFIER_STATS = 's',
> -
> ARG_VERBOSE = 'v',
> };
>
> @@ -202,24 +262,13 @@ static const struct argp_option opts[] = {
> {},
> };
>
> -struct test_env {
> - int test_num_selector;
> - const char *test_name_selector;
> - bool verifier_stats;
> - bool verbose;
> - bool very_verbose;
> -};
> -
> -static struct test_env env = {
> - .test_num_selector = -1,
> -};
> -
> static int libbpf_print_fn(enum libbpf_print_level level,
> const char *format, va_list args)
> {
> if (!env.very_verbose && level == LIBBPF_DEBUG)
> return 0;
> - return vfprintf(stderr, format, args);
> + test__vprintf(format, args);
> + return 0;
> }
>
> static error_t parse_arg(int key, char *arg, struct argp_state *state)
> @@ -267,7 +316,6 @@ static error_t parse_arg(int key, char *arg, struct argp_state *state)
> return 0;
> }
>
> -
> int main(int argc, char **argv)
> {
> static const struct argp argp = {
> @@ -275,7 +323,6 @@ int main(int argc, char **argv)
> .parser = parse_arg,
> .doc = argp_program_doc,
> };
> - struct prog_test_def *test;
> int err, i;
>
> err = argp_parse(&argp, argc, argv, 0, NULL, &env);
> @@ -286,13 +333,14 @@ int main(int argc, char **argv)
>
> srand(time(NULL));
>
> - jit_enabled = is_jit_enabled();
> + env.jit_enabled = is_jit_enabled();
>
> - verifier_stats = env.verifier_stats;
> -
> - for (i = 0; i < ARRAY_SIZE(prog_test_defs); i++) {
> - test = &prog_test_defs[i];
> + for (i = 0; i < prog_test_cnt; i++) {
> + struct prog_test_def *test = &prog_test_defs[i];
> + int old_pass_cnt = pass_cnt;
> + int old_error_cnt = error_cnt;
>
> + env.test = test;
> test->test_num = i + 1;
>
> if (env.test_num_selector >= 0 &&
> @@ -303,8 +351,29 @@ int main(int argc, char **argv)
> continue;
>
> test->run_test();
> + test->tested = true;
> + test->pass_cnt = pass_cnt - old_pass_cnt;
> + test->error_cnt = error_cnt - old_error_cnt;
> + if (test->error_cnt)
> + env.fail_cnt++;
> + else
> + env.succ_cnt++;
> +
> + if (env.verbose || test->force_log || test->error_cnt) {
> + if (env.log_cnt) {
> + fprintf(stdout, "%s", env.log_buf);
> + if (env.log_buf[env.log_cnt - 1] != '\n')
> + fprintf(stdout, "\n");
> + }
> + }
> + env.log_cnt = 0;
> +
> + printf("#%d %s:%s\n", test->test_num, test->test_name,
> + test->error_cnt ? "FAIL" : "OK");
> }
> + printf("Summary: %d PASSED, %d FAILED\n", env.succ_cnt, env.fail_cnt);
> +
> + free(env.log_buf);
>
> - printf("Summary: %d PASSED, %d FAILED\n", pass_cnt, error_cnt);
> return error_cnt ? EXIT_FAILURE : EXIT_SUCCESS;
> }
> diff --git a/tools/testing/selftests/bpf/test_progs.h b/tools/testing/selftests/bpf/test_progs.h
> index 49e0f7d85643..62f55a4231e9 100644
> --- a/tools/testing/selftests/bpf/test_progs.h
> +++ b/tools/testing/selftests/bpf/test_progs.h
> @@ -38,9 +38,33 @@ typedef __u16 __sum16;
> #include "trace_helpers.h"
> #include "flow_dissector_load.h"
>
> -extern int error_cnt, pass_cnt;
> -extern bool jit_enabled;
> -extern bool verifier_stats;
> +struct prog_test_def;
> +
> +struct test_env {
> + int test_num_selector;
> + const char *test_name_selector;
> + bool verifier_stats;
> + bool verbose;
> + bool very_verbose;
> +
> + bool jit_enabled;
> +
> + struct prog_test_def *test;
> + char *log_buf;
> + size_t log_cnt;
> + size_t log_cap;
> +
> + int succ_cnt;
> + int fail_cnt;
> +};
> +
> +extern int error_cnt;
> +extern int pass_cnt;
> +extern struct test_env env;
> +
> +extern void test__printf(const char *fmt, ...);
> +extern void test__vprintf(const char *fmt, va_list args);
> +extern void test__force_log();
>
> #define MAGIC_BYTES 123
>
> @@ -64,11 +88,12 @@ extern struct ipv6_packet pkt_v6;
> int __ret = !!(condition); \
> if (__ret) { \
> error_cnt++; \
> - printf("%s:FAIL:%s ", __func__, tag); \
> - printf(format); \
> + test__printf("%s:FAIL:%s ", __func__, tag); \
> + test__printf(format); \
> } else { \
> pass_cnt++; \
> - printf("%s:PASS:%s %d nsec\n", __func__, tag, duration);\
> + test__printf("%s:PASS:%s %d nsec\n", \
> + __func__, tag, duration); \
> } \
> __ret; \
> })
> --
> 2.17.1
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH bpf-next 1/9] selftests/bpf: prevent headers to be compiled as C code
2019-07-26 21:21 ` Stanislav Fomichev
@ 2019-07-26 21:42 ` Andrii Nakryiko
2019-07-26 22:01 ` Stanislav Fomichev
0 siblings, 1 reply; 28+ messages in thread
From: Andrii Nakryiko @ 2019-07-26 21:42 UTC (permalink / raw)
To: Stanislav Fomichev
Cc: Andrii Nakryiko, bpf, Networking, Alexei Starovoitov,
Daniel Borkmann, Kernel Team
On Fri, Jul 26, 2019 at 2:21 PM Stanislav Fomichev <sdf@fomichev.me> wrote:
>
> On 07/26, Andrii Nakryiko wrote:
> > Apprently listing header as a normal dependency for a binary output
> > makes it go through compilation as if it was C code. This currently
> > works without a problem, but in subsequent commits causes problems for
> > differently generated test.h for test_progs. Marking those headers as
> > order-only dependency solves the issue.
> Are you sure it will not result in a situation where
> test_progs/test_maps is not regenerated if tests.h is updated.
>
> If I read the following doc correctly, order deps make sense for
> directories only:
> https://www.gnu.org/software/make/manual/html_node/Prerequisite-Types.html
>
> Can you maybe double check it with:
> * make
> * add new prog_tests/test_something.c
> * make
> to see if the binary is regenerated with test_something.c?
Yeah, tested that, it triggers test_progs rebuild.
Ordering is still preserved, because test.h is dependency of
test_progs.c, which is dependency of test_progs binary, so that's why
it works.
As to why .h file is compiled as C file, I have no idea and ideally
that should be fixed somehow.
I also started with just removing header as dependency completely
(because it's indirect dependency of test_progs.c), but that broke the
build logic. Dunno, too much magic... This works, tested many-many
times, so I was satisfied enough :)
>
> Maybe fix the problem of header compilation by having '#ifndef
> DECLARE_TEST #define DECLARE_TEST() #endif' in tests.h instead?
That's ugly, I'd like to avoid doing that.
>
> > Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> > ---
> > tools/testing/selftests/bpf/Makefile | 6 +++---
> > 1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> > index 11c9c62c3362..bb66cc4a7f34 100644
> > --- a/tools/testing/selftests/bpf/Makefile
> > +++ b/tools/testing/selftests/bpf/Makefile
> > @@ -235,7 +235,7 @@ PROG_TESTS_H := $(PROG_TESTS_DIR)/tests.h
> > PROG_TESTS_FILES := $(wildcard prog_tests/*.c)
> > test_progs.c: $(PROG_TESTS_H)
> > $(OUTPUT)/test_progs: CFLAGS += $(TEST_PROGS_CFLAGS)
> > -$(OUTPUT)/test_progs: test_progs.c $(PROG_TESTS_H) $(PROG_TESTS_FILES)
> > +$(OUTPUT)/test_progs: test_progs.c $(PROG_TESTS_FILES) | $(PROG_TESTS_H)
> > $(PROG_TESTS_H): $(PROG_TESTS_FILES) | $(PROG_TESTS_DIR)
> > $(shell ( cd prog_tests/; \
> > echo '/* Generated header, do not edit */'; \
> > @@ -256,7 +256,7 @@ MAP_TESTS_H := $(MAP_TESTS_DIR)/tests.h
> > MAP_TESTS_FILES := $(wildcard map_tests/*.c)
> > test_maps.c: $(MAP_TESTS_H)
> > $(OUTPUT)/test_maps: CFLAGS += $(TEST_MAPS_CFLAGS)
> > -$(OUTPUT)/test_maps: test_maps.c $(MAP_TESTS_H) $(MAP_TESTS_FILES)
> > +$(OUTPUT)/test_maps: test_maps.c $(MAP_TESTS_FILES) | $(MAP_TESTS_H)
> > $(MAP_TESTS_H): $(MAP_TESTS_FILES) | $(MAP_TESTS_DIR)
> > $(shell ( cd map_tests/; \
> > echo '/* Generated header, do not edit */'; \
> > @@ -277,7 +277,7 @@ VERIFIER_TESTS_H := $(VERIFIER_TESTS_DIR)/tests.h
> > VERIFIER_TEST_FILES := $(wildcard verifier/*.c)
> > test_verifier.c: $(VERIFIER_TESTS_H)
> > $(OUTPUT)/test_verifier: CFLAGS += $(TEST_VERIFIER_CFLAGS)
> > -$(OUTPUT)/test_verifier: test_verifier.c $(VERIFIER_TESTS_H)
> > +$(OUTPUT)/test_verifier: test_verifier.c | $(VERIFIER_TEST_FILES) $(VERIFIER_TESTS_H)
> > $(VERIFIER_TESTS_H): $(VERIFIER_TEST_FILES) | $(VERIFIER_TESTS_DIR)
> > $(shell ( cd verifier/; \
> > echo '/* Generated header, do not edit */'; \
> > --
> > 2.17.1
> >
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH bpf-next 3/9] selftests/bpf: add test selectors by number and name to test_progs
2019-07-26 21:25 ` Stanislav Fomichev
@ 2019-07-26 21:45 ` Andrii Nakryiko
2019-07-26 22:03 ` Stanislav Fomichev
0 siblings, 1 reply; 28+ messages in thread
From: Andrii Nakryiko @ 2019-07-26 21:45 UTC (permalink / raw)
To: Stanislav Fomichev
Cc: Andrii Nakryiko, bpf, Networking, Alexei Starovoitov,
Daniel Borkmann, Kernel Team
On Fri, Jul 26, 2019 at 2:25 PM Stanislav Fomichev <sdf@fomichev.me> wrote:
>
> On 07/26, Andrii Nakryiko wrote:
> > Add ability to specify either test number or test name substring to
> > narrow down a set of test to run.
> >
> > Usage:
> > sudo ./test_progs -n 1
> > sudo ./test_progs -t attach_probe
> >
> > Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> > ---
> > tools/testing/selftests/bpf/test_progs.c | 43 +++++++++++++++++++++---
> > 1 file changed, 39 insertions(+), 4 deletions(-)
> >
> > diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
> > index eea88ba59225..6e04b9f83777 100644
> > --- a/tools/testing/selftests/bpf/test_progs.c
> > +++ b/tools/testing/selftests/bpf/test_progs.c
> > @@ -4,6 +4,7 @@
[...]
> >
> > static error_t parse_arg(int key, char *arg, struct argp_state *state)
> > {
> > struct test_env *env = state->input;
> >
> > switch (key) {
> [..]
> > + case ARG_TEST_NUM: {
> > + int test_num;
> > +
> > + errno = 0;
> > + test_num = strtol(arg, NULL, 10);
> > + if (errno)
> > + return -errno;
> > + env->test_num_selector = test_num;
> > + break;
> > + }
> Do you think it's really useful? I agree about running by name (I
Special request from Alexei :) But in one of the follow up patches, I
extended this to allow to specify arbitrary subset of tests, e.g.:
1,2,5-10,7-8. So in that regard, it's more powerful than selecting by
name and gives you ultimate freedom.
> usually used grep -v in the Makefile :-), but I'm not sure about running
> by number.
>
> Or is the idea is that you can just copy-paste this number from the
> test_progs output to rerun the tests? In this case, why not copy-paste
> the name instead?
Both were simple to support, I didn't want to dictate one right way to
do this :)
>
> > + case ARG_TEST_NAME:
> > + env->test_name_selector = arg;
> > + break;
> > case ARG_VERIFIER_STATS:
> > env->verifier_stats = true;
> > break;
> > @@ -223,7 +248,7 @@ int main(int argc, char **argv)
> > .parser = parse_arg,
> > .doc = argp_program_doc,
> > };
> > - const struct prog_test_def *def;
> > + struct prog_test_def *test;
> > int err, i;
> >
> > err = argp_parse(&argp, argc, argv, 0, NULL, &env);
> > @@ -237,8 +262,18 @@ int main(int argc, char **argv)
> > verifier_stats = env.verifier_stats;
> >
> > for (i = 0; i < ARRAY_SIZE(prog_test_defs); i++) {
> > - def = &prog_test_defs[i];
> > - def->run_test();
> > + test = &prog_test_defs[i];
> > +
> > + test->test_num = i + 1;
> > +
> > + if (env.test_num_selector >= 0 &&
> > + test->test_num != env.test_num_selector)
> > + continue;
> > + if (env.test_name_selector &&
> > + !strstr(test->test_name, env.test_name_selector))
> > + continue;
> > +
> > + test->run_test();
> > }
> >
> > printf("Summary: %d PASSED, %d FAILED\n", pass_cnt, error_cnt);
> > --
> > 2.17.1
> >
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH bpf-next 4/9] libbpf: add libbpf_swap_print to get previous print func
2019-07-26 21:28 ` Stanislav Fomichev
@ 2019-07-26 21:47 ` Andrii Nakryiko
2019-07-27 0:30 ` Alexei Starovoitov
0 siblings, 1 reply; 28+ messages in thread
From: Andrii Nakryiko @ 2019-07-26 21:47 UTC (permalink / raw)
To: Stanislav Fomichev
Cc: Andrii Nakryiko, bpf, Networking, Alexei Starovoitov,
Daniel Borkmann, Kernel Team
On Fri, Jul 26, 2019 at 2:28 PM Stanislav Fomichev <sdf@fomichev.me> wrote:
>
> On 07/26, Andrii Nakryiko wrote:
> > libbpf_swap_print allows to restore previously set print function.
> > This is useful when running many independent test with one default print
> > function, but overriding log verbosity for particular subset of tests.
> Can we change the return type of libbpf_set_print instead and return
> the old function from it? Will it break ABI?
Yeah, thought about that, but I wasn't sure about ABI breakage. It
seems like it shouldn't, so I'll just change libbpf_set_print
signature instead.
>
> > Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> > ---
> > tools/lib/bpf/libbpf.c | 8 ++++++++
> > tools/lib/bpf/libbpf.h | 1 +
> > tools/lib/bpf/libbpf.map | 5 +++++
> > 3 files changed, 14 insertions(+)
> >
[...]
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH bpf-next 6/9] selftests/bpf: abstract away test log output
2019-07-26 21:31 ` Stanislav Fomichev
@ 2019-07-26 21:51 ` Andrii Nakryiko
2019-07-26 22:26 ` Stanislav Fomichev
0 siblings, 1 reply; 28+ messages in thread
From: Andrii Nakryiko @ 2019-07-26 21:51 UTC (permalink / raw)
To: Stanislav Fomichev
Cc: Andrii Nakryiko, bpf, Networking, Alexei Starovoitov,
Daniel Borkmann, Kernel Team
On Fri, Jul 26, 2019 at 2:31 PM Stanislav Fomichev <sdf@fomichev.me> wrote:
>
> On 07/26, Andrii Nakryiko wrote:
> > This patch changes how test output is printed out. By default, if test
> > had no errors, the only output will be a single line with test number,
> > name, and verdict at the end, e.g.:
> >
> > #31 xdp:OK
> >
> > If test had any errors, all log output captured during test execution
> > will be output after test completes.
> >
> > It's possible to force output of log with `-v` (`--verbose`) option, in
> > which case output won't be buffered and will be output immediately.
> >
> > To support this, individual tests are required to use helper methods for
> > logging: `test__printf()` and `test__vprintf()`.
> >
> > Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> > ---
> > .../selftests/bpf/prog_tests/bpf_obj_id.c | 6 +-
> > .../bpf/prog_tests/bpf_verif_scale.c | 31 ++--
> > .../bpf/prog_tests/get_stack_raw_tp.c | 4 +-
> > .../selftests/bpf/prog_tests/l4lb_all.c | 2 +-
> > .../selftests/bpf/prog_tests/map_lock.c | 10 +-
> > .../selftests/bpf/prog_tests/send_signal.c | 8 +-
> > .../selftests/bpf/prog_tests/spinlock.c | 2 +-
> > .../bpf/prog_tests/stacktrace_build_id.c | 4 +-
> > .../bpf/prog_tests/stacktrace_build_id_nmi.c | 4 +-
> > .../selftests/bpf/prog_tests/xdp_noinline.c | 3 +-
> > tools/testing/selftests/bpf/test_progs.c | 135 +++++++++++++-----
> > tools/testing/selftests/bpf/test_progs.h | 37 ++++-
> > 12 files changed, 173 insertions(+), 73 deletions(-)
> >
[...]
> > error_cnt++;
> > - printf("test_l4lb:FAIL:stats %lld %lld\n", bytes, pkts);
> > + test__printf("test_l4lb:FAIL:stats %lld %lld\n", bytes, pkts);
> #define printf(...) test__printf(...) in tests.h?
>
> A bit ugly, but no need to retrain everyone to use new printf wrappers.
I try to reduce amount of magic and surprising things, not add new
ones :) I also led by example and converted all current instances of
printf usage to test__printf, so anyone new will just copy/paste good
example, hopefully. Even if not, this non-buffered output will be
immediately obvious to anyone who just runs `sudo ./test_progs`. And
author of new test with this problem should hopefully be the first and
the only one to catch and fix this.
>
> > }
> > out:
> > bpf_object__close(obj);
> > diff --git a/tools/testing/selftests/bpf/prog_tests/map_lock.c b/tools/testing/selftests/bpf/prog_tests/map_lock.c
> > index ee99368c595c..2e78217ed3fd 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/map_lock.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/map_lock.c
> > @@ -9,12 +9,12 @@ static void *parallel_map_access(void *arg)
[...]
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH bpf-next 1/9] selftests/bpf: prevent headers to be compiled as C code
2019-07-26 21:42 ` Andrii Nakryiko
@ 2019-07-26 22:01 ` Stanislav Fomichev
2019-07-27 18:53 ` Andrii Nakryiko
0 siblings, 1 reply; 28+ messages in thread
From: Stanislav Fomichev @ 2019-07-26 22:01 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Andrii Nakryiko, bpf, Networking, Alexei Starovoitov,
Daniel Borkmann, Kernel Team
On 07/26, Andrii Nakryiko wrote:
> On Fri, Jul 26, 2019 at 2:21 PM Stanislav Fomichev <sdf@fomichev.me> wrote:
> >
> > On 07/26, Andrii Nakryiko wrote:
> > > Apprently listing header as a normal dependency for a binary output
> > > makes it go through compilation as if it was C code. This currently
> > > works without a problem, but in subsequent commits causes problems for
> > > differently generated test.h for test_progs. Marking those headers as
> > > order-only dependency solves the issue.
> > Are you sure it will not result in a situation where
> > test_progs/test_maps is not regenerated if tests.h is updated.
> >
> > If I read the following doc correctly, order deps make sense for
> > directories only:
> > https://www.gnu.org/software/make/manual/html_node/Prerequisite-Types.html
> >
> > Can you maybe double check it with:
> > * make
> > * add new prog_tests/test_something.c
> > * make
> > to see if the binary is regenerated with test_something.c?
>
> Yeah, tested that, it triggers test_progs rebuild.
>
> Ordering is still preserved, because test.h is dependency of
> test_progs.c, which is dependency of test_progs binary, so that's why
> it works.
>
> As to why .h file is compiled as C file, I have no idea and ideally
> that should be fixed somehow.
I guess that's because it's a prerequisite and we have a target that
puts all prerequisites when calling CC:
test_progs: a.c b.c tests.h
gcc a.c b.c tests.h -o test_progs
So gcc compiles each input file.
I'm not actually sure why default dependency system that uses 'gcc -M'
is not working for us (see scripts/Kbuild.include) and we need to manually
add tests.h dependency. But that's outside of the scope..
> I also started with just removing header as dependency completely
> (because it's indirect dependency of test_progs.c), but that broke the
> build logic. Dunno, too much magic... This works, tested many-many
> times, so I was satisfied enough :)
Yeah, that's my only concern, too much magic already and we add
quite a bit more.
> > Maybe fix the problem of header compilation by having '#ifndef
> > DECLARE_TEST #define DECLARE_TEST() #endif' in tests.h instead?
>
> That's ugly, I'd like to avoid doing that.
That's your call, but I'm not sure what's uglier: complicating already
complex make rules or making a header self contained.
> > > Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> > > ---
> > > tools/testing/selftests/bpf/Makefile | 6 +++---
> > > 1 file changed, 3 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> > > index 11c9c62c3362..bb66cc4a7f34 100644
> > > --- a/tools/testing/selftests/bpf/Makefile
> > > +++ b/tools/testing/selftests/bpf/Makefile
> > > @@ -235,7 +235,7 @@ PROG_TESTS_H := $(PROG_TESTS_DIR)/tests.h
> > > PROG_TESTS_FILES := $(wildcard prog_tests/*.c)
> > > test_progs.c: $(PROG_TESTS_H)
> > > $(OUTPUT)/test_progs: CFLAGS += $(TEST_PROGS_CFLAGS)
> > > -$(OUTPUT)/test_progs: test_progs.c $(PROG_TESTS_H) $(PROG_TESTS_FILES)
> > > +$(OUTPUT)/test_progs: test_progs.c $(PROG_TESTS_FILES) | $(PROG_TESTS_H)
> > > $(PROG_TESTS_H): $(PROG_TESTS_FILES) | $(PROG_TESTS_DIR)
> > > $(shell ( cd prog_tests/; \
> > > echo '/* Generated header, do not edit */'; \
> > > @@ -256,7 +256,7 @@ MAP_TESTS_H := $(MAP_TESTS_DIR)/tests.h
> > > MAP_TESTS_FILES := $(wildcard map_tests/*.c)
> > > test_maps.c: $(MAP_TESTS_H)
> > > $(OUTPUT)/test_maps: CFLAGS += $(TEST_MAPS_CFLAGS)
> > > -$(OUTPUT)/test_maps: test_maps.c $(MAP_TESTS_H) $(MAP_TESTS_FILES)
> > > +$(OUTPUT)/test_maps: test_maps.c $(MAP_TESTS_FILES) | $(MAP_TESTS_H)
> > > $(MAP_TESTS_H): $(MAP_TESTS_FILES) | $(MAP_TESTS_DIR)
> > > $(shell ( cd map_tests/; \
> > > echo '/* Generated header, do not edit */'; \
> > > @@ -277,7 +277,7 @@ VERIFIER_TESTS_H := $(VERIFIER_TESTS_DIR)/tests.h
> > > VERIFIER_TEST_FILES := $(wildcard verifier/*.c)
> > > test_verifier.c: $(VERIFIER_TESTS_H)
> > > $(OUTPUT)/test_verifier: CFLAGS += $(TEST_VERIFIER_CFLAGS)
> > > -$(OUTPUT)/test_verifier: test_verifier.c $(VERIFIER_TESTS_H)
> > > +$(OUTPUT)/test_verifier: test_verifier.c | $(VERIFIER_TEST_FILES) $(VERIFIER_TESTS_H)
> > > $(VERIFIER_TESTS_H): $(VERIFIER_TEST_FILES) | $(VERIFIER_TESTS_DIR)
> > > $(shell ( cd verifier/; \
> > > echo '/* Generated header, do not edit */'; \
> > > --
> > > 2.17.1
> > >
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH bpf-next 3/9] selftests/bpf: add test selectors by number and name to test_progs
2019-07-26 21:45 ` Andrii Nakryiko
@ 2019-07-26 22:03 ` Stanislav Fomichev
0 siblings, 0 replies; 28+ messages in thread
From: Stanislav Fomichev @ 2019-07-26 22:03 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Andrii Nakryiko, bpf, Networking, Alexei Starovoitov,
Daniel Borkmann, Kernel Team
On 07/26, Andrii Nakryiko wrote:
> On Fri, Jul 26, 2019 at 2:25 PM Stanislav Fomichev <sdf@fomichev.me> wrote:
> >
> > On 07/26, Andrii Nakryiko wrote:
> > > Add ability to specify either test number or test name substring to
> > > narrow down a set of test to run.
> > >
> > > Usage:
> > > sudo ./test_progs -n 1
> > > sudo ./test_progs -t attach_probe
> > >
> > > Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> > > ---
> > > tools/testing/selftests/bpf/test_progs.c | 43 +++++++++++++++++++++---
> > > 1 file changed, 39 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
> > > index eea88ba59225..6e04b9f83777 100644
> > > --- a/tools/testing/selftests/bpf/test_progs.c
> > > +++ b/tools/testing/selftests/bpf/test_progs.c
> > > @@ -4,6 +4,7 @@
>
> [...]
>
> > >
> > > static error_t parse_arg(int key, char *arg, struct argp_state *state)
> > > {
> > > struct test_env *env = state->input;
> > >
> > > switch (key) {
> > [..]
> > > + case ARG_TEST_NUM: {
> > > + int test_num;
> > > +
> > > + errno = 0;
> > > + test_num = strtol(arg, NULL, 10);
> > > + if (errno)
> > > + return -errno;
> > > + env->test_num_selector = test_num;
> > > + break;
> > > + }
> > Do you think it's really useful? I agree about running by name (I
>
> Special request from Alexei :) But in one of the follow up patches, I
> extended this to allow to specify arbitrary subset of tests, e.g.:
> 1,2,5-10,7-8. So in that regard, it's more powerful than selecting by
> name and gives you ultimate freedom.
I guess I didn't read the series close enough; that '1,2,3' mode does seem
quite useful indeed!
>
> > usually used grep -v in the Makefile :-), but I'm not sure about running
> > by number.
> >
> > Or is the idea is that you can just copy-paste this number from the
> > test_progs output to rerun the tests? In this case, why not copy-paste
> > the name instead?
>
> Both were simple to support, I didn't want to dictate one right way to
> do this :)
>
> >
> > > + case ARG_TEST_NAME:
> > > + env->test_name_selector = arg;
> > > + break;
> > > case ARG_VERIFIER_STATS:
> > > env->verifier_stats = true;
> > > break;
> > > @@ -223,7 +248,7 @@ int main(int argc, char **argv)
> > > .parser = parse_arg,
> > > .doc = argp_program_doc,
> > > };
> > > - const struct prog_test_def *def;
> > > + struct prog_test_def *test;
> > > int err, i;
> > >
> > > err = argp_parse(&argp, argc, argv, 0, NULL, &env);
> > > @@ -237,8 +262,18 @@ int main(int argc, char **argv)
> > > verifier_stats = env.verifier_stats;
> > >
> > > for (i = 0; i < ARRAY_SIZE(prog_test_defs); i++) {
> > > - def = &prog_test_defs[i];
> > > - def->run_test();
> > > + test = &prog_test_defs[i];
> > > +
> > > + test->test_num = i + 1;
> > > +
> > > + if (env.test_num_selector >= 0 &&
> > > + test->test_num != env.test_num_selector)
> > > + continue;
> > > + if (env.test_name_selector &&
> > > + !strstr(test->test_name, env.test_name_selector))
> > > + continue;
> > > +
> > > + test->run_test();
> > > }
> > >
> > > printf("Summary: %d PASSED, %d FAILED\n", pass_cnt, error_cnt);
> > > --
> > > 2.17.1
> > >
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH bpf-next 6/9] selftests/bpf: abstract away test log output
2019-07-26 21:51 ` Andrii Nakryiko
@ 2019-07-26 22:26 ` Stanislav Fomichev
2019-07-27 0:34 ` Alexei Starovoitov
2019-07-27 18:56 ` Andrii Nakryiko
0 siblings, 2 replies; 28+ messages in thread
From: Stanislav Fomichev @ 2019-07-26 22:26 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Andrii Nakryiko, bpf, Networking, Alexei Starovoitov,
Daniel Borkmann, Kernel Team
On 07/26, Andrii Nakryiko wrote:
> On Fri, Jul 26, 2019 at 2:31 PM Stanislav Fomichev <sdf@fomichev.me> wrote:
> >
> > On 07/26, Andrii Nakryiko wrote:
> > > This patch changes how test output is printed out. By default, if test
> > > had no errors, the only output will be a single line with test number,
> > > name, and verdict at the end, e.g.:
> > >
> > > #31 xdp:OK
> > >
> > > If test had any errors, all log output captured during test execution
> > > will be output after test completes.
> > >
> > > It's possible to force output of log with `-v` (`--verbose`) option, in
> > > which case output won't be buffered and will be output immediately.
> > >
> > > To support this, individual tests are required to use helper methods for
> > > logging: `test__printf()` and `test__vprintf()`.
> > >
> > > Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> > > ---
> > > .../selftests/bpf/prog_tests/bpf_obj_id.c | 6 +-
> > > .../bpf/prog_tests/bpf_verif_scale.c | 31 ++--
> > > .../bpf/prog_tests/get_stack_raw_tp.c | 4 +-
> > > .../selftests/bpf/prog_tests/l4lb_all.c | 2 +-
> > > .../selftests/bpf/prog_tests/map_lock.c | 10 +-
> > > .../selftests/bpf/prog_tests/send_signal.c | 8 +-
> > > .../selftests/bpf/prog_tests/spinlock.c | 2 +-
> > > .../bpf/prog_tests/stacktrace_build_id.c | 4 +-
> > > .../bpf/prog_tests/stacktrace_build_id_nmi.c | 4 +-
> > > .../selftests/bpf/prog_tests/xdp_noinline.c | 3 +-
> > > tools/testing/selftests/bpf/test_progs.c | 135 +++++++++++++-----
> > > tools/testing/selftests/bpf/test_progs.h | 37 ++++-
> > > 12 files changed, 173 insertions(+), 73 deletions(-)
> > >
>
> [...]
>
> > > error_cnt++;
> > > - printf("test_l4lb:FAIL:stats %lld %lld\n", bytes, pkts);
> > > + test__printf("test_l4lb:FAIL:stats %lld %lld\n", bytes, pkts);
> > #define printf(...) test__printf(...) in tests.h?
> >
> > A bit ugly, but no need to retrain everyone to use new printf wrappers.
>
> I try to reduce amount of magic and surprising things, not add new
> ones :) I also led by example and converted all current instances of
> printf usage to test__printf, so anyone new will just copy/paste good
> example, hopefully. Even if not, this non-buffered output will be
> immediately obvious to anyone who just runs `sudo ./test_progs`.
[..]
> And
> author of new test with this problem should hopefully be the first and
> the only one to catch and fix this.
Yeah, that is my only concern, that regular printfs will eventually
creep in. It's already confusing to go to/from printf/printk.
2c:
I'm coming from a perspective of tools/testing/selftests/kselftest.h
which is supposed to be a generic framework with custom
printf variants (ksft_print_msg), but I still see a bunch of tests
calling printf :-/
grep -ril ksft_exit_fail_msg selftests/ | xargs -n1 grep -w printf
Since we don't expect regular buffered io from the tests anyway
it might be easier just to add a bit of magic and call it a day.
> > > }
> > > out:
> > > bpf_object__close(obj);
> > > diff --git a/tools/testing/selftests/bpf/prog_tests/map_lock.c b/tools/testing/selftests/bpf/prog_tests/map_lock.c
> > > index ee99368c595c..2e78217ed3fd 100644
> > > --- a/tools/testing/selftests/bpf/prog_tests/map_lock.c
> > > +++ b/tools/testing/selftests/bpf/prog_tests/map_lock.c
> > > @@ -9,12 +9,12 @@ static void *parallel_map_access(void *arg)
>
> [...]
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH bpf-next 4/9] libbpf: add libbpf_swap_print to get previous print func
2019-07-26 21:47 ` Andrii Nakryiko
@ 2019-07-27 0:30 ` Alexei Starovoitov
2019-07-27 18:49 ` Andrii Nakryiko
0 siblings, 1 reply; 28+ messages in thread
From: Alexei Starovoitov @ 2019-07-27 0:30 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Stanislav Fomichev, Andrii Nakryiko, bpf, Networking,
Alexei Starovoitov, Daniel Borkmann, Kernel Team
On Fri, Jul 26, 2019 at 02:47:28PM -0700, Andrii Nakryiko wrote:
> On Fri, Jul 26, 2019 at 2:28 PM Stanislav Fomichev <sdf@fomichev.me> wrote:
> >
> > On 07/26, Andrii Nakryiko wrote:
> > > libbpf_swap_print allows to restore previously set print function.
> > > This is useful when running many independent test with one default print
> > > function, but overriding log verbosity for particular subset of tests.
> > Can we change the return type of libbpf_set_print instead and return
> > the old function from it? Will it break ABI?
>
> Yeah, thought about that, but I wasn't sure about ABI breakage. It
> seems like it shouldn't, so I'll just change libbpf_set_print
> signature instead.
I think it's ok to change return value of libbpf_set_print() from void
to useful pointer.
This function is not marked as __attribute__((__warn_unused_result__)),
so there should be no abi issues.
Please double check by compiler perf with different gcc-s as Arnaldo's setup does.
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH bpf-next 6/9] selftests/bpf: abstract away test log output
2019-07-26 22:26 ` Stanislav Fomichev
@ 2019-07-27 0:34 ` Alexei Starovoitov
2019-07-27 18:56 ` Andrii Nakryiko
1 sibling, 0 replies; 28+ messages in thread
From: Alexei Starovoitov @ 2019-07-27 0:34 UTC (permalink / raw)
To: Stanislav Fomichev
Cc: Andrii Nakryiko, Andrii Nakryiko, bpf, Networking,
Alexei Starovoitov, Daniel Borkmann, Kernel Team
On Fri, Jul 26, 2019 at 03:26:52PM -0700, Stanislav Fomichev wrote:
> On 07/26, Andrii Nakryiko wrote:
> > On Fri, Jul 26, 2019 at 2:31 PM Stanislav Fomichev <sdf@fomichev.me> wrote:
> > >
> > > On 07/26, Andrii Nakryiko wrote:
> > > > This patch changes how test output is printed out. By default, if test
> > > > had no errors, the only output will be a single line with test number,
> > > > name, and verdict at the end, e.g.:
> > > >
> > > > #31 xdp:OK
> > > >
> > > > If test had any errors, all log output captured during test execution
> > > > will be output after test completes.
> > > >
> > > > It's possible to force output of log with `-v` (`--verbose`) option, in
> > > > which case output won't be buffered and will be output immediately.
> > > >
> > > > To support this, individual tests are required to use helper methods for
> > > > logging: `test__printf()` and `test__vprintf()`.
> > > >
> > > > Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> > > > ---
> > > > .../selftests/bpf/prog_tests/bpf_obj_id.c | 6 +-
> > > > .../bpf/prog_tests/bpf_verif_scale.c | 31 ++--
> > > > .../bpf/prog_tests/get_stack_raw_tp.c | 4 +-
> > > > .../selftests/bpf/prog_tests/l4lb_all.c | 2 +-
> > > > .../selftests/bpf/prog_tests/map_lock.c | 10 +-
> > > > .../selftests/bpf/prog_tests/send_signal.c | 8 +-
> > > > .../selftests/bpf/prog_tests/spinlock.c | 2 +-
> > > > .../bpf/prog_tests/stacktrace_build_id.c | 4 +-
> > > > .../bpf/prog_tests/stacktrace_build_id_nmi.c | 4 +-
> > > > .../selftests/bpf/prog_tests/xdp_noinline.c | 3 +-
> > > > tools/testing/selftests/bpf/test_progs.c | 135 +++++++++++++-----
> > > > tools/testing/selftests/bpf/test_progs.h | 37 ++++-
> > > > 12 files changed, 173 insertions(+), 73 deletions(-)
> > > >
> >
> > [...]
> >
> > > > error_cnt++;
> > > > - printf("test_l4lb:FAIL:stats %lld %lld\n", bytes, pkts);
> > > > + test__printf("test_l4lb:FAIL:stats %lld %lld\n", bytes, pkts);
> > > #define printf(...) test__printf(...) in tests.h?
> > >
> > > A bit ugly, but no need to retrain everyone to use new printf wrappers.
> >
> > I try to reduce amount of magic and surprising things, not add new
> > ones :) I also led by example and converted all current instances of
> > printf usage to test__printf, so anyone new will just copy/paste good
> > example, hopefully. Even if not, this non-buffered output will be
> > immediately obvious to anyone who just runs `sudo ./test_progs`.
>
> [..]
> > And
> > author of new test with this problem should hopefully be the first and
> > the only one to catch and fix this.
> Yeah, that is my only concern, that regular printfs will eventually
> creep in. It's already confusing to go to/from printf/printk.
>
> 2c:
>
> I'm coming from a perspective of tools/testing/selftests/kselftest.h
> which is supposed to be a generic framework with custom
> printf variants (ksft_print_msg), but I still see a bunch of tests
> calling printf :-/
>
> grep -ril ksft_exit_fail_msg selftests/ | xargs -n1 grep -w printf
>
> Since we don't expect regular buffered io from the tests anyway
> it might be easier just to add a bit of magic and call it a day.
I think #define printf()
is not a good style in general.
glibc functions should never be #define-d.
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH bpf-next 4/9] libbpf: add libbpf_swap_print to get previous print func
2019-07-27 0:30 ` Alexei Starovoitov
@ 2019-07-27 18:49 ` Andrii Nakryiko
0 siblings, 0 replies; 28+ messages in thread
From: Andrii Nakryiko @ 2019-07-27 18:49 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Stanislav Fomichev, Andrii Nakryiko, bpf, Networking,
Alexei Starovoitov, Daniel Borkmann, Kernel Team
On Fri, Jul 26, 2019 at 5:30 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Fri, Jul 26, 2019 at 02:47:28PM -0700, Andrii Nakryiko wrote:
> > On Fri, Jul 26, 2019 at 2:28 PM Stanislav Fomichev <sdf@fomichev.me> wrote:
> > >
> > > On 07/26, Andrii Nakryiko wrote:
> > > > libbpf_swap_print allows to restore previously set print function.
> > > > This is useful when running many independent test with one default print
> > > > function, but overriding log verbosity for particular subset of tests.
> > > Can we change the return type of libbpf_set_print instead and return
> > > the old function from it? Will it break ABI?
> >
> > Yeah, thought about that, but I wasn't sure about ABI breakage. It
> > seems like it shouldn't, so I'll just change libbpf_set_print
> > signature instead.
>
> I think it's ok to change return value of libbpf_set_print() from void
> to useful pointer.
Some googling gave inconclusive results. StackOverflow answers claim
it is compatible ABI change ([0]), but I also found some guidelines
for Android that designate any return type change as incompatible
([1]). [2] wasn't very helpful about defining compatibility rules,
unfortunately. I'm going with [0], though, and changing return type.
[0] https://stackoverflow.com/questions/15626579/c-abi-is-changing-a-void-function-to-return-an-int-a-breaking-change
[1] https://source.android.com/devices/architecture/vndk/abi-stability
[2] https://www.cs.dartmouth.edu/~sergey/cs258/ABI/UlrichDrepper-How-To-Write-Shared-Libraries.pdf
> This function is not marked as __attribute__((__warn_unused_result__)),
> so there should be no abi issues.
>
> Please double check by compiler perf with different gcc-s as Arnaldo's setup does.
>
Compiled (make -C tools/perf) with GCC 4.8.5, GCC 7, and Clang 8. None
of them produced any warning, so I'm going forward with just return
type change.
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH bpf-next 1/9] selftests/bpf: prevent headers to be compiled as C code
2019-07-26 22:01 ` Stanislav Fomichev
@ 2019-07-27 18:53 ` Andrii Nakryiko
2019-07-31 13:21 ` Ilya Leoshkevich
0 siblings, 1 reply; 28+ messages in thread
From: Andrii Nakryiko @ 2019-07-27 18:53 UTC (permalink / raw)
To: Stanislav Fomichev
Cc: Andrii Nakryiko, bpf, Networking, Alexei Starovoitov,
Daniel Borkmann, Kernel Team
On Fri, Jul 26, 2019 at 3:01 PM Stanislav Fomichev <sdf@fomichev.me> wrote:
>
> On 07/26, Andrii Nakryiko wrote:
> > On Fri, Jul 26, 2019 at 2:21 PM Stanislav Fomichev <sdf@fomichev.me> wrote:
> > >
> > > On 07/26, Andrii Nakryiko wrote:
> > > > Apprently listing header as a normal dependency for a binary output
> > > > makes it go through compilation as if it was C code. This currently
> > > > works without a problem, but in subsequent commits causes problems for
> > > > differently generated test.h for test_progs. Marking those headers as
> > > > order-only dependency solves the issue.
> > > Are you sure it will not result in a situation where
> > > test_progs/test_maps is not regenerated if tests.h is updated.
> > >
> > > If I read the following doc correctly, order deps make sense for
> > > directories only:
> > > https://www.gnu.org/software/make/manual/html_node/Prerequisite-Types.html
> > >
> > > Can you maybe double check it with:
> > > * make
> > > * add new prog_tests/test_something.c
> > > * make
> > > to see if the binary is regenerated with test_something.c?
> >
> > Yeah, tested that, it triggers test_progs rebuild.
> >
> > Ordering is still preserved, because test.h is dependency of
> > test_progs.c, which is dependency of test_progs binary, so that's why
> > it works.
> >
> > As to why .h file is compiled as C file, I have no idea and ideally
> > that should be fixed somehow.
> I guess that's because it's a prerequisite and we have a target that
> puts all prerequisites when calling CC:
>
> test_progs: a.c b.c tests.h
> gcc a.c b.c tests.h -o test_progs
>
> So gcc compiles each input file.
If that's really a definition of the rule, then it seems not exactly
correct. What if some of the prerequisites are some object files,
directories, etc. I'd say the rule should only include .c files. I'll
add it to my TODO list to go and check how all this is defined
somewhere, but for now I'm leaving everything as is in this patch.
>
> I'm not actually sure why default dependency system that uses 'gcc -M'
> is not working for us (see scripts/Kbuild.include) and we need to manually
> add tests.h dependency. But that's outside of the scope..
>
> > I also started with just removing header as dependency completely
> > (because it's indirect dependency of test_progs.c), but that broke the
> > build logic. Dunno, too much magic... This works, tested many-many
> > times, so I was satisfied enough :)
> Yeah, that's my only concern, too much magic already and we add
> quite a bit more.
>
> > > Maybe fix the problem of header compilation by having '#ifndef
> > > DECLARE_TEST #define DECLARE_TEST() #endif' in tests.h instead?
> >
> > That's ugly, I'd like to avoid doing that.
> That's your call, but I'm not sure what's uglier: complicating already
> complex make rules or making a header self contained.
The right call is to fix selftests/bpf Makefile for good (there are
more issues, e.g., we rebuild all the prog_tests/*.c for alu32 tests,
even if we only modified test_progs.c), but that's for another patch
set, unless someone beats me and fixes that first.
>
> > > > Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> > > > ---
> > > > tools/testing/selftests/bpf/Makefile | 6 +++---
> > > > 1 file changed, 3 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> > > > index 11c9c62c3362..bb66cc4a7f34 100644
> > > > --- a/tools/testing/selftests/bpf/Makefile
> > > > +++ b/tools/testing/selftests/bpf/Makefile
> > > > @@ -235,7 +235,7 @@ PROG_TESTS_H := $(PROG_TESTS_DIR)/tests.h
> > > > PROG_TESTS_FILES := $(wildcard prog_tests/*.c)
> > > > test_progs.c: $(PROG_TESTS_H)
> > > > $(OUTPUT)/test_progs: CFLAGS += $(TEST_PROGS_CFLAGS)
> > > > -$(OUTPUT)/test_progs: test_progs.c $(PROG_TESTS_H) $(PROG_TESTS_FILES)
> > > > +$(OUTPUT)/test_progs: test_progs.c $(PROG_TESTS_FILES) | $(PROG_TESTS_H)
> > > > $(PROG_TESTS_H): $(PROG_TESTS_FILES) | $(PROG_TESTS_DIR)
> > > > $(shell ( cd prog_tests/; \
> > > > echo '/* Generated header, do not edit */'; \
> > > > @@ -256,7 +256,7 @@ MAP_TESTS_H := $(MAP_TESTS_DIR)/tests.h
> > > > MAP_TESTS_FILES := $(wildcard map_tests/*.c)
> > > > test_maps.c: $(MAP_TESTS_H)
> > > > $(OUTPUT)/test_maps: CFLAGS += $(TEST_MAPS_CFLAGS)
> > > > -$(OUTPUT)/test_maps: test_maps.c $(MAP_TESTS_H) $(MAP_TESTS_FILES)
> > > > +$(OUTPUT)/test_maps: test_maps.c $(MAP_TESTS_FILES) | $(MAP_TESTS_H)
> > > > $(MAP_TESTS_H): $(MAP_TESTS_FILES) | $(MAP_TESTS_DIR)
> > > > $(shell ( cd map_tests/; \
> > > > echo '/* Generated header, do not edit */'; \
> > > > @@ -277,7 +277,7 @@ VERIFIER_TESTS_H := $(VERIFIER_TESTS_DIR)/tests.h
> > > > VERIFIER_TEST_FILES := $(wildcard verifier/*.c)
> > > > test_verifier.c: $(VERIFIER_TESTS_H)
> > > > $(OUTPUT)/test_verifier: CFLAGS += $(TEST_VERIFIER_CFLAGS)
> > > > -$(OUTPUT)/test_verifier: test_verifier.c $(VERIFIER_TESTS_H)
> > > > +$(OUTPUT)/test_verifier: test_verifier.c | $(VERIFIER_TEST_FILES) $(VERIFIER_TESTS_H)
> > > > $(VERIFIER_TESTS_H): $(VERIFIER_TEST_FILES) | $(VERIFIER_TESTS_DIR)
> > > > $(shell ( cd verifier/; \
> > > > echo '/* Generated header, do not edit */'; \
> > > > --
> > > > 2.17.1
> > > >
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH bpf-next 6/9] selftests/bpf: abstract away test log output
2019-07-26 22:26 ` Stanislav Fomichev
2019-07-27 0:34 ` Alexei Starovoitov
@ 2019-07-27 18:56 ` Andrii Nakryiko
1 sibling, 0 replies; 28+ messages in thread
From: Andrii Nakryiko @ 2019-07-27 18:56 UTC (permalink / raw)
To: Stanislav Fomichev
Cc: Andrii Nakryiko, bpf, Networking, Alexei Starovoitov,
Daniel Borkmann, Kernel Team
On Fri, Jul 26, 2019 at 3:26 PM Stanislav Fomichev <sdf@fomichev.me> wrote:
>
> On 07/26, Andrii Nakryiko wrote:
> > On Fri, Jul 26, 2019 at 2:31 PM Stanislav Fomichev <sdf@fomichev.me> wrote:
> > >
> > > On 07/26, Andrii Nakryiko wrote:
> > > > This patch changes how test output is printed out. By default, if test
> > > > had no errors, the only output will be a single line with test number,
> > > > name, and verdict at the end, e.g.:
> > > >
> > > > #31 xdp:OK
> > > >
> > > > If test had any errors, all log output captured during test execution
> > > > will be output after test completes.
> > > >
> > > > It's possible to force output of log with `-v` (`--verbose`) option, in
> > > > which case output won't be buffered and will be output immediately.
> > > >
> > > > To support this, individual tests are required to use helper methods for
> > > > logging: `test__printf()` and `test__vprintf()`.
> > > >
> > > > Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> > > > ---
> > > > .../selftests/bpf/prog_tests/bpf_obj_id.c | 6 +-
> > > > .../bpf/prog_tests/bpf_verif_scale.c | 31 ++--
> > > > .../bpf/prog_tests/get_stack_raw_tp.c | 4 +-
> > > > .../selftests/bpf/prog_tests/l4lb_all.c | 2 +-
> > > > .../selftests/bpf/prog_tests/map_lock.c | 10 +-
> > > > .../selftests/bpf/prog_tests/send_signal.c | 8 +-
> > > > .../selftests/bpf/prog_tests/spinlock.c | 2 +-
> > > > .../bpf/prog_tests/stacktrace_build_id.c | 4 +-
> > > > .../bpf/prog_tests/stacktrace_build_id_nmi.c | 4 +-
> > > > .../selftests/bpf/prog_tests/xdp_noinline.c | 3 +-
> > > > tools/testing/selftests/bpf/test_progs.c | 135 +++++++++++++-----
> > > > tools/testing/selftests/bpf/test_progs.h | 37 ++++-
> > > > 12 files changed, 173 insertions(+), 73 deletions(-)
> > > >
> >
> > [...]
> >
> > > > error_cnt++;
> > > > - printf("test_l4lb:FAIL:stats %lld %lld\n", bytes, pkts);
> > > > + test__printf("test_l4lb:FAIL:stats %lld %lld\n", bytes, pkts);
> > > #define printf(...) test__printf(...) in tests.h?
> > >
> > > A bit ugly, but no need to retrain everyone to use new printf wrappers.
> >
> > I try to reduce amount of magic and surprising things, not add new
> > ones :) I also led by example and converted all current instances of
> > printf usage to test__printf, so anyone new will just copy/paste good
> > example, hopefully. Even if not, this non-buffered output will be
> > immediately obvious to anyone who just runs `sudo ./test_progs`.
>
> [..]
> > And
> > author of new test with this problem should hopefully be the first and
> > the only one to catch and fix this.
> Yeah, that is my only concern, that regular printfs will eventually
> creep in. It's already confusing to go to/from printf/printk.
We should catch that in code review, but Alexei and Daniel will be the
last line of defense anywas, as they run test_progs before merging
stuff and will immediately notice extra non-buffered output, given
that successful output from test_progs is now very laconic.
>
> 2c:
>
> I'm coming from a perspective of tools/testing/selftests/kselftest.h
> which is supposed to be a generic framework with custom
> printf variants (ksft_print_msg), but I still see a bunch of tests
> calling printf :-/
>
> grep -ril ksft_exit_fail_msg selftests/ | xargs -n1 grep -w printf
>
> Since we don't expect regular buffered io from the tests anyway
> it might be easier just to add a bit of magic and call it a day.
>
> > > > }
> > > > out:
> > > > bpf_object__close(obj);
> > > > diff --git a/tools/testing/selftests/bpf/prog_tests/map_lock.c b/tools/testing/selftests/bpf/prog_tests/map_lock.c
> > > > index ee99368c595c..2e78217ed3fd 100644
> > > > --- a/tools/testing/selftests/bpf/prog_tests/map_lock.c
> > > > +++ b/tools/testing/selftests/bpf/prog_tests/map_lock.c
> > > > @@ -9,12 +9,12 @@ static void *parallel_map_access(void *arg)
> >
> > [...]
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH bpf-next 1/9] selftests/bpf: prevent headers to be compiled as C code
2019-07-27 18:53 ` Andrii Nakryiko
@ 2019-07-31 13:21 ` Ilya Leoshkevich
2019-07-31 17:04 ` Andrii Nakryiko
0 siblings, 1 reply; 28+ messages in thread
From: Ilya Leoshkevich @ 2019-07-31 13:21 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Stanislav Fomichev, Andrii Nakryiko, bpf, Networking,
Alexei Starovoitov, Daniel Borkmann, Kernel Team
> Am 27.07.2019 um 20:53 schrieb Andrii Nakryiko <andrii.nakryiko@gmail.com>:
>
> On Fri, Jul 26, 2019 at 3:01 PM Stanislav Fomichev <sdf@fomichev.me> wrote:
>>
>> On 07/26, Andrii Nakryiko wrote:
>>> On Fri, Jul 26, 2019 at 2:21 PM Stanislav Fomichev <sdf@fomichev.me> wrote:
>>>>
>>>> On 07/26, Andrii Nakryiko wrote:
>>>>> Apprently listing header as a normal dependency for a binary output
>>>>> makes it go through compilation as if it was C code. This currently
>>>>> works without a problem, but in subsequent commits causes problems for
>>>>> differently generated test.h for test_progs. Marking those headers as
>>>>> order-only dependency solves the issue.
>>>> Are you sure it will not result in a situation where
>>>> test_progs/test_maps is not regenerated if tests.h is updated.
>>>>
>>>> If I read the following doc correctly, order deps make sense for
>>>> directories only:
>>>> https://www.gnu.org/software/make/manual/html_node/Prerequisite-Types.html
>>>>
>>>> Can you maybe double check it with:
>>>> * make
>>>> * add new prog_tests/test_something.c
>>>> * make
>>>> to see if the binary is regenerated with test_something.c?
>>>
>>> Yeah, tested that, it triggers test_progs rebuild.
>>>
>>> Ordering is still preserved, because test.h is dependency of
>>> test_progs.c, which is dependency of test_progs binary, so that's why
>>> it works.
>>>
>>> As to why .h file is compiled as C file, I have no idea and ideally
>>> that should be fixed somehow.
>> I guess that's because it's a prerequisite and we have a target that
>> puts all prerequisites when calling CC:
>>
>> test_progs: a.c b.c tests.h
>> gcc a.c b.c tests.h -o test_progs
>>
>> So gcc compiles each input file.
>
> If that's really a definition of the rule, then it seems not exactly
> correct. What if some of the prerequisites are some object files,
> directories, etc. I'd say the rule should only include .c files. I'll
> add it to my TODO list to go and check how all this is defined
> somewhere, but for now I'm leaving everything as is in this patch.
>
I believe it’s an implicit built-in rule, which is defined by make
itself here:
https://git.savannah.gnu.org/cgit/make.git/tree/default.c?h=4.2.1#n131
The observed behavior arises because these rules use $^ all over the
place. My 2c is that it would be better to make the rule explicit,
because it would cost just an extra line, but it would be immediately
obvious why the code is correct w.r.t. rebuilds.
Best regards,
Ilya
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH bpf-next 1/9] selftests/bpf: prevent headers to be compiled as C code
2019-07-31 13:21 ` Ilya Leoshkevich
@ 2019-07-31 17:04 ` Andrii Nakryiko
0 siblings, 0 replies; 28+ messages in thread
From: Andrii Nakryiko @ 2019-07-31 17:04 UTC (permalink / raw)
To: Ilya Leoshkevich
Cc: Stanislav Fomichev, Andrii Nakryiko, bpf, Networking,
Alexei Starovoitov, Daniel Borkmann, Kernel Team
On Wed, Jul 31, 2019 at 6:21 AM Ilya Leoshkevich <iii@linux.ibm.com> wrote:
>
> > Am 27.07.2019 um 20:53 schrieb Andrii Nakryiko <andrii.nakryiko@gmail.com>:
> >
> > On Fri, Jul 26, 2019 at 3:01 PM Stanislav Fomichev <sdf@fomichev.me> wrote:
> >>
> >> On 07/26, Andrii Nakryiko wrote:
> >>> On Fri, Jul 26, 2019 at 2:21 PM Stanislav Fomichev <sdf@fomichev.me> wrote:
> >>>>
> >>>> On 07/26, Andrii Nakryiko wrote:
> >>>>> Apprently listing header as a normal dependency for a binary output
> >>>>> makes it go through compilation as if it was C code. This currently
> >>>>> works without a problem, but in subsequent commits causes problems for
> >>>>> differently generated test.h for test_progs. Marking those headers as
> >>>>> order-only dependency solves the issue.
> >>>> Are you sure it will not result in a situation where
> >>>> test_progs/test_maps is not regenerated if tests.h is updated.
> >>>>
> >>>> If I read the following doc correctly, order deps make sense for
> >>>> directories only:
> >>>> https://www.gnu.org/software/make/manual/html_node/Prerequisite-Types.html
> >>>>
> >>>> Can you maybe double check it with:
> >>>> * make
> >>>> * add new prog_tests/test_something.c
> >>>> * make
> >>>> to see if the binary is regenerated with test_something.c?
> >>>
> >>> Yeah, tested that, it triggers test_progs rebuild.
> >>>
> >>> Ordering is still preserved, because test.h is dependency of
> >>> test_progs.c, which is dependency of test_progs binary, so that's why
> >>> it works.
> >>>
> >>> As to why .h file is compiled as C file, I have no idea and ideally
> >>> that should be fixed somehow.
> >> I guess that's because it's a prerequisite and we have a target that
> >> puts all prerequisites when calling CC:
> >>
> >> test_progs: a.c b.c tests.h
> >> gcc a.c b.c tests.h -o test_progs
> >>
> >> So gcc compiles each input file.
> >
> > If that's really a definition of the rule, then it seems not exactly
> > correct. What if some of the prerequisites are some object files,
> > directories, etc. I'd say the rule should only include .c files. I'll
> > add it to my TODO list to go and check how all this is defined
> > somewhere, but for now I'm leaving everything as is in this patch.
> >
>
> I believe it’s an implicit built-in rule, which is defined by make
> itself here:
>
> https://git.savannah.gnu.org/cgit/make.git/tree/default.c?h=4.2.1#n131
>
> The observed behavior arises because these rules use $^ all over the
> place. My 2c is that it would be better to make the rule explicit,
> because it would cost just an extra line, but it would be immediately
> obvious why the code is correct w.r.t. rebuilds.
Thanks for pointing this out, Ilya! I agree, I'd rather have a simple
explicit rule in Makefile that having to guess where this is coming
from and why it doesn't work as you'd expect it to. If no one else
adds that first, I'll eventually get to this.
>
> Best regards,
> Ilya
^ permalink raw reply [flat|nested] 28+ messages in thread
end of thread, other threads:[~2019-07-31 17:04 UTC | newest]
Thread overview: 28+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-26 20:37 [PATCH bpf-next 0/9] Revamp test_progs as a test running framework Andrii Nakryiko
2019-07-26 20:37 ` [PATCH bpf-next 1/9] selftests/bpf: prevent headers to be compiled as C code Andrii Nakryiko
2019-07-26 21:21 ` Stanislav Fomichev
2019-07-26 21:42 ` Andrii Nakryiko
2019-07-26 22:01 ` Stanislav Fomichev
2019-07-27 18:53 ` Andrii Nakryiko
2019-07-31 13:21 ` Ilya Leoshkevich
2019-07-31 17:04 ` Andrii Nakryiko
2019-07-26 20:37 ` [PATCH bpf-next 2/9] selftests/bpf: revamp test_progs to allow more control Andrii Nakryiko
2019-07-26 20:37 ` [PATCH bpf-next 3/9] selftests/bpf: add test selectors by number and name to test_progs Andrii Nakryiko
2019-07-26 21:25 ` Stanislav Fomichev
2019-07-26 21:45 ` Andrii Nakryiko
2019-07-26 22:03 ` Stanislav Fomichev
2019-07-26 20:37 ` [PATCH bpf-next 4/9] libbpf: add libbpf_swap_print to get previous print func Andrii Nakryiko
2019-07-26 21:28 ` Stanislav Fomichev
2019-07-26 21:47 ` Andrii Nakryiko
2019-07-27 0:30 ` Alexei Starovoitov
2019-07-27 18:49 ` Andrii Nakryiko
2019-07-26 20:37 ` [PATCH bpf-next 5/9] selftest/bpf: centralize libbpf logging management for test_progs Andrii Nakryiko
2019-07-26 20:37 ` [PATCH bpf-next 6/9] selftests/bpf: abstract away test log output Andrii Nakryiko
2019-07-26 21:31 ` Stanislav Fomichev
2019-07-26 21:51 ` Andrii Nakryiko
2019-07-26 22:26 ` Stanislav Fomichev
2019-07-27 0:34 ` Alexei Starovoitov
2019-07-27 18:56 ` Andrii Nakryiko
2019-07-26 20:37 ` [PATCH bpf-next 7/9] selftests/bpf: add sub-tests support for test_progs Andrii Nakryiko
2019-07-26 20:37 ` [PATCH bpf-next 8/9] selftests/bpf: convert bpf_verif_scale.c to sub-tests API Andrii Nakryiko
2019-07-26 20:37 ` [PATCH bpf-next 9/9] selftests/bpf: convert send_signal.c to use subtests Andrii Nakryiko
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).