bpf.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Andrii Nakryiko <andrii.nakryiko@gmail.com>
To: "Mauricio Vásquez" <mauricio@kinvolk.io>
Cc: Networking <netdev@vger.kernel.org>, bpf <bpf@vger.kernel.org>,
	Alexei Starovoitov <ast@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Andrii Nakryiko <andrii@kernel.org>,
	Quentin Monnet <quentin@isovalent.com>,
	Rafael David Tinoco <rafaeldtinoco@gmail.com>,
	Lorenzo Fontana <lorenzo.fontana@elastic.co>,
	Leonardo Di Donato <leonardo.didonato@elastic.co>
Subject: Re: [PATCH bpf-next v3 3/3] bpftool: Implement btfgen
Date: Wed, 22 Dec 2021 16:33:15 -0800	[thread overview]
Message-ID: <CAEf4BzZ6E_iRT-pBon5G6xA0kK=EYKSZQ3zDLVKQmTT8A12J_Q@mail.gmail.com> (raw)
In-Reply-To: <20211217185654.311609-4-mauricio@kinvolk.io>

On Fri, Dec 17, 2021 at 10:57 AM Mauricio Vásquez <mauricio@kinvolk.io> wrote:
>
> The BTFGen's goal is to produce a BTF file that contains **only** the
> information that is needed by an eBPF program. This algorithm does a
> first step collecting the types involved for each relocation present on
> the object and "marking" them as needed. Types are collected in
> different ways according to the type relocation, for field based
> relocations only the union and structures members involved are
> considered, for type based relocations the whole types are added, enum
> field relocations are not supported in this iteration yet.
>
> A second step generates a BTF file from the "marked" types. This step
> accesses the original BTF file extracting the types and their members
> that were "marked" as needed in the first step.
>
> This command is implemented under the "gen" command in bpftool and the
> syntax is the following:
>
> $ bpftool gen btf INPUT OUTPUT OBJECT(S)
>
> INPUT can be either a single BTF file or a folder containing BTF files,
> when it's a folder, a BTF file is generated for each BTF file contained
> in this folder. OUTPUT is the file (or folder) where generated files are
> stored and OBJECT(S) is the list of bpf objects we want to generate the
> BTF file(s) for (each generated BTF file contains all the types needed
> by all the objects).
>
> 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 | 892 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 892 insertions(+)
>

I haven't looked through details of stripping BTF itself, let's
finalize CO-RE relocation parts first. Maybe for the next revision you
could split bpftool changes in two in some reasonable way so that each
patch concentrates on different steps of the process a bit more? E.g.,
first patch might set up new command and BTF stripping parts but leave
the CO-RE relocation logic unimplemented, and the second path fills in
that part. Should make it easier to review this big patch.

Please also cc Quentin Monnet to review bpftool parts as well.


[...]

> +static int btf_reloc_info_gen_type(struct btf_reloc_info *info, struct bpf_core_spec *targ_spec)
> +{
> +       struct btf *btf = (struct btf *) info->src_btf;
> +       struct btf_type *btf_type;
> +       int err = 0;
> +
> +       btf_type = (struct btf_type *) btf__type_by_id(btf, targ_spec->root_type_id);
> +
> +       return btf_reloc_put_type_all(btf, info, btf_type, targ_spec->root_type_id);
> +}
> +
> +static int btf_reloc_info_gen_enumval(struct btf_reloc_info *info, struct bpf_core_spec *targ_spec)
> +{
> +       p_err("untreated enumval based relocation");

why untreated? what's the problem supporting it?

> +       return -EOPNOTSUPP;
> +}
> +
> +static int btf_reloc_info_gen(struct btf_reloc_info *info, struct bpf_core_spec *res)
> +{
> +       if (core_relo_is_type_based(res->relo_kind))
> +               return btf_reloc_info_gen_type(info, res);
> +
> +       if (core_relo_is_enumval_based(res->relo_kind))
> +               return btf_reloc_info_gen_enumval(info, res);
> +
> +       if (core_relo_is_field_based(res->relo_kind))
> +               return btf_reloc_info_gen_field(info, res);

you can have a simple switch here instead of exposing libbpf internal helpers

> +
> +       return -EINVAL;
> +}
> +
> +#define BPF_INSN_SZ (sizeof(struct bpf_insn))
> +
> +static int btfgen_obj_reloc_info_gen(struct btf_reloc_info *reloc_info, struct bpf_object *obj)
> +{
> +       const struct btf_ext_info_sec *sec;
> +       const struct bpf_core_relo *rec;
> +       const struct btf_ext_info *seg;
> +       struct hashmap *cand_cache;
> +       int err, insn_idx, sec_idx;
> +       struct bpf_program *prog;
> +       struct btf_ext *btf_ext;
> +       const char *sec_name;
> +       size_t nr_programs;
> +       struct btf *btf;
> +       unsigned int i;
> +
> +       btf = bpf_object__btf(obj);
> +       btf_ext = bpf_object__btf_ext(obj);
> +
> +       if (btf_ext->core_relo_info.len == 0)
> +               return 0;
> +
> +       cand_cache = bpf_core_create_cand_cache();
> +       if (IS_ERR(cand_cache))
> +               return PTR_ERR(cand_cache);
> +
> +       bpf_object_set_vmlinux_override(obj, reloc_info->src_btf);
> +
> +       seg = &btf_ext->core_relo_info;
> +       for_each_btf_ext_sec(seg, sec) {
> +               bool prog_found;
> +
> +               sec_name = btf__name_by_offset(btf, sec->sec_name_off);
> +               if (str_is_empty(sec_name)) {
> +                       err = -EINVAL;
> +                       goto out;
> +               }
> +
> +               prog_found = false;
> +               nr_programs = bpf_object__get_nr_programs(obj);
> +               for (i = 0; i < nr_programs; i++)       {
> +                       prog = bpf_object__get_program(obj, i);
> +                       if (strcmp(bpf_program__section_name(prog), sec_name) == 0) {
> +                               prog_found = true;
> +                               break;
> +                       }
> +               }
> +
> +               if (!prog_found) {
> +                       pr_warn("sec '%s': failed to find a BPF program\n", sec_name);
> +                       err = -EINVAL;
> +                       goto out;
> +               }
> +
> +               sec_idx = bpf_program__sec_idx(prog);
> +
> +               for_each_btf_ext_rec(seg, sec, i, rec) {
> +                       struct bpf_core_relo_res targ_res;
> +                       struct bpf_core_spec targ_spec;
> +
> +                       insn_idx = rec->insn_off / BPF_INSN_SZ;
> +
> +                       prog = find_prog_by_sec_insn(obj, sec_idx, insn_idx);
> +                       if (!prog) {
> +                               pr_warn("sec '%s': failed to find program at insn #%d for CO-RE offset relocation #%d\n",
> +                                       sec_name, insn_idx, i);
> +                               err = -EINVAL;
> +                               goto out;
> +                       }
> +
> +                       err = bpf_core_calc_relo_res(prog, rec, i, btf, cand_cache, &targ_res,
> +                                                    &targ_spec);


I don't think you need to do *exactly* what libbpf is doing.
bpf_core_calc_relo_res() doesn't add much on top of
bpf_core_calc_relo_insn(), if you use bpf_core_calc_relo_insn()
directly and expose bpf_core_add_cands/bpf_core_free_cands and use
them directly as well, bypassing bpf_object completely, you won't need
btf_vmlinux override and make everything less coupled, I think. In the
future, if you'd like to support BTF module BTFs, you'll have all the
necessary flexibility to do that, while if you try to reuse every
single line of bpf_object's code we'll be adding more hacks like your
bpf_object_set_vmlinux_override().

Fundamentally, you don't care about bpf_object and bpf_programs. All
you need to do is parse .BTF.ext (you can do that just btf__parse, no
need to even construct bpf_object!). Construct candidate cache, then
iterate each CO-RE record, add find/add candidates, calculate
relocation result, use it for your algorithm.

Yes, there will be a bit of duplication (candidate search), but that's
better than trying to turn bpf_object inside out with all the custom
getters/setters that you are exposing (even if it's libbpf internal
only, still makes it really hard to reasons what's going on and what
are consequences of manual control over a lot of bpf_object internal
implementation details).


> +                       if (err)
> +                               goto out;
> +
> +                       err = btf_reloc_info_gen(reloc_info, &targ_spec);
> +                       if (err)
> +                               goto out;
> +               }
> +       }
> +
> +out:
> +       bpf_core_free_cand_cache(cand_cache);
> +
> +       return err;
> +}
> +

[...]

  reply	other threads:[~2021-12-23  0:33 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-12-17 18:56 [PATCH bpf-next v3 0/3] libbpf: Implement BTFGen Mauricio Vásquez
2021-12-17 18:56 ` [PATCH bpf-next v3 1/3] libbpf: split bpf_core_apply_relo() Mauricio Vásquez
2021-12-23  0:33   ` Andrii Nakryiko
2022-01-12 14:26     ` Mauricio Vásquez Bernal
2021-12-17 18:56 ` [PATCH bpf-next v3 2/3] libbpf: Implement changes needed for BTFGen in bpftool Mauricio Vásquez
2021-12-23  0:33   ` Andrii Nakryiko
2022-01-12 14:26     ` Mauricio Vásquez Bernal
2021-12-17 18:56 ` [PATCH bpf-next v3 3/3] bpftool: Implement btfgen Mauricio Vásquez
2021-12-23  0:33   ` Andrii Nakryiko [this message]
2022-01-12 14:26     ` Mauricio Vásquez Bernal
2021-12-17 23:11 ` [PATCH bpf-next v3 0/3] libbpf: Implement BTFGen Daniel Borkmann
2021-12-20 22:43   ` Mauricio Vásquez Bernal

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAEf4BzZ6E_iRT-pBon5G6xA0kK=EYKSZQ3zDLVKQmTT8A12J_Q@mail.gmail.com' \
    --to=andrii.nakryiko@gmail.com \
    --cc=andrii@kernel.org \
    --cc=ast@kernel.org \
    --cc=bpf@vger.kernel.org \
    --cc=daniel@iogearbox.net \
    --cc=leonardo.didonato@elastic.co \
    --cc=lorenzo.fontana@elastic.co \
    --cc=mauricio@kinvolk.io \
    --cc=netdev@vger.kernel.org \
    --cc=quentin@isovalent.com \
    --cc=rafaeldtinoco@gmail.com \
    --subject='Re: [PATCH bpf-next v3 3/3] bpftool: Implement btfgen' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).