Re: [PATCH bpf-next v3 13/14] bpf: Add tests for new BPF atomic operations

[Brendan Jackman <jackmanb@google.com>]

On Fri, Dec 04, 2020 at 11:49:22AM -0800, Andrii Nakryiko wrote:
> On Fri, Dec 4, 2020 at 7:29 AM Yonghong Song <yhs@fb.com> wrote:
> >
> >
> >
> > On 12/4/20 1:45 AM, Brendan Jackman wrote:
> > > On Thu, Dec 03, 2020 at 11:06:31PM -0800, Yonghong Song wrote:
> > >> On 12/3/20 8:02 AM, Brendan Jackman wrote:
> > > [...]
> > >>> diff --git a/tools/testing/selftests/bpf/prog_tests/atomics_test.c b/tools/testing/selftests/bpf/prog_tests/atomics_test.c
> > >>> new file mode 100644
> > >>> index 000000000000..66f0ccf4f4ec
> > >>> --- /dev/null
> > >>> +++ b/tools/testing/selftests/bpf/prog_tests/atomics_test.c
> > >>> @@ -0,0 +1,262 @@
> > >>> +// SPDX-License-Identifier: GPL-2.0
> > >>> +
> > >>> +#include <test_progs.h>
> > >>> +
> > >>> +
> > >>> +#include "atomics_test.skel.h"
> > >>> +
> > >>> +static struct atomics_test *setup(void)
> > >>> +{
> > >>> +   struct atomics_test *atomics_skel;
> > >>> +   __u32 duration = 0, err;
> > >>> +
> > >>> +   atomics_skel = atomics_test__open_and_load();
> > >>> +   if (CHECK(!atomics_skel, "atomics_skel_load", "atomics skeleton failed\n"))
> > >>> +           return NULL;
> > >>> +
> > >>> +   if (atomics_skel->data->skip_tests) {
> > >>> +           printf("%s:SKIP:no ENABLE_ATOMICS_TEST (missing Clang BPF atomics support)",
> > >>> +                  __func__);
> > >>> +           test__skip();
> > >>> +           goto err;
> > >>> +   }
> > >>> +
> > >>> +   err = atomics_test__attach(atomics_skel);
> > >>> +   if (CHECK(err, "atomics_attach", "atomics attach failed: %d\n", err))
> > >>> +           goto err;
> > >>> +
> > >>> +   return atomics_skel;
> > >>> +
> > >>> +err:
> > >>> +   atomics_test__destroy(atomics_skel);
> > >>> +   return NULL;
> > >>> +}
> > >>> +
> > >>> +static void test_add(void)
> > >>> +{
> > >>> +   struct atomics_test *atomics_skel;
> > >>> +   int err, prog_fd;
> > >>> +   __u32 duration = 0, retval;
> > >>> +
> > >>> +   atomics_skel = setup();
> > >>
> > >> When running the test, I observed a noticeable delay between skel load and
> > >> skel attach. The reason is the bpf program object file contains
> > >> multiple programs and the above setup() tries to do attachment
> > >> for ALL programs but actually below only "add" program is tested.
> > >> This will unnecessarily increase test_progs running time.
> > >>
> > >> The best is for setup() here only load and attach program "add".
> > >> The libbpf API bpf_program__set_autoload() can set a particular
> > >> program not autoload. You can call attach function explicitly
> > >> for one specific program. This should be able to reduce test
> > >> running time.
> > >
> > > Interesting, thanks a lot - I'll try this out next week. Maybe we can
> > > actually load all the progs once at the beginning (i.e. in
> >
> > If you have subtest, people expects subtest can be individual runable.
> > This will complicate your logic.
> >
> > > test_atomics_test) then attach/detch each prog individually as needed...
> > > Sorry, I haven't got much of a grip on libbpf yet.
> >
> > One alternative is not to do subtests. There is nothing run to have
> > just one bpf program instead of many. This way, you load all and attach
> > once, then do all the test verification.
> 
> I think subtests are good for debuggability, at least. But in this
> case it's very easy to achieve everything you've discussed:
> 
> 1. do open() right there in test_atomics_test()  (btw, consider naming
> the test just "atomics" or "atomic_insns" or something, no need for
> test-test tautology)
> 2. check if needs skipping, skip entire test
> 3. if not skipping, load
> 4. then pass the same instance of the skeleton to each subtest
> 5. each subtest will
>   5a. bpf_prog__attach(skel->prog.my_specific_subtest_prog);
>   5b. trigger and do checks
>   5c. bpf_link__destroy(<link from 5a step>);

Thanks, this seems like the way forward to me.