Re: [PATCH bpf-next v6 4/7] bpftool: Implement minimize_btf() and relocations recording for BTFGen

On Fri, Feb 11, 2022 at 7:42 PM Andrii Nakryiko
[off-list ref] wrote:

On Wed, Feb 9, 2022 at 2:27 PM Mauricio Vásquez [off-list ref] wrote:

quoted

It would be good to shorten the subject line, it's very long.

Will do.

quoted

minimize_btf() receives the path of a source and destination BTF files
and a list of BPF objects. This function records the relocations for
all objects and then generates the BTF file by calling btfgen_get_btf()
(implemented in the following commit).

btfgen_record_obj() loads the BTF and BTF.ext sections of the BPF
objects and loops through all CO-RE relocations. It uses
bpf_core_calc_relo_insn() from libbpf and passes the target spec to
btfgen_record_reloc(), that calls one of the following functions
depending on the relocation kind.

btfgen_record_field_relo() uses the target specification to mark all the
types that are involved in a field-based CO-RE relocation. In this case
types resolved and marked recursively using btfgen_mark_type().
Only the struct and union members (and their types) involved in the
relocation are marked to optimize the size of the generated BTF file.

btfgen_record_type_relo() marks the types involved in a type-based
CO-RE relocation. In this case no members for the struct and union
types are marked as libbpf doesn't use them while performing this kind
of relocation. Pointed types are marked as they are used by libbpf in
this case.

btfgen_record_enumval_relo() marks the whole enum type for enum-based
relocations.

It should be enough to leave only used enumerators, but I suppose it
doesn't take much space to record all. We can adjust that later, if
necessary.

I think the overhead is really minimal and we can improve later on if we want.

quoted

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/Makefile |   8 +-
 tools/bpf/bpftool/gen.c    | 452 ++++++++++++++++++++++++++++++++++++-
 2 files changed, 454 insertions(+), 6 deletions(-)

Looks good, few nits and concerns, but it feels like it's really close
to being ready.

[...]

quoted

+}
+
+struct btfgen_info {
+       struct btf *src_btf;
+       struct btf *marked_btf; // btf structure used to mark used types

C++ comment, please use /* */

quoted

+};
+
+static size_t btfgen_hash_fn(const void *key, void *ctx)
+{
+       return (size_t)key;
+}
+
+static bool btfgen_equal_fn(const void *k1, const void *k2, void *ctx)
+{
+       return k1 == k2;
+}
+
+static void *uint_as_hash_key(int x)
+{
+       return (void *)(uintptr_t)x;
+}
+
+static void *u32_as_hash_key(__u32 x)
+{
+       return (void *)(uintptr_t)x;
+}
+
+static void btfgen_free_info(struct btfgen_info *info)
+{
+       if (!info)
+               return;
+
+       btf__free(info->src_btf);
+       btf__free(info->marked_btf);
+
+       free(info);
+}
+
+static struct btfgen_info *
+btfgen_new_info(const char *targ_btf_path)
+{
+       struct btfgen_info *info;
+       int err;
+
+       info = calloc(1, sizeof(*info));
+       if (!info)
+               return NULL;
+
+       info->src_btf = btf__parse(targ_btf_path, NULL);
+       if (!info->src_btf) {
+               p_err("failed parsing '%s' BTF file: %s", targ_btf_path, strerror(errno));
+               err = -errno;

save errno before p_err, it can clobber errno otherwise

quoted

+               goto err_out;
+       }
+
+       info->marked_btf = btf__parse(targ_btf_path, NULL);
+       if (!info->marked_btf) {
+               p_err("failed parsing '%s' BTF file: %s", targ_btf_path, strerror(errno));
+               err = -errno;

same, always save errno first before any non-trivial function/macro call

oh right, thanks!

quoted

+               goto err_out;
+       }
+
+       return info;
+
+err_out:
+       btfgen_free_info(info);
+       errno = -err;
+       return NULL;
+}
+
+#define MARKED UINT32_MAX
+
+static void btfgen_mark_member(struct btfgen_info *info, int type_id, int idx)
+{
+       const struct btf_type *t = btf__type_by_id(info->marked_btf, type_id);
+       struct btf_member *m = btf_members(t) + idx;
+
+       m->name_off = MARKED;
+}
+
+static int
+btfgen_mark_type(struct btfgen_info *info, unsigned int id, bool follow_pointers)

id is type_id or could be some other id? It's best to be consistent in
naming to avoid second guessing like in this case.

It's always type_id. Renamed it.

quoted

+{
+       const struct btf_type *btf_type = btf__type_by_id(info->src_btf, id);
+       struct btf_type *cloned_type;
+       struct btf_param *param;
+       struct btf_array *array;
+       int err, i;

[...]

quoted

+       /* tells if some other type needs to be handled */
+       default:
+               p_err("unsupported kind: %s (%d)", btf_kind_str(btf_type), id);
+               return -EINVAL;
+       }
+
+       return 0;
+}
+
+static int btfgen_record_field_relo(struct btfgen_info *info, struct bpf_core_spec *targ_spec)
+{
+       struct btf *btf = (struct btf *) info->src_btf;

why the cast?

No reason. Will remove it.

quoted

+       const struct btf_type *btf_type;
+       struct btf_member *btf_member;
+       struct btf_array *array;
+       unsigned int id = targ_spec->root_type_id;
+       int idx, err;
+
+       /* mark root type */
+       btf_type = btf__type_by_id(btf, id);
+       err = btfgen_mark_type(info, id, false);
+       if (err)
+               return err;
+
+       /* mark types for complex types (arrays, unions, structures) */
+       for (int i = 1; i < targ_spec->raw_len; i++) {
+               /* skip typedefs and mods */
+               while (btf_is_mod(btf_type) || btf_is_typedef(btf_type)) {
+                       id = btf_type->type;
+                       btf_type = btf__type_by_id(btf, id);
+               }
+
+               switch (btf_kind(btf_type)) {
+               case BTF_KIND_STRUCT:
+               case BTF_KIND_UNION:
+                       idx = targ_spec->raw_spec[i];
+                       btf_member = btf_members(btf_type) + idx;
+
+                       /* mark member */
+                       btfgen_mark_member(info, id, idx);
+
+                       /* mark member's type */
+                       id = btf_member->type;
+                       btf_type = btf__type_by_id(btf, id);
+                       err = btfgen_mark_type(info, id, false);

why would it not follow the pointer? E.g., if I have a field defined as

struct blah ***my_field;

You at the very least would need either an empty struct blah or FWD
for struct blah, no?

It's an optimization we do, we don't follow the pointer here because
it is possible that the definition of the pointed type is not needed.
For instance, a relocation like:

BPF_CORE_READ(task, nsproxy);

will generate this:

[1] STRUCT 'task_struct' size=9472 vlen=1
    'nsproxy' type_id=2 bits_offset=23040
[2] PTR '(anon)' type_id=0

struct nsproxy is not really accessed, so we don't need it's
definition. On the other hand, something like

BPF_CORE_READ(task, nsproxy, count);

has two relocations, and nsproxy is actually accessed, so in this case
the generated BTF includes a nsproxy struct:

[1] STRUCT '(anon)' size=4 vlen=0
[2] TYPEDEF 'atomic_t' type_id=1
[3] STRUCT 'task_struct' size=9472 vlen=1
    'nsproxy' type_id=4 bits_offset=23040
[4] PTR '(anon)' type_id=5
[5] STRUCT 'nsproxy' size=72 vlen=1
    'count' type_id=2 bits_offset=0

quoted

+                       if (err)
+                               return err;
+                       break;
+               case BTF_KIND_ARRAY:
+                       array = btf_array(btf_type);
+                       id = array->type;
+                       btf_type = btf__type_by_id(btf, id);
+                       break;

[...]

quoted

+err_out:
+       bpf_core_free_cands(cands);
+       errno = -err;
+       return NULL;
+}
+
+/* Record relocation information for a single BPF object*/

nit: missing space before */

quoted

+static int btfgen_record_obj(struct btfgen_info *info, const char *obj_path)
+{
+       const struct btf_ext_info_sec *sec;
+       const struct bpf_core_relo *relo;
+       const struct btf_ext_info *seg;
+       struct hashmap_entry *entry;
+       struct hashmap *cand_cache = NULL;
+       struct btf_ext *btf_ext = NULL;
+       unsigned int relo_idx;
+       struct btf *btf = NULL;
+       size_t i;
+       int err;
+
+       btf = btf__parse(obj_path, &btf_ext);
+       if (!btf) {
+               p_err("failed to parse BPF object '%s': %s", obj_path, strerror(errno));
+               return -errno;
+       }

check that btf_ext is not NULL?

Done.

quoted

+
+       if (btf_ext->core_relo_info.len == 0) {
+               err = 0;
+               goto out;
+       }
+

[...]