bpf.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: "Mauricio Vásquez Bernal" <mauricio@kinvolk.io>
To: Andrii Nakryiko <andrii.nakryiko@gmail.com>
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 2/3] libbpf: Implement changes needed for BTFGen in bpftool
Date: Wed, 12 Jan 2022 09:26:55 -0500	[thread overview]
Message-ID: <CAHap4zsy39sGWvW8-aXt3kZweDxXK6gmaFjy28W4qCp1fvywhw@mail.gmail.com> (raw)
In-Reply-To: <CAEf4BzbtbxkPZKUEye9=CbOtR-e25cv_5_FyH_Qd8hk9TtsiJQ@mail.gmail.com>

> > @@ -5498,12 +5498,13 @@ static int record_relo_core(struct bpf_program *prog,
> >         return 0;
> >  }
> >
> > -static int bpf_core_calc_relo_res(struct bpf_program *prog,
> > -                                 const struct bpf_core_relo *relo,
> > -                                 int relo_idx,
> > -                                 const struct btf *local_btf,
> > -                                 struct hashmap *cand_cache,
> > -                                 struct bpf_core_relo_res *targ_res)
> > +int bpf_core_calc_relo_res(struct bpf_program *prog,
> > +                          const struct bpf_core_relo *relo,
> > +                          int relo_idx,
> > +                          const struct btf *local_btf,
> > +                          struct hashmap *cand_cache,
> > +                          struct bpf_core_relo_res *targ_res,
> > +                          struct bpf_core_spec *targ_spec)
>
> maybe let's add targ_spec and local_spec into bpf_core_relo_res? that
> way bpf_core_relo_res contains all the relevant information around
> CO-RE relo resolution?
>

It's not needed anymore now that we're using bpf_core_calc_relo_insn()
directly in bpftool.

> > @@ -8190,6 +8211,11 @@ struct btf *bpf_object__btf(const struct bpf_object *obj)
> >         return obj ? obj->btf : NULL;
> >  }
> >
> > +struct btf_ext *bpf_object__btf_ext(const struct bpf_object *obj)
> > +{
> > +       return obj ? obj->btf_ext : NULL;
>
> just return obj->btf_ext, no one should be passing NULL for those getters

I dropped this function as we don't need it now.

>
> > +}
> > +
> >  int bpf_object__btf_fd(const struct bpf_object *obj)
> >  {
> >         return obj->btf ? btf__fd(obj->btf) : -1;
> > @@ -8281,6 +8307,20 @@ bpf_object__next_program(const struct bpf_object *obj, struct bpf_program *prev)
> >         return prog;
> >  }
> >
> > +size_t bpf_object__get_nr_programs(const struct bpf_object *obj)
> > +{
> > +       return obj->nr_programs;
> > +}
> > +
> > +struct bpf_program *
> > +bpf_object__get_program(const struct bpf_object *obj, unsigned int i)
> > +{
> > +       if (i >= obj->nr_programs)
> > +               return NULL;
> > +
> > +       return &obj->programs[i];
> > +}
> > +
> >  struct bpf_program *
> >  bpf_program__prev(struct bpf_program *next, const struct bpf_object *obj)
> >  {
> > @@ -8360,6 +8400,11 @@ int bpf_program__set_autoload(struct bpf_program *prog, bool autoload)
> >         return 0;
> >  }
> >
> > +int bpf_program__sec_idx(const struct bpf_program *prog)
> > +{
> > +       return prog->sec_idx;
> > +}
> > +
> >  static int bpf_program_nth_fd(const struct bpf_program *prog, int n);
> >
> >  int bpf_program__fd(const struct bpf_program *prog)
> > @@ -11779,3 +11824,8 @@ void bpf_object__destroy_skeleton(struct bpf_object_skeleton *s)
> >         free(s->progs);
> >         free(s);
> >  }
> > +
> > +void bpf_object_set_vmlinux_override(struct bpf_object *obj, struct btf *btf)
> > +{
> > +       obj->btf_vmlinux_override = btf;
> > +}
>
> I don't think we need this, see comments in next patch
>
>
> > diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> > index 42b2f36fd9f0..2b048ee5a9b2 100644
> > --- a/tools/lib/bpf/libbpf.h
> > +++ b/tools/lib/bpf/libbpf.h
> > @@ -225,6 +225,8 @@ LIBBPF_API int bpf_object__set_kversion(struct bpf_object *obj, __u32 kern_versi
> >
> >  struct btf;
> >  LIBBPF_API struct btf *bpf_object__btf(const struct bpf_object *obj);
> > +struct btf_ext;
> > +LIBBPF_API struct btf_ext *bpf_object__btf_ext(const struct bpf_object *obj);
> >  LIBBPF_API int bpf_object__btf_fd(const struct bpf_object *obj);
> >
> >  LIBBPF_DEPRECATED_SINCE(0, 7, "use bpf_object__find_program_by_name() instead")
> > @@ -290,6 +292,7 @@ LIBBPF_API LIBBPF_DEPRECATED("BPF program title is confusing term; please use bp
> >  const char *bpf_program__title(const struct bpf_program *prog, bool needs_copy);
> >  LIBBPF_API bool bpf_program__autoload(const struct bpf_program *prog);
> >  LIBBPF_API int bpf_program__set_autoload(struct bpf_program *prog, bool autoload);
> > +LIBBPF_API int bpf_program__sec_idx(const struct bpf_program *prog);
> >
> >  /* returns program size in bytes */
> >  LIBBPF_DEPRECATED_SINCE(0, 7, "use bpf_program__insn_cnt() instead")
> > diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
> > index b3938b3f8fc9..15da4075e0b5 100644
> > --- a/tools/lib/bpf/libbpf.map
> > +++ b/tools/lib/bpf/libbpf.map
> > @@ -392,6 +392,7 @@ LIBBPF_0.6.0 {
> >                 bpf_map__map_extra;
> >                 bpf_map__set_map_extra;
> >                 bpf_map_create;
> > +               bpf_object__btf_ext;
> >                 bpf_object__next_map;
> >                 bpf_object__next_program;
> >                 bpf_object__prev_map;
> > @@ -401,6 +402,7 @@ LIBBPF_0.6.0 {
> >                 bpf_program__flags;
> >                 bpf_program__insn_cnt;
> >                 bpf_program__insns;
> > +               bpf_program__sec_idx;
> >                 bpf_program__set_flags;
> >                 btf__add_btf;
> >                 btf__add_decl_tag;
> > diff --git a/tools/lib/bpf/libbpf_internal.h b/tools/lib/bpf/libbpf_internal.h
> > index 5dbe4f463880..b1962adb110c 100644
> > --- a/tools/lib/bpf/libbpf_internal.h
> > +++ b/tools/lib/bpf/libbpf_internal.h
> > @@ -524,4 +524,26 @@ static inline int ensure_good_fd(int fd)
> >         return fd;
> >  }
> >
> > +struct hashmap;
> > +
> > +int bpf_core_calc_relo_res(struct bpf_program *prog,
> > +                          const struct bpf_core_relo *relo,
> > +                          int relo_idx,
> > +                          const struct btf *local_btf,
> > +                          struct hashmap *cand_cache,
> > +                          struct bpf_core_relo_res *targ_res,
> > +                          struct bpf_core_spec *targ_spec);
> > +void bpf_object_set_vmlinux_override(struct bpf_object *obj, struct btf *btf);
> > +struct hashmap *bpf_core_create_cand_cache(void);
> > +void bpf_core_free_cand_cache(struct hashmap *cand_cache);
> > +
> > +struct bpf_program *find_prog_by_sec_insn(const struct bpf_object *obj,
> > +                                         size_t sec_idx, size_t insn_idx);
> > +
> > +size_t bpf_object__get_nr_programs(const struct bpf_object *obj);
> > +
> > +struct bpf_program *
> > +bpf_object__get_program(const struct bpf_object *obj, unsigned int n);
> > +
>
> that's too much, I don't think you need bpf_program and all the things
> around it (sec_idx, look up, etc). As for core_relo_is_field_based and
> co, bpftool can do those simple checks on their own, no need to make
> all the "internal API", don't overdo it with "let's expose internals
> of libbpf to bpftool".
>
>
> > +
> >  #endif /* __LIBBPF_LIBBPF_INTERNAL_H */
> > diff --git a/tools/lib/bpf/relo_core.c b/tools/lib/bpf/relo_core.c
> > index 4f3552468624..66dfb7fa89a2 100644
> > --- a/tools/lib/bpf/relo_core.c
> > +++ b/tools/lib/bpf/relo_core.c
> > @@ -102,7 +102,7 @@ static const char *core_relo_kind_str(enum bpf_core_relo_kind kind)
> >         }
> >  }
> >
> > -static bool core_relo_is_field_based(enum bpf_core_relo_kind kind)
> > +bool core_relo_is_field_based(enum bpf_core_relo_kind kind)
> >  {
> >         switch (kind) {
> >         case BPF_CORE_FIELD_BYTE_OFFSET:
> > @@ -117,7 +117,7 @@ static bool core_relo_is_field_based(enum bpf_core_relo_kind kind)
> >         }
> >  }
> >
> > -static bool core_relo_is_type_based(enum bpf_core_relo_kind kind)
> > +bool core_relo_is_type_based(enum bpf_core_relo_kind kind)
> >  {
> >         switch (kind) {
> >         case BPF_CORE_TYPE_ID_LOCAL:
> > @@ -130,7 +130,7 @@ static bool core_relo_is_type_based(enum bpf_core_relo_kind kind)
> >         }
> >  }
> >
> > -static bool core_relo_is_enumval_based(enum bpf_core_relo_kind kind)
> > +bool core_relo_is_enumval_based(enum bpf_core_relo_kind kind)
> >  {
> >         switch (kind) {
> >         case BPF_CORE_ENUMVAL_EXISTS:
> > diff --git a/tools/lib/bpf/relo_core.h b/tools/lib/bpf/relo_core.h
> > index a28bf3711ce2..e969dfb032f4 100644
> > --- a/tools/lib/bpf/relo_core.h
> > +++ b/tools/lib/bpf/relo_core.h
> > @@ -84,4 +84,8 @@ int bpf_core_patch_insn(const char *prog_name, struct bpf_insn *insn,
> >                         int insn_idx, const struct bpf_core_relo *relo,
> >                         int relo_idx, const struct bpf_core_relo_res *res);
> >
> > +bool core_relo_is_field_based(enum bpf_core_relo_kind kind);
> > +bool core_relo_is_type_based(enum bpf_core_relo_kind kind);
> > +bool core_relo_is_enumval_based(enum bpf_core_relo_kind kind);
> > +
> >  #endif
> > --
> > 2.25.1
> >

  reply	other threads:[~2022-01-12 14:27 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 [this message]
2021-12-17 18:56 ` [PATCH bpf-next v3 3/3] bpftool: Implement btfgen Mauricio Vásquez
2021-12-23  0:33   ` Andrii Nakryiko
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=CAHap4zsy39sGWvW8-aXt3kZweDxXK6gmaFjy28W4qCp1fvywhw@mail.gmail.com \
    --to=mauricio@kinvolk.io \
    --cc=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=netdev@vger.kernel.org \
    --cc=quentin@isovalent.com \
    --cc=rafaeldtinoco@gmail.com \
    --subject='Re: [PATCH bpf-next v3 2/3] libbpf: Implement changes needed for BTFGen in bpftool' \
    /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).