On Tue, Aug 22, 2023 at 9:58 AM Kumar Kartikeya Dwivedi
<memxor@xxxxxxxxx> wrote:
>
> On Tue, 22 Aug 2023 at 22:05, Alexei Starovoitov
> <alexei.starovoitov@xxxxxxxxx> wrote:
> >
> > On Wed, Aug 9, 2023 at 4:44 AM Kumar Kartikeya Dwivedi <memxor@xxxxxxxxx> wrote:
> > >
> > > Add support to libbpf to append exception callbacks when loading a
> > > program. The exception callback is found by discovering the declaration
> > > tag 'exception_callback:<value>' and finding the callback in the value
> > > of the tag.
> >
> > ...
> >
> > > + /* After adding all programs, now pair them with their exception
> > > + * callbacks if specified.
> > > + */
> > > + if (!kernel_supports(obj, FEAT_BTF_DECL_TAG))
> > > + goto out;
> > > +out:
> >
> > The above looks odd. Accidental leftover?
> >
>
> Oops, yes. I have dropped it now.
>
> > > return 0;
> > > }
> > >
> > > @@ -3137,6 +3148,80 @@ static int bpf_object__sanitize_and_load_btf(struct bpf_object *obj)
> > > }
> > > }
> > >
> > > + if (!kernel_supports(obj, FEAT_BTF_DECL_TAG))
> > > + goto skip_exception_cb;
> > > + for (i = 0; i < obj->nr_programs; i++) {
> > > + struct bpf_program *prog = &obj->programs[i];
> > > + int j, k, n;
> > > +
> > > + if (prog_is_subprog(obj, prog))
> > > + continue;
> > > + n = btf__type_cnt(obj->btf);
> > > + for (j = 1; j < n; j++) {
> > > + const char *str = "exception_callback:", *name;
> >
> > On the first read of this patch and corresponding kernel support
> > I started doubting my earlier suggestion to use decl_tag and
> > reconsidered going back to fake bpf_register_except_cb() call,
>
> This is exactly what I thought when I realised it's not that simple
> when implementing it :).
>
> > but after sleeping on it I think it is a useful extension for both
> > kernel and libbpf to support such tagging.
> > We might specify ctors and dtors with decl_tag in the future
> > and other various callbacks that are never explicitly referenced
> > in bpf_call, ld_imm64 or other bpf insns.
> > So having libbpf and kernel support such tagging will help in the long run.
> > It's not going to be limited to exceptions.
> > Despite the extra complexity this is a good step forward.
> >
>
> I agree. This same code can also be reused to establish an edge from
> one BTF type to some other BTF type (by name).
> function -> exception_cb. struct -> ctor, struct -> dtor etc.
>
> I did have some questions though.
> First of all this is explicitly an unstable feature. How do we feel
> about putting related support for it in libbpf and making breaking
> changes later?
> Will the expectation be to pair the libbpf with its corresponding
> kernel release to use such features? Or do we have to make the changes
> in a backwards compatible fashion?
We should always do backwards compatible changes in both kernel and libbpf,
but sooner or later we might hit an issue where we would have to break things.
At that time the special prefix won't save us, so...
>
> Secondly, due to proliferation of BTF tag usage, do you think it's
> time we reserve a namespace for all tags that would be recognized by
> the verifier? E.g. require all of them to be prefixed with "bpf."
> similar to "llvm." etc.? Since they are simply attributes for a
> specific BTF type (or component of a type).
> It may be too late for some BTF tags, but we could do better going forward.
>
> It may also allow us to indicate that a tag is experimental until its
> effect on the program becomes stabilized. E.g.
> bpf.experimental.exception_callback instead of exception_callback? Or
> do you feel it's unnecessary?
... I don't think any of that is necessary.
Whether btf tag is prefixed with "exception_callback:" or
"bpf.experimental.debug.exception_callback:" we will be doing the same thing.
We'll keep backward compat if trade-offs allow.
