| Message ID | B8801F77-37E8-4EF8-8994-D366D48169A3@araalinetworks.com (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | BPF |
| Headers | show |
| Series | [bpf,v1] Add `core_btf_path` to `bpf_object_open_opts` to pass BTF path from skeleton program | expand |
| Context | Check | Description |
|---|---|---|
| netdev/apply | fail | Patch does not apply to bpf |
| netdev/tree_selection | success | Clearly marked for bpf |
On 1/9/21 3:36 AM, Vamsi Kodavanty wrote: [...] > 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. For submitting patches there is an official write-up here [0]. An example of a commit message can be found here [1]. Please make sure to add your own Signed-off-by before officially submitting the patch. If you are stuck somewhere please let us know so we can help. Cheers, Daniel [0] https://www.kernel.org/doc/html/latest/process/submitting-patches.html [1] https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git/patch/?id=e22d7f05e445165e58feddb4e40cc9c0f94453bc
Thank you so much!. Will follow protocol and get back here. Best Regards Vamsi. On Mon, Jan 11, 2021 at 6:14 AM Daniel Borkmann <daniel@iogearbox.net> wrote: > > On 1/9/21 3:36 AM, Vamsi Kodavanty wrote: > [...] > > 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. > > For submitting patches there is an official write-up here [0]. An example of a commit message > can be found here [1]. Please make sure to add your own Signed-off-by before officially submitting > the patch. If you are stuck somewhere please let us know so we can help. > > Cheers, > Daniel > > [0] https://www.kernel.org/doc/html/latest/process/submitting-patches.html > [1] https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git/patch/?id=e22d7f05e445165e58feddb4e40cc9c0f94453bc
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 >
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 > >
On Mon, Jan 11, 2021 at 7:33 PM Vamsi Kodavanty <vamsi@araalinetworks.com> 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@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 > > >
On Wed, Jan 13, 2021 at 2:17 PM Vamsi Kodavanty <vamsi@araalinetworks.com> wrote: > > On Mon, Jan 11, 2021 at 7:33 PM Vamsi Kodavanty > <vamsi@araalinetworks.com> 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. > There are two Linux upstream repositories to which BPF and libbpf patches are applied: bpf ([0]) and bpf-next ([1]). Fixes usually go into bpf, while all the new features go into bpf-next. They are periodically merged and thus converge. Then, specifically for libbpf, there is a Github mirror ([2]), which is synced by me periodically from bpf-next and bpf trees. This Github repo is what is considered to be "canonical" libbpf repo for the purposes of building libbpf packages and consuming libbpf in user applications. You shouldn't concern yourself with this one when submitting patches, because it's a derivative of upstream repositories. What is confusing to me, though, is that all three of them have code with btf_vmlinux_override, so I'm curious which "bpf" repository did you find that doesn't yet have btf_vmlinux_override? [0] https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf.git [1] https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git [2] https://github.com/libbpf/libbpf > 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@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(-) > > > > [...]
On Wed, Jan 13, 2021 at 7:21 PM Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Wed, Jan 13, 2021 at 2:17 PM Vamsi Kodavanty > <vamsi@araalinetworks.com> wrote: > > > > On Mon, Jan 11, 2021 at 7:33 PM Vamsi Kodavanty > > <vamsi@araalinetworks.com> 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. > > > > There are two Linux upstream repositories to which BPF and libbpf > patches are applied: bpf ([0]) and bpf-next ([1]). Fixes usually go > into bpf, while all the new features go into bpf-next. They are > periodically merged and thus converge. > > Then, specifically for libbpf, there is a Github mirror ([2]), which > is synced by me periodically from bpf-next and bpf trees. This Github > repo is what is considered to be "canonical" libbpf repo for the > purposes of building libbpf packages and consuming libbpf in user > applications. You shouldn't concern yourself with this one when > submitting patches, because it's a derivative of upstream > repositories. > > What is confusing to me, though, is that all three of them have code > with btf_vmlinux_override, so I'm curious which "bpf" repository did > you find that doesn't yet have btf_vmlinux_override? > > [0] https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf.git > [1] https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git > [2] https://github.com/libbpf/libbpf > Thank you again. I was looking at [1]. I cloned the repo today morning and noticed the absence. I just did a 'git pull' and it seems to have the `btf_vmlinux_override` now. So, I will use `bpf-next`. Thank you for the repo links. Also, my earlier diffs were incorrectly using the `libbpf` repo. Regards Vamsi. > > 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@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(-) > > > > > > > [...]
On Wed, Jan 13, 2021 at 7:52 PM Vamsi Kodavanty <vamsi@araalinetworks.com> wrote: > > On Wed, Jan 13, 2021 at 7:21 PM Andrii Nakryiko > <andrii.nakryiko@gmail.com> wrote: > > > > On Wed, Jan 13, 2021 at 2:17 PM Vamsi Kodavanty > > <vamsi@araalinetworks.com> wrote: > > > > > > On Mon, Jan 11, 2021 at 7:33 PM Vamsi Kodavanty > > > <vamsi@araalinetworks.com> 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. > > > > > > > There are two Linux upstream repositories to which BPF and libbpf > > patches are applied: bpf ([0]) and bpf-next ([1]). Fixes usually go > > into bpf, while all the new features go into bpf-next. They are > > periodically merged and thus converge. > > > > Then, specifically for libbpf, there is a Github mirror ([2]), which > > is synced by me periodically from bpf-next and bpf trees. This Github > > repo is what is considered to be "canonical" libbpf repo for the > > purposes of building libbpf packages and consuming libbpf in user > > applications. You shouldn't concern yourself with this one when > > submitting patches, because it's a derivative of upstream > > repositories. > > > > What is confusing to me, though, is that all three of them have code > > with btf_vmlinux_override, so I'm curious which "bpf" repository did > > you find that doesn't yet have btf_vmlinux_override? > > > > [0] https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf.git > > [1] https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git > > [2] https://github.com/libbpf/libbpf > > > > Thank you again. I was looking at [1]. I cloned the repo today morning and > noticed the absence. I just did a 'git pull' and it seems to have the > `btf_vmlinux_override` now. So, I will use `bpf-next`. Thank you for the > repo links. Also, my earlier diffs were incorrectly using the `libbpf` repo. > Hi Vamsi, Did you get a chance to finish this patch by any chance? It seems like the request to support the ability to provide a custom vmlinux BTF (usually to get a chance to use BPF CO-RE on older kernels) comes up quite often recently, so it would be good to have this landed. Please let me know if you can finish this up. It's ok if not, but please let us know. Thanks! > Regards > Vamsi. > > > > 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@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. + */ if (obj->btf_ext && obj->btf_ext->core_relo_info.len) - return true; + if (!obj->btf_vmlinux_override) + 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) +{ + /* Could have been be set via `bpf_object_open_opts` */ + if (obj->btf_vmlinux_override) + return 0; + + 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; - } - } - 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); + obj->btf_vmlinux_override = btf__parse(core_btf_path, NULL); + if (IS_ERR_OR_NULL(obj->btf_vmlinux_override)) + pr_warn("can't parse btf at '%s'\n", core_btf_path); + } + 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); + 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. */ + 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 *
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. Best Regards, Vamsi. --- src/libbpf.c | 56 +++++++++++++++++++++++++++++++++++++--------------- src/libbpf.h | 4 +++- 2 files changed, 43 insertions(+), 17 deletions(-)