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

On Thu, Feb 3, 2022 at 8:40 AM Mauricio Vásquez Bernal
[off-list ref] wrote:

On Wed, Feb 2, 2022 at 2:31 PM Andrii Nakryiko
[off-list ref] wrote:

quoted

On Fri, Jan 28, 2022 at 2:33 PM Mauricio Vásquez [off-list ref] wrote:

quoted

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.

quoted

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.

quoted

quoted

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 <redacted>
Signed-off-by: Rafael David Tinoco <redacted>
Signed-off-by: Lorenzo Fontana <redacted>
Signed-off-by: Leonardo Di Donato <redacted>
---
 tools/bpf/bpftool/gen.c | 260 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 257 insertions(+), 3 deletions(-)

[...]

quoted

quoted

+
+       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).

quoted

quoted

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

[...]