Re: [PATCH v3 bpf-next 3/3] selftests/bpf: add custom SEC() handling selftest

[Alexei Starovoitov <alexei.starovoitov@gmail.com>]

On Fri, Feb 11, 2022 at 03:31:56PM -0800, Andrii Nakryiko wrote:
> On Fri, Feb 11, 2022 at 3:13 PM Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
> >
> > On Fri, Feb 11, 2022 at 01:14:50PM -0800, Andrii Nakryiko wrote:
> > > Add a selftest validating various aspects of libbpf's handling of custom
> > > SEC() handlers. It also demonstrates how libraries can ensure very early
> > > callbacks registration and unregistration using
> > > __attribute__((constructor))/__attribute__((destructor)) functions.
> > >
> > > Reviewed-by: Alan Maguire <alan.maguire@oracle.com>
> > > Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> > > ---
> > >  .../bpf/prog_tests/custom_sec_handlers.c      | 176 ++++++++++++++++++
> > >  .../bpf/progs/test_custom_sec_handlers.c      |  63 +++++++
> > >  2 files changed, 239 insertions(+)
> > >  create mode 100644 tools/testing/selftests/bpf/prog_tests/custom_sec_handlers.c
> > >  create mode 100644 tools/testing/selftests/bpf/progs/test_custom_sec_handlers.c
> > >
> > > diff --git a/tools/testing/selftests/bpf/prog_tests/custom_sec_handlers.c b/tools/testing/selftests/bpf/prog_tests/custom_sec_handlers.c
> > > new file mode 100644
> > > index 000000000000..28264528280d
> > > --- /dev/null
> > > +++ b/tools/testing/selftests/bpf/prog_tests/custom_sec_handlers.c
> > > @@ -0,0 +1,176 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/* Copyright (c) 2022 Facebook */
> > > +
> > > +#include <test_progs.h>
> > > +#include "test_custom_sec_handlers.skel.h"
> > > +
> > > +#define COOKIE_ABC1 1
> > > +#define COOKIE_ABC2 2
> > > +#define COOKIE_CUSTOM 3
> > > +#define COOKIE_FALLBACK 4
> > > +#define COOKIE_KPROBE 5
> > > +
> > > +static int custom_init_prog(struct bpf_program *prog, long cookie)
> > > +{
> > > +     if (cookie == COOKIE_ABC1)
> > > +             bpf_program__set_autoload(prog, false);
> > > +
> > > +     return 0;
> > > +}
> >
> > What is the value of init_fn callback?
> > afaict it was and still unused in libbpf.
> > The above example doesn't make a compelling case, since set_autoload
> > can be done out of preload callback.
> > Maybe drop init_fn for now and extend libbpf_prog_handler_opts
> > when there is actual need for it?
> 
> Great question, but no, you can't set_autoload() in the preload
> handler, because once preload is called, loading of the program is
> inevitable.

Ahh!, but we can add 'if (prog->load)' in bpf_object_load_prog_instance()
after preload_fn() was called.
Surely the libbpf would waste some time preping the prog with relos,
but that's not a big deal.
Another option is to move preload_fn earlier.
Especially since currently it's only setting attach types.

Calling the callback 'preload' when it cannot affect the load is odd too.

> We might need to adjust the obj->loaded flag so that set_autoload()
> returns an error when called from the preload() callback, but that's a
> bit orthogonal. I suspect there will be few more adjustments like this
> as we get actual users of this API in the wild.
> 
> It's not used by libbpf because we do all the initialization outside
> of the callback, as it is the same for all programs and serves as
> "default" behavior that custom handlers can override.
> 
> Also, keep in mind that this is the very beginning of v0.8 dev cycle,
> we'll have time to adjust interfaces and callback contracts in the
> next 2-3 months, if necessary. USDT library open-sourcing will almost
> 100% happen during this time frame (though I think USDT library is a
> pretty simple use case, so isn't a great "stress test"). But it also
> seems like perf might need to use fallback handler for their crazy
> SEC() conventions, that will be a good test as well.

It would be much easier to take your word if there was an actual example
(like libusdt) that demonstrates the use of callbacks.
"We will have time to fix things before release" isn't very comforting
in the case of big api extension like this one.