Date:   Fri, 25 Sep 2020 09:53:14 -0700
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Alexei Starovoitov <ast@...com>
Cc:     Alexei Starovoitov <alexei.starovoitov@...il.com>,
        Andrii Nakryiko <andriin@...com>, bpf <bpf@...r.kernel.org>,
        Networking <netdev@...r.kernel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Kernel Team <kernel-team@...com>,
        Arnaldo Carvalho de Melo <acme@...hat.com>
Subject: Re: [PATCH bpf-next 7/9] libbpf: add BTF writing APIs

On Fri, Sep 25, 2020 at 8:37 AM Alexei Starovoitov <ast@...com> wrote:
>
> On 9/24/20 11:21 PM, Andrii Nakryiko wrote:
> > On Thu, Sep 24, 2020 at 8:55 PM Alexei Starovoitov
> > <alexei.starovoitov@...il.com> wrote:
> >>
> >> On Wed, Sep 23, 2020 at 08:54:34AM -0700, Andrii Nakryiko wrote:
> >>> Add APIs for appending new BTF types at the end of BTF object.
> >>>
> >>> Each BTF kind has either one API of the form btf__append_<kind>(). For types
> >>> that have variable amount of additional items (struct/union, enum, func_proto,
> >>> datasec), additional API is provided to emit each such item. E.g., for
> >>> emitting a struct, one would use the following sequence of API calls:
> >>>
> >>> btf__append_struct(...);
> >>> btf__append_field(...);
> >>> ...
> >>> btf__append_field(...);
> >>
> >> I've just started looking through the diffs. The first thing that struck me
> >> is the name :) Why 'append' instead of 'add' ? The latter is shorter.
> >
> > Append is very precise about those types being added at the end. Add
> > is more ambiguous in that sense and doesn't imply any specific order.
> > E.g., for btf__add_str() that's suitable, because the order in which
> > strings are inserted might be different (e.g., if the string is
> > duplicated). But it's not an "insert" either, so I'm fine with
> > renaming to "add", if you prefer it.
>
> The reason I prefer shorter is to be able to write:
> btf__add_var(btf, "my_var", global,
>               btf__add_const(btf,
>               btf__add_volatile(btf,
>               btf__add_ptr(btf,
>               btf__add_int(btf, "int", 4, signed))));

That's an interesting way of using it, I'll give you that :)

Ok, I'll switch to "add" name.

>
> In other words the shorter the type the more suitable the api
> will be for manual construction of types.
> Looks like the api already checks for invalid type_id,
> so no need to check validity at every build stage.
> Hence it's nice to combine multiple btf__add_*() into single line.
>
> I think C language isn't great for 'constructor' style api.
> May be on top of the above api we can add c++-like api?
> For example we can define
> struct btf_builder {
>     struct btf_builder * (*_volatile) (void);
>     struct btf_builder * (*_const) (void);
>     struct btf_builder * (*_int) (char *name, int sz, int encoding);
>     struct btf_builder * (_ptr) (void);
> };
>
> and the use it as:
>      struct btf_builder *b = btf__create_global_builer(...);
>
>      b->_int("int", 4, singed)
>       ->_const()
>       ->_volatile()
>       ->_ptr()
>       ->_var("my_var", global);
>
> Every method will be return its own object (only one such object)
> while the actual building will be happening in some 'invisible',
> global, and mutex protected place.
>
> >>
> >> Also how would you add anon struct that is within another struct ?
> >> The anon one would have to be added first and then added as a field?
> >> Feels a bit odd that struct/union building doesn't have 'finish' method,
> >> but I guess it can work.
> >
> > That embedded anon struct will be a separate type, then the field
> > (anonymous or not, that's orthogonal to anonymity of a struct (!))
> > will refer to that anon struct type by its ID. Anon struct can be
> > added right before, right after, or in between completely unrelated
> > types, it doesn't matter to BTF itself as long as all the type IDs
> > match in the end.
> >
> > As for the finish method... There wasn't a need so far to have it, as
> > BTF constantly maintains correct vlen for the "current"
> > struct/union/func_proto/datasec/enum (anything with extra members).
> > I've been writing a few more tests than what I posted here (they will
> > come in the second wave of patches) and converted pahole to this new
> > API completely. And it does feel pretty nice and natural so far. In
> > the future we might add something like that, I suppose, that would
> > allow to do some more validations at the end. But that would be a
> > non-breaking extension, so I don't want to do it right now.
>
> sure. that's fine.
> Also I suspect sooner or later would be good to do at least partial
> dedup of types while they're being built.
> Instead of passing exact btf_id of 'int' everywhere it would be
> human friendlier to say:
>    b->_int("int", 4, singed)->_var("my_var")
> instead of having extra variable that holds btf_id of 'int' and
> use as it:
>    u32 btf_id_of_int; /* that is used everywhere where 'int' field of
> var needs to be added */
>    b->__var("my_var", btf_id_of_int);
>
> I mean since types are being built the real dedup cannot happen,
> but dedup of simple types can happen on the fly.
> That will justify 'add' vs 'append' as well.
> Just a thought.
>

That's doable, though certainly added complexity. Unless you need to
generate millions of those variables, just appending "int"s many times
and then doing dedup once would be faster and simpler, using just a
touch more memory. But certainly something to keep in mind.

Powered by blists - more mailing lists