Message-ID: <CAEf4BzYs=0BLAhNuJz=arN2VqWCc+L+nv7HqNaFV-bLYMP_1rw@mail.gmail.com>
Date:   Tue, 15 Feb 2022 17:23:56 -0800
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Mauricio Vásquez Bernal <mauricio@...volk.io>
Cc:     Networking <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andrii@...nel.org>,
        Quentin Monnet <quentin@...valent.com>,
        Rafael David Tinoco <rafaeldtinoco@...il.com>,
        Lorenzo Fontana <lorenzo.fontana@...stic.co>,
        Leonardo Di Donato <leonardo.didonato@...stic.co>
Subject: Re: [PATCH bpf-next v6 4/7] bpftool: Implement minimize_btf() and
 relocations recording for BTFGen

On Tue, Feb 15, 2022 at 2:56 PM Mauricio Vásquez Bernal
<mauricio@...volk.io> wrote:
>
> On Fri, Feb 11, 2022 at 7:42 PM Andrii Nakryiko
> <andrii.nakryiko@...il.com> wrote:
> >
> > On Wed, Feb 9, 2022 at 2:27 PM Mauricio Vásquez <mauricio@...volk.io> wrote:
> > >
> >
> > It would be good to shorten the subject line, it's very long.
> >
>
> Will do.
>
> > > 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.
>
> > >
> > > Signed-off-by: Mauricio Vásquez <mauricio@...volk.io>
> > > Signed-off-by: Rafael David Tinoco <rafael.tinoco@...asec.com>
> > > Signed-off-by: Lorenzo Fontana <lorenzo.fontana@...stic.co>
> > > Signed-off-by: Leonardo Di Donato <leonardo.didonato@...stic.co>
> > > ---
> > >  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.
> >
> > [...]
> >
> > > +}
> > > +
> > > +struct btfgen_info {
> > > +       struct btf *src_btf;
> > > +       struct btf *marked_btf; // btf structure used to mark used types
> >
> > C++ comment, please use /* */
> >
> > > +};
> > > +
> > > +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
> >
> > > +               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!
>
> >
> > > +               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.
>
> >
> > > +{
> > > +       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;
> >
> > [...]
> >
> > > +       /* 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.
>
> > > +       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

Ok, so you are just replacing what would be a pointer to forward
declaration with void *. Ok, I guess that works as well.

>
> > > +                       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;
> >
> > [...]
> >
> > > +err_out:
> > > +       bpf_core_free_cands(cands);
> > > +       errno = -err;
> > > +       return NULL;
> > > +}
> > > +
> > > +/* Record relocation information for a single BPF object*/
> >
> > nit: missing space before */
> >
> > > +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.
>
>
> > > +
> > > +       if (btf_ext->core_relo_info.len == 0) {
> > > +               err = 0;
> > > +               goto out;
> > > +       }
> > > +
> >
> > [...]

Powered by blists - more mailing lists