Re: [PATCH bpf-next 3/4] libbpf: add subskeleton scaffolding

[Andrii Nakryiko <andrii.nakryiko@gmail.com>]

On Thu, Mar 3, 2022 at 11:09 AM Delyan Kratunov <delyank@fb.com> wrote:
>
> On Wed, 2022-03-02 at 20:34 -0800, Andrii Nakryiko wrote:
> > >
> >
> > forgot to mention, this patch logically probably should go before
> > bpftool changes: 1) define types and APIs in libbpf, and only then 2)
> > "use" those in bpftool
>
> Sure.
>
> > > >
> > > > +struct bpf_sym_skeleton {
> > >
> > > I tried to get used to this "sym" terminology for a bit, but it still
> > > feels off. From user's perspective all this are variables. Any
> > > objections to use "var" terminology?
>
> "var" has a specific meaning in btf and I didn't want to make bpf_var_skeleton
> look related to btf_var for example. Given the extern usage that libs require, I
> figured "sym" would make sense to the user.
>

Even for extern cases, we only generate stuff that really is a
variable. That is, it has allocated memory and there is specific
value. Only .kconfig is like that. .ksyms, for example, doesn't get
any exposure in skeleton as it can't be used from user-space code.

For me, symbol is just way too generic (could be basically anything,
including ELF section symbol, function, etc, etc). But our use case is
always variables available to both user-space and BPF code. I don't
think btf_var vs btf_var_skeleton confusion is significant (and even
then, each extern in .kconfig has corresponding BTF_KIND_VAR, so it
all is in sync).

> If you don't think the confusion with btf_var is significant, I can rename it -
> this is all used by generated code anyway.
>
> > >
> > > > +       const char *name;
> > > > +       const char *section;
> > >
> > > what if we store a pointer to struct bpf_map * instead, that way we
> > > won't need to search, we'll just have a pointer ready
>
> We'd have to search *somewhere*. I'd rather have the search inside libbpf than
> inside the generated code. Besides, finding the right bpf_map from within the
> subskeleton is pretty annoying - you'll have to do suffix searches on the
> bpf_map names in the passed-in bpf_object and codegening all that is unnecessary
> when libbpf can look at real_name.

I think you misunderstood what I proposed. There is no explicit
searching. Here is a simple example of sub-skeleton struct and how
code-generated code will fill it out


struct my_subskel {
    struct {
        struct bpf_map *my_map;
    } maps;
    struct my_subskel__data {
        int *my_var;
    } data;
};


/* in codegen'ed code */

struct my_subskel *s;

subskel->syms[0].name = "my_var";
subskel->syms[0].map = &s->maps.data_syn;


It's similar in principle to how we define maps (that are found and
filled out by libbpf):

        s->maps[4].name = ".data.dyn";
        s->maps[4].map = &obj->maps.data_dyn;
        s->maps[4].mmaped = (void **)&obj->data_dyn;


Except in this case we use &s->maps.data_syn for reading, not for
writing into it.

Hope this is clearer now.


>
> >
>
> Thanks,
> Delyan