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 v6 4/7] bpftool: Implement minimize_btf() and relocations recording for BTFGen
Date: Fri, 11 Feb 2022 16:42:33 -0800 [thread overview]
Message-ID: <CAEf4BzZaVTdsQbFhStzNavHMhkv4yVm=yc2vqsgFQnZqKZfXpg@mail.gmail.com> (raw)
In-Reply-To: <20220209222646.348365-5-mauricio@kinvolk.io>
On Wed, Feb 9, 2022 at 2:27 PM Mauricio Vásquez <mauricio@kinvolk.io> wrote:
>
It would be good to shorten the subject line, it's very long.
> 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.
>
> 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/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
> + 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.
> +{
> + 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?
> + 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?
> + 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?
> +
> + if (btf_ext->core_relo_info.len == 0) {
> + err = 0;
> + goto out;
> + }
> +
[...]
next prev parent reply other threads:[~2022-02-12 0:42 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-02-09 22:26 [PATCH bpf-next v6 0/7] libbpf: Implement BTFGen Mauricio Vásquez
2022-02-09 22:26 ` [PATCH bpf-next v6 1/7] libbpf: split bpf_core_apply_relo() Mauricio Vásquez
2022-02-12 0:42 ` Andrii Nakryiko
2022-02-09 22:26 ` [PATCH bpf-next v6 2/7] libbpf: Expose bpf_core_{add,free}_cands() to bpftool Mauricio Vásquez
2022-02-12 0:42 ` Andrii Nakryiko
2022-02-09 22:26 ` [PATCH bpf-next v6 3/7] bpftool: Add gen min_core_btf command Mauricio Vásquez
2022-02-12 0:42 ` Andrii Nakryiko
2022-02-09 22:26 ` [PATCH bpf-next v6 4/7] bpftool: Implement minimize_btf() and relocations recording for BTFGen Mauricio Vásquez
2022-02-12 0:42 ` Andrii Nakryiko [this message]
2022-02-15 22:56 ` Mauricio Vásquez Bernal
2022-02-16 1:23 ` Andrii Nakryiko
2022-02-09 22:26 ` [PATCH bpf-next v6 5/7] bpftool: Implement btfgen_get_btf() Mauricio Vásquez
2022-02-12 0:42 ` Andrii Nakryiko
2022-02-15 22:56 ` Mauricio Vásquez Bernal
2022-02-09 22:26 ` [PATCH bpf-next v6 6/7] bpftool: gen min_core_btf explanation and examples Mauricio Vásquez
2022-02-12 0:42 ` Andrii Nakryiko
2022-02-15 22:56 ` Mauricio Vásquez Bernal
2022-02-16 1:26 ` Andrii Nakryiko
2022-02-09 22:26 ` [PATCH bpf-next v6 7/7] selftests/bpf: Test "bpftool gen min_core_btf" Mauricio Vásquez
2022-02-12 0:42 ` Andrii Nakryiko
2022-02-15 22:56 ` 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='CAEf4BzZaVTdsQbFhStzNavHMhkv4yVm=yc2vqsgFQnZqKZfXpg@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 \
/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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).