All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH bpf v1] Add `core_btf_path` to `bpf_object_open_opts` to pass BTF path from skeleton program
@ 2021-01-09  2:36 Vamsi Kodavanty
  2021-01-11 14:13 ` Daniel Borkmann
  2021-01-11 22:02 ` Andrii Nakryiko
  0 siblings, 2 replies; 9+ messages in thread
From: Vamsi Kodavanty @ 2021-01-09  2:36 UTC (permalink / raw)
  To: bpf, andrii.nakryiko

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(-)

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 *
-- 
2.23.3


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH bpf v1] Add `core_btf_path` to `bpf_object_open_opts` to pass BTF path from skeleton program
  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
  1 sibling, 1 reply; 9+ messages in thread
From: Daniel Borkmann @ 2021-01-11 14:13 UTC (permalink / raw)
  To: Vamsi Kodavanty, bpf, andrii.nakryiko

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH bpf v1] Add `core_btf_path` to `bpf_object_open_opts` to pass BTF path from skeleton program
  2021-01-11 14:13 ` Daniel Borkmann
@ 2021-01-11 19:07   ` Vamsi Kodavanty
  0 siblings, 0 replies; 9+ messages in thread
From: Vamsi Kodavanty @ 2021-01-11 19:07 UTC (permalink / raw)
  To: Daniel Borkmann; +Cc: bpf, Andrii Nakryiko

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH bpf v1] Add `core_btf_path` to `bpf_object_open_opts` to pass BTF path from skeleton program
  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 22:02 ` Andrii Nakryiko
  2021-01-12  3:33   ` Vamsi Kodavanty
  1 sibling, 1 reply; 9+ messages in thread
From: Andrii Nakryiko @ 2021-01-11 22:02 UTC (permalink / raw)
  To: Vamsi Kodavanty; +Cc: bpf

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
>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH bpf v1] Add `core_btf_path` to `bpf_object_open_opts` to pass BTF path from skeleton program
  2021-01-11 22:02 ` Andrii Nakryiko
@ 2021-01-12  3:33   ` Vamsi Kodavanty
  2021-01-13 22:16     ` Vamsi Kodavanty
  0 siblings, 1 reply; 9+ messages in thread
From: Vamsi Kodavanty @ 2021-01-12  3:33 UTC (permalink / raw)
  To: Andrii Nakryiko; +Cc: bpf

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
> >

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH bpf v1] Add `core_btf_path` to `bpf_object_open_opts` to pass BTF path from skeleton program
  2021-01-12  3:33   ` Vamsi Kodavanty
@ 2021-01-13 22:16     ` Vamsi Kodavanty
  2021-01-14  3:21       ` Andrii Nakryiko
  0 siblings, 1 reply; 9+ messages in thread
From: Vamsi Kodavanty @ 2021-01-13 22:16 UTC (permalink / raw)
  To: Andrii Nakryiko; +Cc: bpf

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
> > >

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH bpf v1] Add `core_btf_path` to `bpf_object_open_opts` to pass BTF path from skeleton program
  2021-01-13 22:16     ` Vamsi Kodavanty
@ 2021-01-14  3:21       ` Andrii Nakryiko
  2021-01-14  3:52         ` Vamsi Kodavanty
  0 siblings, 1 reply; 9+ messages in thread
From: Andrii Nakryiko @ 2021-01-14  3:21 UTC (permalink / raw)
  To: Vamsi Kodavanty; +Cc: bpf

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(-)
> > > >

[...]

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH bpf v1] Add `core_btf_path` to `bpf_object_open_opts` to pass BTF path from skeleton program
  2021-01-14  3:21       ` Andrii Nakryiko
@ 2021-01-14  3:52         ` Vamsi Kodavanty
  2021-03-03  4:11           ` Andrii Nakryiko
  0 siblings, 1 reply; 9+ messages in thread
From: Vamsi Kodavanty @ 2021-01-14  3:52 UTC (permalink / raw)
  To: Andrii Nakryiko; +Cc: bpf

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(-)
> > > > >
>
> [...]

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH bpf v1] Add `core_btf_path` to `bpf_object_open_opts` to pass BTF path from skeleton program
  2021-01-14  3:52         ` Vamsi Kodavanty
@ 2021-03-03  4:11           ` Andrii Nakryiko
  0 siblings, 0 replies; 9+ messages in thread
From: Andrii Nakryiko @ 2021-03-03  4:11 UTC (permalink / raw)
  To: Vamsi Kodavanty; +Cc: bpf

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(-)
> > > > > >
> >
> > [...]

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2021-03-03 22:43 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
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
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

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.