On Mon, Jan 11, 2021 at 7:33 PM Vamsi Kodavanty
<vamsi@xxxxxxxxxxxxxxxxxx> wrote:
>
> 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.
>
Andrii,
I understand the `bpf` repository being a mirror of the
`bpf-next` tools/lib/bpf. Do the patches
to `bpf` go back into `bpf-next`. I see there is a script for
`bpf-next` to `bpf`syncs.
I ask because the `btf_vmlinux_override` changes only exist in
the `bpf` repo. So, I make my
changes in `bpf`?. In that case what happens to the `selftests` which
are in `bpf-next`. And they
won't have any idea of the new open option 'core_btf_path` that is
being introduced.
Thanks again. Hopefully this is my last question before I come back to
you with a proper patch.
Cheers
Vamsi.
> On Mon, Jan 11, 2021 at 2:02 PM Andrii Nakryiko
> <andrii.nakryiko@xxxxxxxxx> wrote:
> >
> > On Fri, Jan 8, 2021 at 6:36 PM Vamsi Kodavanty <vamsi@xxxxxxxxxxxxxxxxxx> 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
> > >
