Re: [PATCH bpf-next v6 02/14] selftests/bpf: Allow some tests to be executed in sequence

From: Andrii Nakryiko <andrii.nakryiko@gmail.com>
To: "sunyucong@gmail.com" <sunyucong@gmail.com>
Cc: Yucong Sun <fallentree@fb.com>, bpf <bpf@vger.kernel.org>,
	Andrii Nakryiko <andrii@kernel.org>
Subject: Re: [PATCH bpf-next v6 02/14] selftests/bpf: Allow some tests to be executed in sequence
Date: Fri, 8 Oct 2021 20:17:08 -0700	[thread overview]
Message-ID: <CAEf4Bzb8ukSDJk0sZokCVwh-7qNGKMD=PCh-aXVLQHPbgAYADA@mail.gmail.com> (raw)
In-Reply-To: <CAJygYd34kafprTobpd7gT9shdpPZF=aAcYF-YY0Vs+8Hh16bAg@mail.gmail.com>

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