Re: [PATCH bpf-next v5 6/9] bpftool: Implement relocations recording for BTFGen

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

On Thu, Feb 3, 2022 at 8:40 AM Mauricio Vásquez Bernal
<mauricio@kinvolk.io> wrote:
>
> On Wed, Feb 2, 2022 at 2:31 PM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> >
> > On Fri, Jan 28, 2022 at 2:33 PM Mauricio Vásquez <mauricio@kinvolk.io> wrote:
> > >
> > > This commit implements the logic to record the relocation information
> > > for the different kind of relocations.
> > >
> > > btfgen_record_field_relo() uses the target specification to save all the
> > > types that are involved in a field-based CO-RE relocation. In this case
> > > types resolved and added recursively (using btfgen_put_type()).
> > > Only the struct and union members and their types) involved in the
> > > relocation are added to optimize the size of the generated BTF file.
> > >
> > > On the other hand, btfgen_record_type_relo() saves the types involved in
> > > a type-based CO-RE relocation. In this case all the members for the
> >
> > Do I understand correctly that if someone does
> > bpf_core_type_size(struct task_struct), you'll save not just
> > task_struct, but also any type that directly and indirectly referenced
> > from any task_struct's field, even if that is through a pointer.
>
> That's correct.
>
> > As
> > in, do you substitute forward declarations for types that are never
> > directly used? If not, that's going to be very suboptimal for
> > something like task_struct and any other type that's part of a big
> > cluster of types.
> >
>
> We decided to include the whole types and all direct and indirect
> types referenced from a structure field for type-based relocations.
> Our reasoning is that we don't know if the matching algorithm of
> libbpf could be changed to require more information in the future and
> type-based relocations are few compared to field based relocations.
>

It will depend on application and which type is used in relocation.
task_struct reaches tons of types and will add a very noticeable size
to minimized BTF, for no good reason, IMO. If we discover that we do
need those types, we'll update bpftool to generate more.


> If you are confident enough that adding empty structures/unions is ok
> then I'll update the algorithm. Actually it'll make our lives easier.
>

Well, test it of course, but I think it should work.

> > > struct and union types are added. This is not strictly required since
> > > libbpf doesn't use them while performing this kind of relocation,
> > > however that logic could change on the future. Additionally, we expect
> > > that the number of this kind of relocations in an BPF object to be very
> > > low, hence the impact on the size of the generated BTF should be
> > > negligible.
> > >
> > > Finally, btfgen_record_enumval_relo() saves the whole enum type for
> > > enum-based relocations.
> > >
> > > Signed-off-by: Mauricio Vásquez <mauricio@kinvolk.io>
> > > Signed-off-by: Rafael David Tinoco <rafael.tinoco@aquasec.com>
> > > Signed-off-by: Lorenzo Fontana <lorenzo.fontana@elastic.co>
> > > Signed-off-by: Leonardo Di Donato <leonardo.didonato@elastic.co>
> > > ---
> > >  tools/bpf/bpftool/gen.c | 260 +++++++++++++++++++++++++++++++++++++++-
> > >  1 file changed, 257 insertions(+), 3 deletions(-)
> > >

[...]

> > > +
> > > +       btfgen_member = calloc(1, sizeof(*btfgen_member));
> > > +       if (!btfgen_member)
> > > +               return -ENOMEM;
> > > +       btfgen_member->member = btf_member;
> > > +       btfgen_member->idx = idx;
> > > +       /* add btf_member as member to given btfgen_type */
> > > +       err = hashmap__add(btfgen_type->members, uint_as_hash_key(btfgen_member->idx),
> > > +                          btfgen_member);
> > > +       if (err) {
> > > +               free(btfgen_member);
> > > +               if (err != -EEXIST)
> >
> > why not check that such a member exists before doing btfgen_member allocation?
> >
>
> I thought that it could be more efficient calling hashmap__add()
> directly without checking and then handling the case when it was
> already there. Having a second thought it seems to me that it's not
> always true and depends on how many times the code follows each path,
> what we don't know. I'll change it to check if it's there before.
>

See my other reply on this patch. Maybe you won't need a hashmap at
all if you modify btf_type in place (As in, set extra bit to mark that
type or its member is needed)? It feels a bit hacky, but this is an
internal and one specific case inside bpftool, so I think it's
justified (and it will be much cleaner and shorter code, IMO).

> > > +                       return err;
> > > +       }
> > > +
> > > +       return 0;
> > > +}
> > > +

[...]