All the mail mirrored from lore.kernel.org
 help / color / mirror / Atom feed
From: Vamsi Kodavanty <vamsi@araalinetworks.com>
To: Andrii Nakryiko <andrii.nakryiko@gmail.com>
Cc: bpf <bpf@vger.kernel.org>
Subject: Re: [PATCH bpf v1] Add `core_btf_path` to `bpf_object_open_opts` to pass BTF path from skeleton program
Date: Mon, 11 Jan 2021 19:33:52 -0800	[thread overview]
Message-ID: <CADmGQ+3_h22VmJPddhf4Vy2J4PwwkhJAj+N3qSV7vERb+PZw8Q@mail.gmail.com> (raw)
In-Reply-To: <CAEf4Bzbgw49w2PtowsrzKQNcxD4fZRE6AKByX-5-dMo-+oWHHA@mail.gmail.com>

Andrii,
   Thank you for the detailed review. I will address them as well as
the self tests. And will send out a new patch addressing them and
conforming to style/expectations.

Cheers
Vamsi.

On Mon, Jan 11, 2021 at 2:02 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Fri, Jan 8, 2021 at 6:36 PM Vamsi Kodavanty <vamsi@araalinetworks.com> wrote:
> >
> > Andrii,
> >      I have made the following changes as discussed to add an option to the `open_opts`
> > to take in the BTF.
> >      Please do take a look. Also, I am not sure what the procedure is for submitting patches/reviews.
> > If anyone has any pointers to a webpage where this is described I can go through it. But, below are
> > the proposed changes.
> >
>
> Daniel already gave you pointers. Also make sure you add [PATCH
> bpf-next] prefix to email subject to identify the patch is for
> bpf-next kernel tree.
> And with all changes like this we should also add selftests,
> exercising new features. Please take a look at
> tools/testing/selftests/bpf. I think updating
> test_progs/test_core_reloc.c in there to use this instead of
> bpf_object__load_xattr() might be enough of the testing.
>
> > Best Regards,
> > Vamsi.
> >
> > ---
> >  src/libbpf.c | 56 +++++++++++++++++++++++++++++++++++++---------------
> >  src/libbpf.h |  4 +++-
> >  2 files changed, 43 insertions(+), 17 deletions(-)
> >
> > diff --git a/src/libbpf.c b/src/libbpf.c
> > index 6ae748f..35d7254 100644
> > --- a/src/libbpf.c
> > +++ b/src/libbpf.c
> > @@ -2538,9 +2538,12 @@ static bool obj_needs_vmlinux_btf(const struct bpf_object *obj)
> >         struct bpf_program *prog;
> >         int i;
> >
> > -       /* CO-RE relocations need kernel BTF */
> > +       /* CO-RE relocations need kernel BTF or an override BTF.
> > +        * If override BTF present CO-RE can use it.
>
> nit: "CO-RE relocations need kernel BTF, unless custom BTF override is
> specified"
>
> > +        */
> >         if (obj->btf_ext && obj->btf_ext->core_relo_info.len)
> > -               return true;
> > +               if (!obj->btf_vmlinux_override)
>
> please combine this into a single if
>
> > +                       return true;
> >
> >         /* Support for typed ksyms needs kernel BTF */
> >         for (i = 0; i < obj->nr_extern; i++) {
> > @@ -2561,6 +2564,27 @@ static bool obj_needs_vmlinux_btf(const struct bpf_object *obj)
> >         return false;
> >  }
> >
> > +static int bpf_object__load_override_btf(struct bpf_object *obj,
> > +                                                                                const char *targ_btf_path)
>
> formatting is off? scripts/checkpatch.pl -f <path to file> will report
> issues like this
>
> > +{
> > +       /* Could have been be set via `bpf_object_open_opts` */
> > +       if (obj->btf_vmlinux_override)
> > +               return 0;
>
> see below, let's make sure we load btf_vmlinux_override in one place
> (and cleanup somewhere close)
>
> > +
> > +       if (!targ_btf_path)
> > +               return 0;
> > +
> > +       obj->btf_vmlinux_override = btf__parse(targ_btf_path, NULL);
> > +       if (IS_ERR_OR_NULL(obj->btf_vmlinux_override)) {
> > +               int err = PTR_ERR(obj->btf_vmlinux_override);
> > +               obj->btf_vmlinux_override = NULL;
> > +               pr_warn("failed to parse target BTF: %d\n", err);
> > +               return err;
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> >  static int bpf_object__load_vmlinux_btf(struct bpf_object *obj, bool force)
> >  {
> >         int err;
> > @@ -6031,7 +6055,7 @@ patch_insn:
> >  }
> >
> >  static int
> > -bpf_object__relocate_core(struct bpf_object *obj, const char *targ_btf_path)
> > +bpf_object__relocate_core(struct bpf_object *obj)
> >  {
> >         const struct btf_ext_info_sec *sec;
> >         const struct bpf_core_relo *rec;
> > @@ -6045,15 +6069,6 @@ bpf_object__relocate_core(struct bpf_object *obj, const char *targ_btf_path)
> >         if (obj->btf_ext->core_relo_info.len == 0)
> >                 return 0;
> >
> > -       if (targ_btf_path) {
> > -               obj->btf_vmlinux_override = btf__parse(targ_btf_path, NULL);
> > -               if (IS_ERR_OR_NULL(obj->btf_vmlinux_override)) {
> > -                       err = PTR_ERR(obj->btf_vmlinux_override);
> > -                       pr_warn("failed to parse target BTF: %d\n", err);
> > -                       return err;
> > -               }
> > -       }
> > -
>
> given we are moving out btf_vmlinux_override loading from
> bpf_object__relocate_core, we need to move out its destruction and
> cleanup to the same function that does BTF parsing. That will keep the
> logic simpler.
>
> >         cand_cache = hashmap__new(bpf_core_hash_fn, bpf_core_equal_fn, NULL);
> >         if (IS_ERR(cand_cache)) {
> >                 err = PTR_ERR(cand_cache);
> > @@ -6556,14 +6571,14 @@ bpf_object__relocate_calls(struct bpf_object *obj, struct bpf_program *prog)
> >  }
> >
> >  static int
> > -bpf_object__relocate(struct bpf_object *obj, const char *targ_btf_path)
> > +bpf_object__relocate(struct bpf_object *obj)
> >  {
> >         struct bpf_program *prog;
> >         size_t i;
> >         int err;
> >
> >         if (obj->btf_ext) {
> > -               err = bpf_object__relocate_core(obj, targ_btf_path);
> > +               err = bpf_object__relocate_core(obj);
> >                 if (err) {
> >                         pr_warn("failed to perform CO-RE relocations: %d\n",
> >                                 err);
> > @@ -7088,7 +7103,7 @@ static struct bpf_object *
> >  __bpf_object__open(const char *path, const void *obj_buf, size_t obj_buf_sz,
> >                    const struct bpf_object_open_opts *opts)
> >  {
> > -       const char *obj_name, *kconfig;
> > +       const char *obj_name, *kconfig, *core_btf_path;
> >         struct bpf_program *prog;
> >         struct bpf_object *obj;
> >         char tmp_name[64];
> > @@ -7126,6 +7141,14 @@ __bpf_object__open(const char *path, const void *obj_buf, size_t obj_buf_sz,
> >                         return ERR_PTR(-ENOMEM);
> >         }
> >
> > +       core_btf_path = OPTS_GET(opts, core_btf_path, NULL);
> > +       if (core_btf_path) {
> > +               pr_debug("parse btf '%s' for CO-RE relocations\n", core_btf_path);
>
> Move this right after btf__parse(), so you can report success or
> failure in one log statement; see how libbpf_find_kernel_btf() does
> it. Please use similar wording (just "target BTF" instead of "kernel
> BTF" to distinguish them).
>
> > +               obj->btf_vmlinux_override = btf__parse(core_btf_path, NULL);
>
> This BTF is not needed until the load phase, so let's not attempt to
> load it on open() unnecessarily. Just remember the path and defer till
> bpf_object__load() time.
>
> > +               if (IS_ERR_OR_NULL(obj->btf_vmlinux_override))
> > +                       pr_warn("can't parse btf at '%s'\n", core_btf_path);
>
> if BTF can't be loaded, load should fail
>
> > +       }
> > +
> >         err = bpf_object__elf_init(obj);
> >         err = err ? : bpf_object__check_endianness(obj);
> >         err = err ? : bpf_object__elf_collect(obj);
> > @@ -7481,13 +7504,14 @@ int bpf_object__load_xattr(struct bpf_object_load_attr *attr)
> >         }
> >
> >         err = bpf_object__probe_loading(obj);
> > +       err = err ? : bpf_object__load_override_btf(obj, attr->target_btf_path);
> >         err = err ? : bpf_object__load_vmlinux_btf(obj, false);
> >         err = err ? : bpf_object__resolve_externs(obj, obj->kconfig);
> >         err = err ? : bpf_object__sanitize_and_load_btf(obj);
> >         err = err ? : bpf_object__sanitize_maps(obj);
> >         err = err ? : bpf_object__init_kern_struct_ops_maps(obj);
> >         err = err ? : bpf_object__create_maps(obj);
> > -       err = err ? : bpf_object__relocate(obj, attr->target_btf_path);
>
> For backwards compatibility, we need to still respect
> attr->target_btf_path. I'd say let attr->target_btf_path override
> opts->core_btf_path, if specified. Later on we'll probably just
> deprecate bpf_object__load_xattr() and target_btf_path will go away.
>
> > +       err = err ? : bpf_object__relocate(obj);
> >         err = err ? : bpf_object__load_progs(obj, attr->log_level);
> >
> >         /* clean up module BTFs */
> > diff --git a/src/libbpf.h b/src/libbpf.h
> > index 3c35eb4..40c4ee9 100644
> > --- a/src/libbpf.h
> > +++ b/src/libbpf.h
> > @@ -93,8 +93,10 @@ struct bpf_object_open_opts {
> >          * system Kconfig for CONFIG_xxx externs.
> >          */
> >         const char *kconfig;
> > +       /* Path to ELF file with BTF section to be used for relocations. */
>
> Given you use btf__parse() when opening this BTF, it handles both ELF
> and raw BTF data. So let's reword this comment to mention BTF in
> general.
>
> > +       const char *core_btf_path;
> >  };
> > -#define bpf_object_open_opts__last_field kconfig
> > +#define bpf_object_open_opts__last_field core_btf_path
> >
> >  LIBBPF_API struct bpf_object *bpf_object__open(const char *path);
> >  LIBBPF_API struct bpf_object *
> > --
> > 2.23.3
> >

  reply	other threads:[~2021-01-12  3:34 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-01-09  2:36 [PATCH bpf v1] Add `core_btf_path` to `bpf_object_open_opts` to pass BTF path from skeleton program Vamsi Kodavanty
2021-01-11 14:13 ` Daniel Borkmann
2021-01-11 19:07   ` Vamsi Kodavanty
2021-01-11 22:02 ` Andrii Nakryiko
2021-01-12  3:33   ` Vamsi Kodavanty [this message]
2021-01-13 22:16     ` Vamsi Kodavanty
2021-01-14  3:21       ` Andrii Nakryiko
2021-01-14  3:52         ` Vamsi Kodavanty
2021-03-03  4:11           ` Andrii Nakryiko

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=CADmGQ+3_h22VmJPddhf4Vy2J4PwwkhJAj+N3qSV7vERb+PZw8Q@mail.gmail.com \
    --to=vamsi@araalinetworks.com \
    --cc=andrii.nakryiko@gmail.com \
    --cc=bpf@vger.kernel.org \
    /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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.