| Message ID | 20211006185619.364369-3-fallentree@fb.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | BPF |
| Headers | show |
| Series | selftests/bpf: Add parallelism to test_progs | expand |
On Wed, Oct 6, 2021 at 11:56 AM Yucong Sun <fallentree@fb.com> wrote: > > From: Yucong Sun <sunyucong@gmail.com> > > This patch allows tests to define serial_test_name() instead of > test_name(), and this will make test_progs execute those in sequence > after all other tests finished executing concurrently. > > Signed-off-by: Yucong Sun <sunyucong@gmail.com> > --- > tools/testing/selftests/bpf/test_progs.c | 60 +++++++++++++++++++++--- > 1 file changed, 54 insertions(+), 6 deletions(-) > [...] > @@ -1129,6 +1136,40 @@ static int server_main(void) > free(env.worker_current_test); > free(data); > > + /* run serial tests */ > + save_netns(); > + > + for (int i = 0; i < prog_test_cnt; i++) { > + struct prog_test_def *test = &prog_test_defs[i]; > + struct test_result *result = &test_results[i]; > + > + if (!test->should_run || !test->run_serial_test) > + continue; > + > + stdio_hijack(); > + > + run_one_test(i); > + > + stdio_restore(); > + if (env.log_buf) { > + result->log_cnt = env.log_cnt; > + result->log_buf = strdup(env.log_buf); > + > + free(env.log_buf); > + env.log_buf = NULL; > + env.log_cnt = 0; > + } > + restore_netns(); > + > + fprintf(stdout, "#%d %s:%s\n", > + test->test_num, test->test_name, > + test->error_cnt ? "FAIL" : (test->skip_cnt ? "SKIP" : "OK")); > + > + result->error_cnt = test->error_cnt; > + result->skip_cnt = test->skip_cnt; > + result->sub_succ_cnt = test->sub_succ_cnt; > + } > + Did you try to just reuse sequential running loop logic in main() for this? I'd like to avoid the third test running loop copy, if possible. What were the problems of reusing the sequential logic from main(), they do the same work, no? > /* generate summary */ > fflush(stderr); > fflush(stdout); > @@ -1326,6 +1367,13 @@ int main(int argc, char **argv) > test->should_run = true; > else > test->should_run = false; > + > + if ((test->run_test == NULL && test->run_serial_test == NULL) || > + (test->run_test != NULL && test->run_serial_test != NULL)) { > + fprintf(stderr, "Test %d:%s must have either test_%s() or serial_test_%sl() defined.\n", > + test->test_num, test->test_name, test->test_name, test->test_name); > + exit(EXIT_ERR_SETUP_INFRA); > + } > } > > /* ignore workers if we are just listing */ > -- > 2.30.2 >
On Fri, Oct 8, 2021 at 3:26 PM Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Wed, Oct 6, 2021 at 11:56 AM Yucong Sun <fallentree@fb.com> wrote: > > > > From: Yucong Sun <sunyucong@gmail.com> > > > > This patch allows tests to define serial_test_name() instead of > > test_name(), and this will make test_progs execute those in sequence > > after all other tests finished executing concurrently. > > > > Signed-off-by: Yucong Sun <sunyucong@gmail.com> > > --- > > tools/testing/selftests/bpf/test_progs.c | 60 +++++++++++++++++++++--- > > 1 file changed, 54 insertions(+), 6 deletions(-) > > > > [...] > > > @@ -1129,6 +1136,40 @@ static int server_main(void) > > free(env.worker_current_test); > > free(data); > > > > + /* run serial tests */ > > + save_netns(); > > + > > + for (int i = 0; i < prog_test_cnt; i++) { > > + struct prog_test_def *test = &prog_test_defs[i]; > > + struct test_result *result = &test_results[i]; > > + > > + if (!test->should_run || !test->run_serial_test) > > + continue; > > + > > + stdio_hijack(); > > + > > + run_one_test(i); > > + > > + stdio_restore(); > > + if (env.log_buf) { > > + result->log_cnt = env.log_cnt; > > + result->log_buf = strdup(env.log_buf); > > + > > + free(env.log_buf); > > + env.log_buf = NULL; > > + env.log_cnt = 0; > > + } > > + restore_netns(); > > + > > + fprintf(stdout, "#%d %s:%s\n", > > + test->test_num, test->test_name, > > + test->error_cnt ? "FAIL" : (test->skip_cnt ? "SKIP" : "OK")); > > + > > + result->error_cnt = test->error_cnt; > > + result->skip_cnt = test->skip_cnt; > > + result->sub_succ_cnt = test->sub_succ_cnt; > > + } > > + > > Did you try to just reuse sequential running loop logic in main() for > this? I'd like to avoid the third test running loop copy, if possible. > What were the problems of reusing the sequential logic from main(), > they do the same work, no? Well, yes and no The loop itself is small/simple enough, I'm not sure there is a value to extract them to a common function with multiple arguments. I think the main issue that needs to be refactored is that log printing still works differently in serial mode or parallel mode, it works now, but I would like to get rid of the old dump_test_log() function. > > > > /* generate summary */ > > fflush(stderr); > > fflush(stdout); > > @@ -1326,6 +1367,13 @@ int main(int argc, char **argv) > > test->should_run = true; > > else > > test->should_run = false; > > + > > + if ((test->run_test == NULL && test->run_serial_test == NULL) || > > + (test->run_test != NULL && test->run_serial_test != NULL)) { > > + fprintf(stderr, "Test %d:%s must have either test_%s() or serial_test_%sl() defined.\n", > > + test->test_num, test->test_name, test->test_name, test->test_name); > > + exit(EXIT_ERR_SETUP_INFRA); > > + } > > } > > > > /* ignore workers if we are just listing */ > > -- > > 2.30.2 > >
On Fri, Oct 8, 2021 at 4:06 PM sunyucong@gmail.com <sunyucong@gmail.com> wrote: > > On Fri, Oct 8, 2021 at 3:26 PM Andrii Nakryiko > <andrii.nakryiko@gmail.com> wrote: > > > > On Wed, Oct 6, 2021 at 11:56 AM Yucong Sun <fallentree@fb.com> wrote: > > > > > > From: Yucong Sun <sunyucong@gmail.com> > > > > > > This patch allows tests to define serial_test_name() instead of > > > test_name(), and this will make test_progs execute those in sequence > > > after all other tests finished executing concurrently. > > > > > > Signed-off-by: Yucong Sun <sunyucong@gmail.com> > > > --- > > > tools/testing/selftests/bpf/test_progs.c | 60 +++++++++++++++++++++--- > > > 1 file changed, 54 insertions(+), 6 deletions(-) > > > > > > > [...] > > > > > @@ -1129,6 +1136,40 @@ static int server_main(void) > > > free(env.worker_current_test); > > > free(data); > > > > > > + /* run serial tests */ > > > + save_netns(); > > > + > > > + for (int i = 0; i < prog_test_cnt; i++) { > > > + struct prog_test_def *test = &prog_test_defs[i]; > > > + struct test_result *result = &test_results[i]; > > > + > > > + if (!test->should_run || !test->run_serial_test) > > > + continue; > > > + > > > + stdio_hijack(); > > > + > > > + run_one_test(i); > > > + > > > + stdio_restore(); > > > + if (env.log_buf) { > > > + result->log_cnt = env.log_cnt; > > > + result->log_buf = strdup(env.log_buf); > > > + > > > + free(env.log_buf); > > > + env.log_buf = NULL; > > > + env.log_cnt = 0; > > > + } > > > + restore_netns(); > > > + > > > + fprintf(stdout, "#%d %s:%s\n", > > > + test->test_num, test->test_name, > > > + test->error_cnt ? "FAIL" : (test->skip_cnt ? "SKIP" : "OK")); > > > + > > > + result->error_cnt = test->error_cnt; > > > + result->skip_cnt = test->skip_cnt; > > > + result->sub_succ_cnt = test->sub_succ_cnt; > > > + } > > > + > > > > Did you try to just reuse sequential running loop logic in main() for > > this? I'd like to avoid the third test running loop copy, if possible. > > What were the problems of reusing the sequential logic from main(), > > they do the same work, no? > > Well, yes and no > > The loop itself is small/simple enough, I'm not sure there is a value > to extract them to a common function with multiple arguments. The loop has netns save/restore, stdio hijacking, output formatting, we might add some more logic later. I'm mainly asking because there is already a sequential loop in the main, and I was wondering if we can reuse that (as in, let it run regardless of -j option, just run only serial tests if -j is specified). > I think the main issue that needs to be refactored is that log > printing still works differently in serial mode or parallel mode, it > works now, but I would like to get rid of the old dump_test_log() > function. > > > > > > > > /* generate summary */ > > > fflush(stderr); > > > fflush(stdout); > > > @@ -1326,6 +1367,13 @@ int main(int argc, char **argv) > > > test->should_run = true; > > > else > > > test->should_run = false; > > > + > > > + if ((test->run_test == NULL && test->run_serial_test == NULL) || > > > + (test->run_test != NULL && test->run_serial_test != NULL)) { > > > + fprintf(stderr, "Test %d:%s must have either test_%s() or serial_test_%sl() defined.\n", > > > + test->test_num, test->test_name, test->test_name, test->test_name); > > > + exit(EXIT_ERR_SETUP_INFRA); > > > + } > > > } > > > > > > /* ignore workers if we are just listing */ > > > -- > > > 2.30.2 > > >
diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c index 51e18d8df7f2..4e2028189471 100644 --- a/tools/testing/selftests/bpf/test_progs.c +++ b/tools/testing/selftests/bpf/test_progs.c @@ -50,6 +50,7 @@ struct prog_test_def { const char *test_name; int test_num; void (*run_test)(void); + void (*run_serial_test)(void); bool force_log; int error_cnt; int skip_cnt; @@ -455,14 +456,17 @@ static int load_bpf_testmod(void) } /* extern declarations for test funcs */ -#define DEFINE_TEST(name) extern void test_##name(void); +#define DEFINE_TEST(name) \ + extern void test_##name(void) __weak; \ + extern void serial_test_##name(void) __weak; #include <prog_tests/tests.h> #undef DEFINE_TEST 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, \ + .run_serial_test = &serial_test_##name, \ }, #include <prog_tests/tests.h> #undef DEFINE_TEST @@ -902,7 +906,10 @@ static void run_one_test(int test_num) env.test = test; - test->run_test(); + if (test->run_test) + test->run_test(); + else if (test->run_serial_test) + test->run_serial_test(); /* ensure last sub-test is finalized properly */ if (test->subtest_name) @@ -952,7 +959,7 @@ void *dispatch_thread(void *ctx) pthread_mutex_unlock(¤t_test_lock); } - if (!test->should_run) + if (!test->should_run || test->run_serial_test) continue; /* run test through worker */ @@ -1129,6 +1136,40 @@ static int server_main(void) free(env.worker_current_test); free(data); + /* run serial tests */ + save_netns(); + + for (int i = 0; i < prog_test_cnt; i++) { + struct prog_test_def *test = &prog_test_defs[i]; + struct test_result *result = &test_results[i]; + + if (!test->should_run || !test->run_serial_test) + continue; + + stdio_hijack(); + + run_one_test(i); + + stdio_restore(); + if (env.log_buf) { + result->log_cnt = env.log_cnt; + result->log_buf = strdup(env.log_buf); + + free(env.log_buf); + env.log_buf = NULL; + env.log_cnt = 0; + } + restore_netns(); + + fprintf(stdout, "#%d %s:%s\n", + test->test_num, test->test_name, + test->error_cnt ? "FAIL" : (test->skip_cnt ? "SKIP" : "OK")); + + result->error_cnt = test->error_cnt; + result->skip_cnt = test->skip_cnt; + result->sub_succ_cnt = test->sub_succ_cnt; + } + /* generate summary */ fflush(stderr); fflush(stdout); @@ -1326,6 +1367,13 @@ int main(int argc, char **argv) test->should_run = true; else test->should_run = false; + + if ((test->run_test == NULL && test->run_serial_test == NULL) || + (test->run_test != NULL && test->run_serial_test != NULL)) { + fprintf(stderr, "Test %d:%s must have either test_%s() or serial_test_%sl() defined.\n", + test->test_num, test->test_name, test->test_name, test->test_name); + exit(EXIT_ERR_SETUP_INFRA); + } } /* ignore workers if we are just listing */