Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20201203160245.1014867-1-jackmanb@google.com> <20201203160245.1014867-14-jackmanb@google.com>
 <b629793c-fb9c-6ef5-e2d6-7acaf1d2fc7f@fb.com> <X8oFJW/mMFHVxngY@google.com> <6f008322-0b8f-223a-9148-ce9fee0810dc@fb.com>
In-Reply-To: <6f008322-0b8f-223a-9148-ce9fee0810dc@fb.com>
From:   Andrii Nakryiko <andrii.nakryiko@gmail.com>
Date:   Fri, 4 Dec 2020 11:49:22 -0800
Message-ID: <CAEf4BzZHty17jLH7T-vDLGZftr077BUb9mSciX2Lt3Ofs4r7CQ@mail.gmail.com>
Subject: Re: [PATCH bpf-next v3 13/14] bpf: Add tests for new BPF atomic operations
To:     Yonghong Song <yhs@fb.com>
Cc:     Brendan Jackman <jackmanb@google.com>, bpf <bpf@vger.kernel.org>,
        Alexei Starovoitov <ast@kernel.org>,
        Daniel Borkmann <daniel@iogearbox.net>,
        KP Singh <kpsingh@chromium.org>,
        Florent Revest <revest@chromium.org>,
        open list <linux-kernel@vger.kernel.org>,
        Jann Horn <jannh@google.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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