[PATCH bpf-next 0/4] Subskeleton support for BPF libraries

[PATCH bpf-next 0/4] Subskeleton support for BPF libraries

[Delyan Kratunov]

In the quest for ever more modularity, a new need has arisen - the ability to
access data associated with a BPF library from a corresponding userspace library.
The catch is that we don't want the userspace library to know about the structure of the
final BPF object that the BPF library is linked into.

In pursuit of this modularity, this patch series introduces *subskeletons.*
Subskeletons are similar in use and design to skeletons with a couple of differences:

1. The generated storage types do not rely on contiguous storage for the library's
variables because they may be interspersed randomly throughout the final BPF object's sections.

2. Subskeletons do not own objects and instead require a loaded bpf_object* to
be passed at runtime in order to be initialized. By extension, symbols are resolved at
runtime by parsing the final object's BTF. This has the interesting effect that the same
userspace code can interoperate with the library BPF code *linked into different final objects.*

3. Currently, only globals are supported though the codegen can be extended to support
non-owning pointers to maps, progs, links, etc.

Areas that are RFC/TODO:
* AFAICT, the ELF section names are the only way to find the correct maps in the final
linked object. As a result, I've added bpf_map__section_name so bpftool can use the section
names in the codegen. Do let me know if there's a better design I'm missing.

* The bpf_object__{open,destroy}_subskeleton approach mirrors the corresponding skeleton
support functionality. Do let me know if there's anything that needs to exist in it to ensure
forward compatibility. (Unfortunately, I don't see any way for subskeletons to work with older
libbpf versions, so I'd rather introduce all the new APIs they may need in a single version.)


Delyan Kratunov (4):
  libbpf: expose map elf section name
  bpftool: add support for subskeletons
  libbpf: add subskeleton scaffolding
  selftests/bpf: test subskeleton functionality

 tools/bpf/bpftool/gen.c                       | 322 +++++++++++++++++-
 tools/lib/bpf/libbpf.c                        |  84 +++++
 tools/lib/bpf/libbpf.h                        |  23 ++
 tools/lib/bpf/libbpf.map                      |   7 +
 tools/lib/bpf/libbpf_version.h                |   2 +-
 tools/testing/selftests/bpf/Makefile          |  18 +-
 .../selftests/bpf/prog_tests/subskeleton.c    |  38 +++
 .../bpf/prog_tests/subskeleton_lib.c          |  29 ++
 .../selftests/bpf/progs/test_subskeleton.c    |  20 ++
 .../bpf/progs/test_subskeleton_lib.c          |  22 ++
 10 files changed, 553 insertions(+), 12 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/subskeleton.c
 create mode 100644 tools/testing/selftests/bpf/prog_tests/subskeleton_lib.c
 create mode 100644 tools/testing/selftests/bpf/progs/test_subskeleton.c
 create mode 100644 tools/testing/selftests/bpf/progs/test_subskeleton_lib.c

--
2.34.1

[PATCH bpf-next 3/4] libbpf: add subskeleton scaffolding

[Delyan Kratunov]

In symmetry with bpf_object__open_skeleton(),
bpf_object__open_subskeleton() performs the actual walking and linking
of symbols described by bpf_sym_skeleton objects.

Signed-off-by: Delyan Kratunov <delyank@fb.com>
---
 tools/lib/bpf/libbpf.c   | 76 ++++++++++++++++++++++++++++++++++++++++
 tools/lib/bpf/libbpf.h   | 21 +++++++++++
 tools/lib/bpf/libbpf.map |  2 ++
 3 files changed, 99 insertions(+)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index d20ae8f225ee..e6c27f4b9dea 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -11748,6 +11748,82 @@ int bpf_object__open_skeleton(struct bpf_object_skeleton *s,
 	return 0;
 }
 
+int bpf_object__open_subskeleton(struct bpf_object_subskeleton *s)
+{
+	int i, len, map_type_id, sym_idx;
+	const char *var_name;
+	struct bpf_map *map;
+	struct btf *btf;
+	const struct btf_type *map_type, *var_type;
+	const struct bpf_sym_skeleton *sym;
+	struct btf_var_secinfo *var;
+	struct bpf_map *last_map = NULL;
+	const struct btf_type *last_map_type = NULL;
+
+	if (!s->obj)
+		return libbpf_err(-EINVAL);
+
+	btf = bpf_object__btf(s->obj);
+	if (!btf)
+		return libbpf_err(errno);
+
+	for (sym_idx = 0; sym_idx < s->sym_cnt; sym_idx++) {
+		sym = &s->syms[sym_idx];
+		if (last_map && (strcmp(sym->section, bpf_map__section_name(last_map)) == 0)) {
+			map = last_map;
+			map_type = last_map_type;
+		} else {
+			map = bpf_object__find_map_by_name(s->obj, sym->section);
+			if (!map) {
+				pr_warn("Could not find map for section %1$s, symbol %2$s",
+					sym->section, s->syms[i].name);
+				return libbpf_err(-EINVAL);
+			}
+			map_type_id = btf__find_by_name_kind(btf, sym->section, BTF_KIND_DATASEC);
+			if (map_type_id < 0) {
+				pr_warn("Could not find map type in btf for section %1$s (due to symbol %2$s)",
+					sym->section, sym->name);
+				return libbpf_err(-EINVAL);
+			}
+			map_type = btf__type_by_id(btf, map_type_id);
+		}
+
+		/* We have a section and a corresponding type, now find the
+		 * symbol in the loaded map. This is clearly quadratic in the
+		 * number of symbols in the section, but that's easy to optimize
+		 * once the need arises.
+		 */
+
+		len = btf_vlen(map_type);
+		for (i = 0, var = btf_var_secinfos(map_type); i < len; i++, var++) {
+			var_type = btf__type_by_id(btf, var->type);
+			if (!var_type) {
+				pr_warn("Could not find var type for item %1$d in section %2$s",
+					i, sym->section);
+				return libbpf_err(-EINVAL);
+			}
+			var_name = btf__name_by_offset(btf, var_type->name_off);
+			if (strcmp(var_name, sym->name) == 0) {
+				*sym->addr = (char *) map->mmaped + var->offset;
+				break;
+			}
+		}
+
+		last_map = map;
+		last_map_type = map_type;
+	}
+	return 0;
+}
+
+void bpf_object__destroy_subskeleton(struct bpf_object_subskeleton *s)
+{
+	if (!s)
+		return;
+	if (s->syms)
+		free(s->syms);
+	free(s);
+}
+
 int bpf_object__load_skeleton(struct bpf_object_skeleton *s)
 {
 	int i, err;
diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
index 7b66794f1c0a..915d59c31ad5 100644
--- a/tools/lib/bpf/libbpf.h
+++ b/tools/lib/bpf/libbpf.h
@@ -1291,6 +1291,27 @@ LIBBPF_API int bpf_object__attach_skeleton(struct bpf_object_skeleton *s);
 LIBBPF_API void bpf_object__detach_skeleton(struct bpf_object_skeleton *s);
 LIBBPF_API void bpf_object__destroy_skeleton(struct bpf_object_skeleton *s);
 
+struct bpf_sym_skeleton {
+	const char *name;
+	const char *section;
+	void **addr;
+};
+
+struct bpf_object_subskeleton {
+	size_t sz; /* size of this struct, for forward/backward compatibility */
+
+	const struct bpf_object *obj;
+
+	int sym_cnt;
+	int sym_skel_sz;
+	struct bpf_sym_skeleton *syms;
+};
+
+LIBBPF_API int
+bpf_object__open_subskeleton(struct bpf_object_subskeleton *s);
+LIBBPF_API void
+bpf_object__destroy_subskeleton(struct bpf_object_subskeleton *s);
+
 struct gen_loader_opts {
 	size_t sz; /* size of this struct, for forward/backward compatiblity */
 	const char *data;
diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
index 5c85d297d955..81a1d0259866 100644
--- a/tools/lib/bpf/libbpf.map
+++ b/tools/lib/bpf/libbpf.map
@@ -443,4 +443,6 @@ LIBBPF_0.7.0 {
 LIBBPF_0.8.0 {
 	global:
     bpf_map__section_name;
+    bpf_object__open_subskeleton;
+    bpf_object__destroy_subskeleton;
 } LIBBPF_0.7.0;
-- 
2.34.1

Re: [PATCH bpf-next 3/4] libbpf: add subskeleton scaffolding

[Daniel Borkmann]

Hi Delyan,

On 3/2/22 3:48 AM, Delyan Kratunov wrote:
> In symmetry with bpf_object__open_skeleton(),
> bpf_object__open_subskeleton() performs the actual walking and linking
> of symbols described by bpf_sym_skeleton objects.
> 
> Signed-off-by: Delyan Kratunov <delyank@fb.com>
> ---
>   tools/lib/bpf/libbpf.c   | 76 ++++++++++++++++++++++++++++++++++++++++
>   tools/lib/bpf/libbpf.h   | 21 +++++++++++
>   tools/lib/bpf/libbpf.map |  2 ++
>   3 files changed, 99 insertions(+)
> 

Triggers CI failure with:

 > build_kernel - Building kernel

   libbpf.c: In function ‘bpf_object__open_subskeleton’:
   libbpf.c:11779:27: error: ‘i’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
   11779 |      sym->section, s->syms[i].name);
         |                           ^
   cc1: all warnings being treated as errors
   make[5]: *** [/tmp/runner/work/bpf/bpf/tools/build/Makefile.build:96: /tmp/runner/work/bpf/bpf/tools/bpf/resolve_btfids/libbpf/staticobjs/libbpf.o] Error 1
   make[4]: *** [Makefile:157: /tmp/runner/work/bpf/bpf/tools/bpf/resolve_btfids/libbpf/staticobjs/libbpf-in.o] Error 2
   make[3]: *** [Makefile:55: /tmp/runner/work/bpf/bpf/tools/bpf/resolve_btfids//libbpf/libbpf.a] Error 2
   make[3]: *** Waiting for unfinished jobs....
   make[2]: *** [Makefile:72: bpf/resolve_btfids] Error 2
   make[1]: *** [Makefile:1334: tools/bpf/resolve_btfids] Error 2
   make[1]: *** Waiting for unfinished jobs....
   make: *** [Makefile:350: __build_one_by_one] Error 2
   Error: Process completed with exit code 2.

Thanks,
Daniel

Re: [PATCH bpf-next 3/4] libbpf: add subskeleton scaffolding

[Delyan Kratunov]

On Wed, 2022-03-02 at 22:43 +0100, Daniel Borkmann wrote:
> 
> Triggers CI failure with:
> 
>  > build_kernel - Building kernel
> 
>    libbpf.c: In function ‘bpf_object__open_subskeleton’:
>    libbpf.c:11779:27: error: ‘i’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
>    11779 |      sym->section, s->syms[i].name);
>          |                           ^
>    cc1: all warnings being treated as errors
>    make[5]: *** [/tmp/runner/work/bpf/bpf/tools/build/Makefile.build:96: /tmp/runner/work/bpf/bpf/tools/bpf/resolve_btfids/libbpf/staticobjs/libbpf.o] Error 1
>    make[4]: *** [Makefile:157: /tmp/runner/work/bpf/bpf/tools/bpf/resolve_btfids/libbpf/staticobjs/libbpf-in.o] Error 2
>    make[3]: *** [Makefile:55: /tmp/runner/work/bpf/bpf/tools/bpf/resolve_btfids//libbpf/libbpf.a] Error 2
>    make[3]: *** Waiting for unfinished jobs....
>    make[2]: *** [Makefile:72: bpf/resolve_btfids] Error 2
>    make[1]: *** [Makefile:1334: tools/bpf/resolve_btfids] Error 2
>    make[1]: *** Waiting for unfinished jobs....
>    make: *** [Makefile:350: __build_one_by_one] Error 2
>    Error: Process completed with exit code 2.
> 
> Thanks,
> Daniel

Argh, sorry about that, sending reroll in a few.

Re: [PATCH bpf-next 3/4] libbpf: add subskeleton scaffolding

[Andrii Nakryiko]

On Wed, Mar 2, 2022 at 4:20 PM Delyan Kratunov <delyank@fb.com> wrote:
>
> On Wed, 2022-03-02 at 22:43 +0100, Daniel Borkmann wrote:
> >
> > Triggers CI failure with:
> >
> >  > build_kernel - Building kernel
> >
> >    libbpf.c: In function ‘bpf_object__open_subskeleton’:
> >    libbpf.c:11779:27: error: ‘i’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
> >    11779 |      sym->section, s->syms[i].name);
> >          |                           ^
> >    cc1: all warnings being treated as errors
> >    make[5]: *** [/tmp/runner/work/bpf/bpf/tools/build/Makefile.build:96: /tmp/runner/work/bpf/bpf/tools/bpf/resolve_btfids/libbpf/staticobjs/libbpf.o] Error 1
> >    make[4]: *** [Makefile:157: /tmp/runner/work/bpf/bpf/tools/bpf/resolve_btfids/libbpf/staticobjs/libbpf-in.o] Error 2
> >    make[3]: *** [Makefile:55: /tmp/runner/work/bpf/bpf/tools/bpf/resolve_btfids//libbpf/libbpf.a] Error 2
> >    make[3]: *** Waiting for unfinished jobs....
> >    make[2]: *** [Makefile:72: bpf/resolve_btfids] Error 2
> >    make[1]: *** [Makefile:1334: tools/bpf/resolve_btfids] Error 2
> >    make[1]: *** Waiting for unfinished jobs....
> >    make: *** [Makefile:350: __build_one_by_one] Error 2
> >    Error: Process completed with exit code 2.
> >
> > Thanks,
> > Daniel
>
> Argh, sorry about that, sending reroll in a few.

Please hold off until you get review feedback

Re: [PATCH bpf-next 3/4] libbpf: add subskeleton scaffolding

[Delyan Kratunov]

On Wed, 2022-03-02 at 16:28 -0800, Andrii Nakryiko wrote:
> Please hold off until you get review feedback

Happy to wait, I discovered some functional issues in the test once rodata and
bpf helpers are involved in the lib. I have some debugging to do anyway.

Re: [PATCH bpf-next 3/4] libbpf: add subskeleton scaffolding

[Andrii Nakryiko]

On Tue, Mar 1, 2022 at 6:49 PM Delyan Kratunov <delyank@fb.com> wrote:
>
> In symmetry with bpf_object__open_skeleton(),
> bpf_object__open_subskeleton() performs the actual walking and linking
> of symbols described by bpf_sym_skeleton objects.
>
> Signed-off-by: Delyan Kratunov <delyank@fb.com>
> ---
>  tools/lib/bpf/libbpf.c   | 76 ++++++++++++++++++++++++++++++++++++++++
>  tools/lib/bpf/libbpf.h   | 21 +++++++++++
>  tools/lib/bpf/libbpf.map |  2 ++
>  3 files changed, 99 insertions(+)
>
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index d20ae8f225ee..e6c27f4b9dea 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -11748,6 +11748,82 @@ int bpf_object__open_skeleton(struct bpf_object_skeleton *s,
>         return 0;
>  }
>
> +int bpf_object__open_subskeleton(struct bpf_object_subskeleton *s)
> +{
> +       int i, len, map_type_id, sym_idx;
> +       const char *var_name;
> +       struct bpf_map *map;
> +       struct btf *btf;
> +       const struct btf_type *map_type, *var_type;
> +       const struct bpf_sym_skeleton *sym;
> +       struct btf_var_secinfo *var;
> +       struct bpf_map *last_map = NULL;
> +       const struct btf_type *last_map_type = NULL;
> +
> +       if (!s->obj)
> +               return libbpf_err(-EINVAL);
> +
> +       btf = bpf_object__btf(s->obj);
> +       if (!btf)
> +               return libbpf_err(errno);

-errno

> +
> +       for (sym_idx = 0; sym_idx < s->sym_cnt; sym_idx++) {
> +               sym = &s->syms[sym_idx];
> +               if (last_map && (strcmp(sym->section, bpf_map__section_name(last_map)) == 0)) {

see above about having struct bpf_map ** instead of sym->section

> +                       map = last_map;
> +                       map_type = last_map_type;
> +               } else {
> +                       map = bpf_object__find_map_by_name(s->obj, sym->section);
> +                       if (!map) {
> +                               pr_warn("Could not find map for section %1$s, symbol %2$s",
> +                                       sym->section, s->syms[i].name);
> +                               return libbpf_err(-EINVAL);
> +                       }
> +                       map_type_id = btf__find_by_name_kind(btf, sym->section, BTF_KIND_DATASEC);

bpf_map__btf_value_type_id() ?

> +                       if (map_type_id < 0) {
> +                               pr_warn("Could not find map type in btf for section %1$s (due to symbol %2$s)",
> +                                       sym->section, sym->name);
> +                               return libbpf_err(-EINVAL);
> +                       }
> +                       map_type = btf__type_by_id(btf, map_type_id);
> +               }
> +

[...]

> +
> +void bpf_object__destroy_subskeleton(struct bpf_object_subskeleton *s)
> +{
> +       if (!s)
> +               return;
> +       if (s->syms)
> +               free(s->syms);

no need to check s->syms, free handles NULL just fine

> +       free(s);
> +}
> +
>  int bpf_object__load_skeleton(struct bpf_object_skeleton *s)
>  {
>         int i, err;
> diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> index 7b66794f1c0a..915d59c31ad5 100644
> --- a/tools/lib/bpf/libbpf.h
> +++ b/tools/lib/bpf/libbpf.h
> @@ -1291,6 +1291,27 @@ LIBBPF_API int bpf_object__attach_skeleton(struct bpf_object_skeleton *s);
>  LIBBPF_API void bpf_object__detach_skeleton(struct bpf_object_skeleton *s);
>  LIBBPF_API void bpf_object__destroy_skeleton(struct bpf_object_skeleton *s);
>
> +struct bpf_sym_skeleton {

I tried to get used to this "sym" terminology for a bit, but it still
feels off. From user's perspective all this are variables. Any
objections to use "var" terminology?

> +       const char *name;
> +       const char *section;

what if we store a pointer to struct bpf_map * instead, that way we
won't need to search, we'll just have a pointer ready

> +       void **addr;
> +};
> +
> +struct bpf_object_subskeleton {
> +       size_t sz; /* size of this struct, for forward/backward compatibility */
> +
> +       const struct bpf_object *obj;
> +
> +       int sym_cnt;
> +       int sym_skel_sz;
> +       struct bpf_sym_skeleton *syms;

as mentioned in previous patch, let's also record maps and prog, it is
important and needed from the very beginning

> +};
> +
> +LIBBPF_API int
> +bpf_object__open_subskeleton(struct bpf_object_subskeleton *s);
> +LIBBPF_API void
> +bpf_object__destroy_subskeleton(struct bpf_object_subskeleton *s);
> +
>  struct gen_loader_opts {
>         size_t sz; /* size of this struct, for forward/backward compatiblity */
>         const char *data;
> diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
> index 5c85d297d955..81a1d0259866 100644
> --- a/tools/lib/bpf/libbpf.map
> +++ b/tools/lib/bpf/libbpf.map
> @@ -443,4 +443,6 @@ LIBBPF_0.7.0 {
>  LIBBPF_0.8.0 {
>         global:
>      bpf_map__section_name;
> +    bpf_object__open_subskeleton;
> +    bpf_object__destroy_subskeleton;

indentation looks off here... global should be indented with one tab,
and then APIs with two tabs

>  } LIBBPF_0.7.0;
> --
> 2.34.1

Re: [PATCH bpf-next 3/4] libbpf: add subskeleton scaffolding

[Andrii Nakryiko]

On Wed, Mar 2, 2022 at 8:33 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Tue, Mar 1, 2022 at 6:49 PM Delyan Kratunov <delyank@fb.com> wrote:
> >
> > In symmetry with bpf_object__open_skeleton(),
> > bpf_object__open_subskeleton() performs the actual walking and linking
> > of symbols described by bpf_sym_skeleton objects.
> >
> > Signed-off-by: Delyan Kratunov <delyank@fb.com>
> > ---
> >  tools/lib/bpf/libbpf.c   | 76 ++++++++++++++++++++++++++++++++++++++++
> >  tools/lib/bpf/libbpf.h   | 21 +++++++++++
> >  tools/lib/bpf/libbpf.map |  2 ++
> >  3 files changed, 99 insertions(+)
> >

forgot to mention, this patch logically probably should go before
bpftool changes: 1) define types and APIs in libbpf, and only then 2)
"use" those in bpftool

> > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > index d20ae8f225ee..e6c27f4b9dea 100644
> > --- a/tools/lib/bpf/libbpf.c
> > +++ b/tools/lib/bpf/libbpf.c
> > @@ -11748,6 +11748,82 @@ int bpf_object__open_skeleton(struct bpf_object_skeleton *s,
> >         return 0;
> >  }
> >
> > +int bpf_object__open_subskeleton(struct bpf_object_subskeleton *s)
> > +{
> > +       int i, len, map_type_id, sym_idx;
> > +       const char *var_name;
> > +       struct bpf_map *map;
> > +       struct btf *btf;
> > +       const struct btf_type *map_type, *var_type;
> > +       const struct bpf_sym_skeleton *sym;
> > +       struct btf_var_secinfo *var;
> > +       struct bpf_map *last_map = NULL;
> > +       const struct btf_type *last_map_type = NULL;
> > +
> > +       if (!s->obj)
> > +               return libbpf_err(-EINVAL);
> > +
> > +       btf = bpf_object__btf(s->obj);
> > +       if (!btf)
> > +               return libbpf_err(errno);
>
> -errno
>
> > +
> > +       for (sym_idx = 0; sym_idx < s->sym_cnt; sym_idx++) {
> > +               sym = &s->syms[sym_idx];
> > +               if (last_map && (strcmp(sym->section, bpf_map__section_name(last_map)) == 0)) {
>
> see above about having struct bpf_map ** instead of sym->section
>
> > +                       map = last_map;
> > +                       map_type = last_map_type;
> > +               } else {
> > +                       map = bpf_object__find_map_by_name(s->obj, sym->section);
> > +                       if (!map) {
> > +                               pr_warn("Could not find map for section %1$s, symbol %2$s",
> > +                                       sym->section, s->syms[i].name);
> > +                               return libbpf_err(-EINVAL);
> > +                       }
> > +                       map_type_id = btf__find_by_name_kind(btf, sym->section, BTF_KIND_DATASEC);
>
> bpf_map__btf_value_type_id() ?
>
> > +                       if (map_type_id < 0) {
> > +                               pr_warn("Could not find map type in btf for section %1$s (due to symbol %2$s)",
> > +                                       sym->section, sym->name);
> > +                               return libbpf_err(-EINVAL);
> > +                       }
> > +                       map_type = btf__type_by_id(btf, map_type_id);
> > +               }
> > +
>
> [...]
>
> > +
> > +void bpf_object__destroy_subskeleton(struct bpf_object_subskeleton *s)
> > +{
> > +       if (!s)
> > +               return;
> > +       if (s->syms)
> > +               free(s->syms);
>
> no need to check s->syms, free handles NULL just fine
>
> > +       free(s);
> > +}
> > +
> >  int bpf_object__load_skeleton(struct bpf_object_skeleton *s)
> >  {
> >         int i, err;
> > diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> > index 7b66794f1c0a..915d59c31ad5 100644
> > --- a/tools/lib/bpf/libbpf.h
> > +++ b/tools/lib/bpf/libbpf.h
> > @@ -1291,6 +1291,27 @@ LIBBPF_API int bpf_object__attach_skeleton(struct bpf_object_skeleton *s);
> >  LIBBPF_API void bpf_object__detach_skeleton(struct bpf_object_skeleton *s);
> >  LIBBPF_API void bpf_object__destroy_skeleton(struct bpf_object_skeleton *s);
> >
> > +struct bpf_sym_skeleton {
>
> I tried to get used to this "sym" terminology for a bit, but it still
> feels off. From user's perspective all this are variables. Any
> objections to use "var" terminology?
>
> > +       const char *name;
> > +       const char *section;
>
> what if we store a pointer to struct bpf_map * instead, that way we
> won't need to search, we'll just have a pointer ready
>
> > +       void **addr;
> > +};
> > +
> > +struct bpf_object_subskeleton {
> > +       size_t sz; /* size of this struct, for forward/backward compatibility */
> > +
> > +       const struct bpf_object *obj;
> > +
> > +       int sym_cnt;
> > +       int sym_skel_sz;
> > +       struct bpf_sym_skeleton *syms;
>
> as mentioned in previous patch, let's also record maps and prog, it is
> important and needed from the very beginning
>
> > +};
> > +
> > +LIBBPF_API int
> > +bpf_object__open_subskeleton(struct bpf_object_subskeleton *s);
> > +LIBBPF_API void
> > +bpf_object__destroy_subskeleton(struct bpf_object_subskeleton *s);
> > +
> >  struct gen_loader_opts {
> >         size_t sz; /* size of this struct, for forward/backward compatiblity */
> >         const char *data;
> > diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
> > index 5c85d297d955..81a1d0259866 100644
> > --- a/tools/lib/bpf/libbpf.map
> > +++ b/tools/lib/bpf/libbpf.map
> > @@ -443,4 +443,6 @@ LIBBPF_0.7.0 {
> >  LIBBPF_0.8.0 {
> >         global:
> >      bpf_map__section_name;
> > +    bpf_object__open_subskeleton;
> > +    bpf_object__destroy_subskeleton;
>
> indentation looks off here... global should be indented with one tab,
> and then APIs with two tabs
>
> >  } LIBBPF_0.7.0;
> > --
> > 2.34.1

Re: [PATCH bpf-next 3/4] libbpf: add subskeleton scaffolding

[Delyan Kratunov]

On Wed, 2022-03-02 at 20:34 -0800, Andrii Nakryiko wrote:
> > 
> 
> forgot to mention, this patch logically probably should go before
> bpftool changes: 1) define types and APIs in libbpf, and only then 2)
> "use" those in bpftool

Sure.

> > > 
> > > +struct bpf_sym_skeleton {
> > 
> > I tried to get used to this "sym" terminology for a bit, but it still
> > feels off. From user's perspective all this are variables. Any
> > objections to use "var" terminology?

"var" has a specific meaning in btf and I didn't want to make bpf_var_skeleton
look related to btf_var for example. Given the extern usage that libs require, I
figured "sym" would make sense to the user. 

If you don't think the confusion with btf_var is significant, I can rename it -
this is all used by generated code anyway.

> > 
> > > +       const char *name;
> > > +       const char *section;
> > 
> > what if we store a pointer to struct bpf_map * instead, that way we
> > won't need to search, we'll just have a pointer ready

We'd have to search *somewhere*. I'd rather have the search inside libbpf than
inside the generated code. Besides, finding the right bpf_map from within the
subskeleton is pretty annoying - you'll have to do suffix searches on the
bpf_map names in the passed-in bpf_object and codegening all that is unnecessary
when libbpf can look at real_name.

> 

Thanks,
Delyan

Re: [PATCH bpf-next 3/4] libbpf: add subskeleton scaffolding

[Andrii Nakryiko]

On Thu, Mar 3, 2022 at 11:09 AM Delyan Kratunov <delyank@fb.com> wrote:
>
> On Wed, 2022-03-02 at 20:34 -0800, Andrii Nakryiko wrote:
> > >
> >
> > forgot to mention, this patch logically probably should go before
> > bpftool changes: 1) define types and APIs in libbpf, and only then 2)
> > "use" those in bpftool
>
> Sure.
>
> > > >
> > > > +struct bpf_sym_skeleton {
> > >
> > > I tried to get used to this "sym" terminology for a bit, but it still
> > > feels off. From user's perspective all this are variables. Any
> > > objections to use "var" terminology?
>
> "var" has a specific meaning in btf and I didn't want to make bpf_var_skeleton
> look related to btf_var for example. Given the extern usage that libs require, I
> figured "sym" would make sense to the user.
>

Even for extern cases, we only generate stuff that really is a
variable. That is, it has allocated memory and there is specific
value. Only .kconfig is like that. .ksyms, for example, doesn't get
any exposure in skeleton as it can't be used from user-space code.

For me, symbol is just way too generic (could be basically anything,
including ELF section symbol, function, etc, etc). But our use case is
always variables available to both user-space and BPF code. I don't
think btf_var vs btf_var_skeleton confusion is significant (and even
then, each extern in .kconfig has corresponding BTF_KIND_VAR, so it
all is in sync).

> If you don't think the confusion with btf_var is significant, I can rename it -
> this is all used by generated code anyway.
>
> > >
> > > > +       const char *name;
> > > > +       const char *section;
> > >
> > > what if we store a pointer to struct bpf_map * instead, that way we
> > > won't need to search, we'll just have a pointer ready
>
> We'd have to search *somewhere*. I'd rather have the search inside libbpf than
> inside the generated code. Besides, finding the right bpf_map from within the
> subskeleton is pretty annoying - you'll have to do suffix searches on the
> bpf_map names in the passed-in bpf_object and codegening all that is unnecessary
> when libbpf can look at real_name.

I think you misunderstood what I proposed. There is no explicit
searching. Here is a simple example of sub-skeleton struct and how
code-generated code will fill it out


struct my_subskel {
    struct {
        struct bpf_map *my_map;
    } maps;
    struct my_subskel__data {
        int *my_var;
    } data;
};


/* in codegen'ed code */

struct my_subskel *s;

subskel->syms[0].name = "my_var";
subskel->syms[0].map = &s->maps.data_syn;


It's similar in principle to how we define maps (that are found and
filled out by libbpf):

        s->maps[4].name = ".data.dyn";
        s->maps[4].map = &obj->maps.data_dyn;
        s->maps[4].mmaped = (void **)&obj->data_dyn;


Except in this case we use &s->maps.data_syn for reading, not for
writing into it.

Hope this is clearer now.


>
> >
>
> Thanks,
> Delyan

[PATCH bpf-next 4/4] selftests/bpf: test subskeleton functionality

[Delyan Kratunov]

The new Makefile support via SUBSKELS and .skel.h-deps is a mixture
of LINKED_SKELS and LSKELS. By definition subskeletons require multiple
BPF object files to be linked together. However, generating the
subskeleton only requires the library object file and not the final
program object file.

Signed-off-by: Delyan Kratunov <delyank@fb.com>
---
 tools/testing/selftests/bpf/Makefile          | 18 ++++++++-
 .../selftests/bpf/prog_tests/subskeleton.c    | 38 +++++++++++++++++++
 .../bpf/prog_tests/subskeleton_lib.c          | 29 ++++++++++++++
 .../selftests/bpf/progs/test_subskeleton.c    | 20 ++++++++++
 .../bpf/progs/test_subskeleton_lib.c          | 22 +++++++++++
 5 files changed, 125 insertions(+), 2 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/subskeleton.c
 create mode 100644 tools/testing/selftests/bpf/prog_tests/subskeleton_lib.c
 create mode 100644 tools/testing/selftests/bpf/progs/test_subskeleton.c
 create mode 100644 tools/testing/selftests/bpf/progs/test_subskeleton_lib.c

diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index fe12b4f5fe20..57da63ba790b 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -326,19 +326,23 @@ endef
 SKEL_BLACKLIST := btf__% test_pinning_invalid.c test_sk_assign.c
 
 LINKED_SKELS := test_static_linked.skel.h linked_funcs.skel.h		\
-		linked_vars.skel.h linked_maps.skel.h
+		linked_vars.skel.h linked_maps.skel.h test_subskeleton.skel.h
+
+SUBSKELS := test_subskeleton_lib.skel.h
 
 LSKELS := kfunc_call_test.c fentry_test.c fexit_test.c fexit_sleep.c \
 	test_ringbuf.c atomics.c trace_printk.c trace_vprintk.c \
 	map_ptr_kern.c core_kern.c core_kern_overflow.c
 # Generate both light skeleton and libbpf skeleton for these
 LSKELS_EXTRA := test_ksyms_module.c test_ksyms_weak.c kfunc_call_test_subprog.c
-SKEL_BLACKLIST += $$(LSKELS)
+SKEL_BLACKLIST += $$(LSKELS) $$(SUBSKELS)
 
 test_static_linked.skel.h-deps := test_static_linked1.o test_static_linked2.o
 linked_funcs.skel.h-deps := linked_funcs1.o linked_funcs2.o
 linked_vars.skel.h-deps := linked_vars1.o linked_vars2.o
 linked_maps.skel.h-deps := linked_maps1.o linked_maps2.o
+test_subskeleton.skel.h-deps := test_subskeleton_lib.o test_subskeleton.o
+test_subskeleton_lib.skel.h-deps := test_subskeleton_lib.o
 
 LINKED_BPF_SRCS := $(patsubst %.o,%.c,$(foreach skel,$(LINKED_SKELS),$($(skel)-deps)))
 
@@ -363,6 +367,7 @@ TRUNNER_BPF_SKELS := $$(patsubst %.c,$$(TRUNNER_OUTPUT)/%.skel.h,	\
 				 $$(filter-out $(SKEL_BLACKLIST) $(LINKED_BPF_SRCS),\
 					       $$(TRUNNER_BPF_SRCS)))
 TRUNNER_BPF_LSKELS := $$(patsubst %.c,$$(TRUNNER_OUTPUT)/%.lskel.h, $$(LSKELS) $$(LSKELS_EXTRA))
+TRUNNER_BPF_SUBSKELS := $$(addprefix $$(TRUNNER_OUTPUT)/,$(SUBSKELS))
 TRUNNER_BPF_SKELS_LINKED := $$(addprefix $$(TRUNNER_OUTPUT)/,$(LINKED_SKELS))
 TEST_GEN_FILES += $$(TRUNNER_BPF_OBJS)
 
@@ -405,6 +410,14 @@ $(TRUNNER_BPF_SKELS): %.skel.h: %.o $(BPFTOOL) | $(TRUNNER_OUTPUT)
 	$(Q)diff $$(<:.o=.linked2.o) $$(<:.o=.linked3.o)
 	$(Q)$$(BPFTOOL) gen skeleton $$(<:.o=.linked3.o) name $$(notdir $$(<:.o=)) > $$@
 
+$(TRUNNER_BPF_SUBSKELS): %.skel.h: %.o $(BPFTOOL) | $(TRUNNER_OUTPUT)
+	$$(call msg,GEN-SUBSKEL,$(TRUNNER_BINARY),$$@)
+	$(Q)$$(BPFTOOL) gen object $$(<:.o=.linked1.o) $$<
+	$(Q)$$(BPFTOOL) gen object $$(<:.o=.linked2.o) $$(<:.o=.linked1.o)
+	$(Q)$$(BPFTOOL) gen object $$(<:.o=.linked3.o) $$(<:.o=.linked2.o)
+	$(Q)diff $$(<:.o=.linked2.o) $$(<:.o=.linked3.o)
+	$(Q)$$(BPFTOOL) gen subskeleton $$(<:.o=.linked3.o) name $$(notdir $$(<:.o=)) > $$@
+
 $(TRUNNER_BPF_LSKELS): %.lskel.h: %.o $(BPFTOOL) | $(TRUNNER_OUTPUT)
 	$$(call msg,GEN-SKEL,$(TRUNNER_BINARY),$$@)
 	$(Q)$$(BPFTOOL) gen object $$(<:.o=.linked1.o) $$<
@@ -441,6 +454,7 @@ $(TRUNNER_TEST_OBJS): $(TRUNNER_OUTPUT)/%.test.o:			\
 		      $(TRUNNER_EXTRA_HDRS)				\
 		      $(TRUNNER_BPF_OBJS)				\
 		      $(TRUNNER_BPF_SKELS)				\
+		      $(TRUNNER_BPF_SUBSKELS)				\
 		      $(TRUNNER_BPF_LSKELS)				\
 		      $(TRUNNER_BPF_SKELS_LINKED)			\
 		      $$(BPFOBJ) | $(TRUNNER_OUTPUT)
diff --git a/tools/testing/selftests/bpf/prog_tests/subskeleton.c b/tools/testing/selftests/bpf/prog_tests/subskeleton.c
new file mode 100644
index 000000000000..651aafc28e7f
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/subskeleton.c
@@ -0,0 +1,38 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2019 Facebook */
+
+#include <test_progs.h>
+#include "test_subskeleton.skel.h"
+
+extern void subskeleton_lib_setup(struct bpf_object *obj);
+extern int subskeleton_lib_subresult(struct bpf_object *obj);
+
+void test_subskeleton(void)
+{
+	int duration = 0, err, result;
+	struct test_subskeleton *skel;
+
+	skel = test_subskeleton__open();
+	if (CHECK(!skel, "skel_open", "failed to open skeleton\n"))
+		return;
+
+	skel->rodata->rovar1 = 10;
+
+	err = test_subskeleton__load(skel);
+	if (CHECK(err, "skel_load", "failed to load skeleton: %d\n", err))
+		goto cleanup;
+
+	subskeleton_lib_setup(skel->obj);
+
+	err = test_subskeleton__attach(skel);
+	if (CHECK(err, "skel_attach", "skeleton attach failed: %d\n", err))
+		goto cleanup;
+
+	/* trigger tracepoint */
+	usleep(1);
+
+	result = subskeleton_lib_subresult(skel->obj) * 10;
+	ASSERT_EQ(skel->bss->out1, result, "unexpected calculation");
+cleanup:
+	test_subskeleton__destroy(skel);
+}
diff --git a/tools/testing/selftests/bpf/prog_tests/subskeleton_lib.c b/tools/testing/selftests/bpf/prog_tests/subskeleton_lib.c
new file mode 100644
index 000000000000..f7f98b3febaf
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/subskeleton_lib.c
@@ -0,0 +1,29 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2019 Facebook */
+
+#include <test_progs.h>
+#include <bpf/libbpf.h>
+
+#include "test_subskeleton_lib.skel.h"
+
+void subskeleton_lib_setup(struct bpf_object *obj)
+{
+	struct test_subskeleton_lib *lib = test_subskeleton_lib__open(obj);
+
+	ASSERT_OK_PTR(lib, "open subskeleton");
+
+	*lib->data.var1 = 1;
+	*lib->bss.var2 = 2;
+	lib->bss.var3->var3_1 = 3;
+	lib->bss.var3->var3_2 = 4;
+}
+
+int subskeleton_lib_subresult(struct bpf_object *obj)
+{
+	struct test_subskeleton_lib *lib = test_subskeleton_lib__open(obj);
+
+	ASSERT_OK_PTR(lib, "open subskeleton");
+
+	ASSERT_EQ(*lib->bss.libout1, 1 + 2 + 3 + 4, "lib subresult");
+	return *lib->bss.libout1;
+}
diff --git a/tools/testing/selftests/bpf/progs/test_subskeleton.c b/tools/testing/selftests/bpf/progs/test_subskeleton.c
new file mode 100644
index 000000000000..bad3970718cb
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/test_subskeleton.c
@@ -0,0 +1,20 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2021 Facebook */
+
+#include <linux/bpf.h>
+#include <bpf/bpf_helpers.h>
+
+const int rovar1;
+int out1;
+
+extern int lib_routine(void);
+
+SEC("raw_tp/sys_enter")
+int handler1(const void *ctx)
+{
+	out1 = lib_routine() * rovar1;
+	return 0;
+}
+
+char LICENSE[] SEC("license") = "GPL";
+int VERSION SEC("version") = 1;
diff --git a/tools/testing/selftests/bpf/progs/test_subskeleton_lib.c b/tools/testing/selftests/bpf/progs/test_subskeleton_lib.c
new file mode 100644
index 000000000000..23c7f24997a7
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/test_subskeleton_lib.c
@@ -0,0 +1,22 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2021 Facebook */
+
+#include <linux/bpf.h>
+#include <bpf/bpf_helpers.h>
+
+int var1 = -1;
+int var2;
+struct {
+	int var3_1;
+	__s64 var3_2;
+} var3;
+int libout1;
+
+int lib_routine(void)
+{
+	libout1 =  var1 + var2 + var3.var3_1 + var3.var3_2;
+	return libout1;
+}
+
+char LICENSE[] SEC("license") = "GPL";
+int VERSION SEC("version") = 1;
-- 
2.34.1

Re: [PATCH bpf-next 4/4] selftests/bpf: test subskeleton functionality

[Alexei Starovoitov]

On Wed, Mar 02, 2022 at 02:48:48AM +0000, Delyan Kratunov wrote:
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/subskeleton.c
> @@ -0,0 +1,38 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (c) 2019 Facebook */
> +
> +#include <test_progs.h>
> +#include "test_subskeleton.skel.h"
> +
> +extern void subskeleton_lib_setup(struct bpf_object *obj);
> +extern int subskeleton_lib_subresult(struct bpf_object *obj);
> +
> +void test_subskeleton(void)
> +{
> +	int duration = 0, err, result;
> +	struct test_subskeleton *skel;
> +
> +	skel = test_subskeleton__open();
> +	if (CHECK(!skel, "skel_open", "failed to open skeleton\n"))
> +		return;
> +
> +	skel->rodata->rovar1 = 10;

The rodata vars in subskeleton will need extra '*', right?
The above is confusing to read comparing to below:

> +void subskeleton_lib_setup(struct bpf_object *obj)
> +{
> +	struct test_subskeleton_lib *lib = test_subskeleton_lib__open(obj);
> +
> +	ASSERT_OK_PTR(lib, "open subskeleton");
> +
> +	*lib->data.var1 = 1;
> +	*lib->bss.var2 = 2;
> +	lib->bss.var3->var3_1 = 3;
> +	lib->bss.var3->var3_2 = 4;
> +}

Could you add rodata to subskel as well?
Just to make it obvious that rodata is not special.

An example of generated skel in commit log would be great.

Re: [PATCH bpf-next 4/4] selftests/bpf: test subskeleton functionality

[Delyan Kratunov]

On Wed, 2022-03-02 at 14:30 -0800, Alexei Starovoitov wrote:
> On Wed, Mar 02, 2022 at 02:48:48AM +0000, Delyan Kratunov wrote:
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/prog_tests/subskeleton.c
> > @@ -0,0 +1,38 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/* Copyright (c) 2019 Facebook */
> > +
> > +#include <test_progs.h>
> > +#include "test_subskeleton.skel.h"
> > +
> > +extern void subskeleton_lib_setup(struct bpf_object *obj);
> > +extern int subskeleton_lib_subresult(struct bpf_object *obj);
> > +
> > +void test_subskeleton(void)
> > +{
> > +	int duration = 0, err, result;
> > +	struct test_subskeleton *skel;
> > +
> > +	skel = test_subskeleton__open();
> > +	if (CHECK(!skel, "skel_open", "failed to open skeleton\n"))
> > +		return;
> > +
> > +	skel->rodata->rovar1 = 10;
> 
> The rodata vars in subskeleton will need extra '*', right?

Possibly, depending on the exact structure (see the var3 assignments in the
subskel). 
Overall, the naming here is confusing. This is the subskeleton.c test, the 
subskeleton.o final bpf object, but this is the *full* skeleton for that object.

Naming suggestions welcome!

> The above is confusing to read comparing to below:
> 
> > +void subskeleton_lib_setup(struct bpf_object *obj)
> > +{
> > +	struct test_subskeleton_lib *lib = test_subskeleton_lib__open(obj);
> > +
> > +	ASSERT_OK_PTR(lib, "open subskeleton");
> > +
> > +	*lib->data.var1 = 1;
> > +	*lib->bss.var2 = 2;
> > +	lib->bss.var3->var3_1 = 3;
> > +	lib->bss.var3->var3_2 = 4;
> > +}
> 
> Could you add rodata to subskel as well?
> Just to make it obvious that rodata is not special.

Sure, no objections from me.

> An example of generated skel in commit log would be great.

Will add in reroll. This is it in its entirety:

/* SPDX-License-Identifier: (LGPL-2.1 OR BSD-2-Clause) */

/* THIS FILE IS AUTOGENERATED! */
#ifndef __TEST_SUBSKELETON_LIB_SKEL_H__
#define __TEST_SUBSKELETON_LIB_SKEL_H__

#include <errno.h>
#include <stdlib.h>
#include <bpf/libbpf.h>

struct test_subskeleton_lib {
	struct bpf_object *obj;
	struct bpf_object_subskeleton *subskel;
	struct test_subskeleton_lib__data {
		int *var1;
	} data;
	struct test_subskeleton_lib__bss {
		int *var2;
		struct {
			int var3_1;
			__s64 var3_2;
		} *var3;
		int *libout1;
	} bss;

#ifdef __cplusplus
	static inline struct test_subskeleton_lib *open(const struct
bpf_object_open_opts *opts = nullptr);
	static inline void test_subskeleton_lib::destroy(struct
test_subskeleton_lib *skel);
#endif /* __cplusplus */
};

static inline void
test_subskeleton_lib__destroy(struct test_subskeleton_lib *skel)
{
	if (!skel)
		return;
	if (skel->subskel)
		bpf_object__destroy_subskeleton(skel->subskel);
	free(skel);
}

static inline struct test_subskeleton_lib *
test_subskeleton_lib__open(const struct bpf_object *src)
{
	struct test_subskeleton_lib *obj;
	struct bpf_object_subskeleton *subskel;
	struct bpf_sym_skeleton *syms;
	int err;

	obj = (struct test_subskeleton_lib *)calloc(1, sizeof(*obj));
	if (!obj) {
		errno = ENOMEM;
		return NULL;
	}
	subskel = (struct bpf_object_subskeleton *)calloc(1, sizeof(*subskel));
	if (!subskel) {
		errno = ENOMEM;
		return NULL;
	}
	subskel->sz = sizeof(*subskel);
	subskel->obj = src;
	subskel->sym_skel_sz = sizeof(struct bpf_sym_skeleton);
	subskel->sym_cnt = 4;
	obj->subskel = subskel;

	syms = (struct bpf_sym_skeleton *)calloc(4, sizeof(*syms));
	if (!syms) {
		free(subskel);
		errno = ENOMEM;
		return NULL;
	}
	subskel->syms = syms;

	syms[0].name = "var1";
	syms[0].section = ".data";
	syms[0].addr = (void**) &obj->data.var1;
	syms[1].name = "var2";
	syms[1].section = ".bss";
	syms[1].addr = (void**) &obj->bss.var2;
	syms[2].name = "var3";
	syms[2].section = ".bss";
	syms[2].addr = (void**) &obj->bss.var3;
	syms[3].name = "libout1";
	syms[3].section = ".bss";
	syms[3].addr = (void**) &obj->bss.libout1;

	err = bpf_object__open_subskeleton(subskel);
	if (err) {
		test_subskeleton_lib__destroy(obj);
		errno = err;
		return NULL;
	}

	return obj;
}

#ifdef __cplusplus
struct test_subskeleton_lib *test_subskeleton_lib::open(const struct bpf_object
*src) { return test_subskeleton_lib__open(src); }
void test_subskeleton_lib::destroy(struct test_subskeleton_lib *skel) {
test_subskeleton_lib__destroy(skel); }
#endif /* __cplusplus */

#endif /* __TEST_SUBSKELETON_LIB_SKEL_H__ */

Re: [PATCH bpf-next 4/4] selftests/bpf: test subskeleton functionality

[Andrii Nakryiko]

On Tue, Mar 1, 2022 at 6:49 PM Delyan Kratunov <delyank@fb.com> wrote:
>
> The new Makefile support via SUBSKELS and .skel.h-deps is a mixture
> of LINKED_SKELS and LSKELS. By definition subskeletons require multiple
> BPF object files to be linked together. However, generating the
> subskeleton only requires the library object file and not the final
> program object file.
>
> Signed-off-by: Delyan Kratunov <delyank@fb.com>
> ---
>  tools/testing/selftests/bpf/Makefile          | 18 ++++++++-
>  .../selftests/bpf/prog_tests/subskeleton.c    | 38 +++++++++++++++++++
>  .../bpf/prog_tests/subskeleton_lib.c          | 29 ++++++++++++++
>  .../selftests/bpf/progs/test_subskeleton.c    | 20 ++++++++++
>  .../bpf/progs/test_subskeleton_lib.c          | 22 +++++++++++
>  5 files changed, 125 insertions(+), 2 deletions(-)
>  create mode 100644 tools/testing/selftests/bpf/prog_tests/subskeleton.c
>  create mode 100644 tools/testing/selftests/bpf/prog_tests/subskeleton_lib.c
>  create mode 100644 tools/testing/selftests/bpf/progs/test_subskeleton.c
>  create mode 100644 tools/testing/selftests/bpf/progs/test_subskeleton_lib.c
>
> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> index fe12b4f5fe20..57da63ba790b 100644
> --- a/tools/testing/selftests/bpf/Makefile
> +++ b/tools/testing/selftests/bpf/Makefile
> @@ -326,19 +326,23 @@ endef
>  SKEL_BLACKLIST := btf__% test_pinning_invalid.c test_sk_assign.c
>
>  LINKED_SKELS := test_static_linked.skel.h linked_funcs.skel.h          \
> -               linked_vars.skel.h linked_maps.skel.h
> +               linked_vars.skel.h linked_maps.skel.h test_subskeleton.skel.h
> +
> +SUBSKELS := test_subskeleton_lib.skel.h

So, unless I'm mistaken, bpf_object__open() will succeed for
"incomplete" BPF object file (e.g., even if they have unresolved
externs, for example). At least that used to be the case.

In such a case, we can totally generate both skeletons and
sub-skeletons for all files for which we currently generate skeletons.
It will keep Makefile simpler and will test sub-skeleton code
generator on a much wider variety of BPF object files. Let's use
.subskel.h naming convention for those. We can even add a simple test
in test_skeleton, test_vmlinux and a bunch of others that "stress
test" skeleton features to make sure that corresponding sub-skeleton
can be opened just fine.

We probably will run into name conflicts for <skel>__open and
<skel>__destroy... So we can either use <skel>__open_subskel and
<skel>__destroy_subskel to disambiguate (might not be a bad idea to
make it clear that we are dealing with "incomplete" sub-skeleton), or
we can just not test skeleton and sub-skeleton in the same user-space
.c file. Not sure if anyone feels strongly about naming, let me know.

>
>  LSKELS := kfunc_call_test.c fentry_test.c fexit_test.c fexit_sleep.c \
>         test_ringbuf.c atomics.c trace_printk.c trace_vprintk.c \
>         map_ptr_kern.c core_kern.c core_kern_overflow.c
>  # Generate both light skeleton and libbpf skeleton for these
>  LSKELS_EXTRA := test_ksyms_module.c test_ksyms_weak.c kfunc_call_test_subprog.c
> -SKEL_BLACKLIST += $$(LSKELS)
> +SKEL_BLACKLIST += $$(LSKELS) $$(SUBSKELS)
>
>  test_static_linked.skel.h-deps := test_static_linked1.o test_static_linked2.o
>  linked_funcs.skel.h-deps := linked_funcs1.o linked_funcs2.o
>  linked_vars.skel.h-deps := linked_vars1.o linked_vars2.o
>  linked_maps.skel.h-deps := linked_maps1.o linked_maps2.o
> +test_subskeleton.skel.h-deps := test_subskeleton_lib.o test_subskeleton.o
> +test_subskeleton_lib.skel.h-deps := test_subskeleton_lib.o
>
>  LINKED_BPF_SRCS := $(patsubst %.o,%.c,$(foreach skel,$(LINKED_SKELS),$($(skel)-deps)))
>
> @@ -363,6 +367,7 @@ TRUNNER_BPF_SKELS := $$(patsubst %.c,$$(TRUNNER_OUTPUT)/%.skel.h,   \
>                                  $$(filter-out $(SKEL_BLACKLIST) $(LINKED_BPF_SRCS),\
>                                                $$(TRUNNER_BPF_SRCS)))
>  TRUNNER_BPF_LSKELS := $$(patsubst %.c,$$(TRUNNER_OUTPUT)/%.lskel.h, $$(LSKELS) $$(LSKELS_EXTRA))
> +TRUNNER_BPF_SUBSKELS := $$(addprefix $$(TRUNNER_OUTPUT)/,$(SUBSKELS))
>  TRUNNER_BPF_SKELS_LINKED := $$(addprefix $$(TRUNNER_OUTPUT)/,$(LINKED_SKELS))
>  TEST_GEN_FILES += $$(TRUNNER_BPF_OBJS)
>
> @@ -405,6 +410,14 @@ $(TRUNNER_BPF_SKELS): %.skel.h: %.o $(BPFTOOL) | $(TRUNNER_OUTPUT)
>         $(Q)diff $$(<:.o=.linked2.o) $$(<:.o=.linked3.o)
>         $(Q)$$(BPFTOOL) gen skeleton $$(<:.o=.linked3.o) name $$(notdir $$(<:.o=)) > $$@
>
> +$(TRUNNER_BPF_SUBSKELS): %.skel.h: %.o $(BPFTOOL) | $(TRUNNER_OUTPUT)
> +       $$(call msg,GEN-SUBSKEL,$(TRUNNER_BINARY),$$@)
> +       $(Q)$$(BPFTOOL) gen object $$(<:.o=.linked1.o) $$<
> +       $(Q)$$(BPFTOOL) gen object $$(<:.o=.linked2.o) $$(<:.o=.linked1.o)
> +       $(Q)$$(BPFTOOL) gen object $$(<:.o=.linked3.o) $$(<:.o=.linked2.o)
> +       $(Q)diff $$(<:.o=.linked2.o) $$(<:.o=.linked3.o)
> +       $(Q)$$(BPFTOOL) gen subskeleton $$(<:.o=.linked3.o) name $$(notdir $$(<:.o=)) > $$@
> +
>  $(TRUNNER_BPF_LSKELS): %.lskel.h: %.o $(BPFTOOL) | $(TRUNNER_OUTPUT)
>         $$(call msg,GEN-SKEL,$(TRUNNER_BINARY),$$@)
>         $(Q)$$(BPFTOOL) gen object $$(<:.o=.linked1.o) $$<
> @@ -441,6 +454,7 @@ $(TRUNNER_TEST_OBJS): $(TRUNNER_OUTPUT)/%.test.o:                   \
>                       $(TRUNNER_EXTRA_HDRS)                             \
>                       $(TRUNNER_BPF_OBJS)                               \
>                       $(TRUNNER_BPF_SKELS)                              \
> +                     $(TRUNNER_BPF_SUBSKELS)                           \
>                       $(TRUNNER_BPF_LSKELS)                             \
>                       $(TRUNNER_BPF_SKELS_LINKED)                       \
>                       $$(BPFOBJ) | $(TRUNNER_OUTPUT)
> diff --git a/tools/testing/selftests/bpf/prog_tests/subskeleton.c b/tools/testing/selftests/bpf/prog_tests/subskeleton.c
> new file mode 100644
> index 000000000000..651aafc28e7f
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/subskeleton.c
> @@ -0,0 +1,38 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (c) 2019 Facebook */
> +

year is off

> +#include <test_progs.h>
> +#include "test_subskeleton.skel.h"
> +
> +extern void subskeleton_lib_setup(struct bpf_object *obj);
> +extern int subskeleton_lib_subresult(struct bpf_object *obj);
> +
> +void test_subskeleton(void)
> +{
> +       int duration = 0, err, result;
> +       struct test_subskeleton *skel;
> +
> +       skel = test_subskeleton__open();
> +       if (CHECK(!skel, "skel_open", "failed to open skeleton\n"))

no CHECK()s

> +               return;
> +
> +       skel->rodata->rovar1 = 10;
> +
> +       err = test_subskeleton__load(skel);
> +       if (CHECK(err, "skel_load", "failed to load skeleton: %d\n", err))

CHECK

> +               goto cleanup;
> +
> +       subskeleton_lib_setup(skel->obj);
> +
> +       err = test_subskeleton__attach(skel);
> +       if (CHECK(err, "skel_attach", "skeleton attach failed: %d\n", err))

CHECK

> +               goto cleanup;
> +
> +       /* trigger tracepoint */
> +       usleep(1);
> +
> +       result = subskeleton_lib_subresult(skel->obj) * 10;
> +       ASSERT_EQ(skel->bss->out1, result, "unexpected calculation");
> +cleanup:
> +       test_subskeleton__destroy(skel);
> +}
> diff --git a/tools/testing/selftests/bpf/prog_tests/subskeleton_lib.c b/tools/testing/selftests/bpf/prog_tests/subskeleton_lib.c
> new file mode 100644
> index 000000000000..f7f98b3febaf
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/subskeleton_lib.c
> @@ -0,0 +1,29 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (c) 2019 Facebook */
> +

outdated year?

> +#include <test_progs.h>
> +#include <bpf/libbpf.h>
> +
> +#include "test_subskeleton_lib.skel.h"
> +
> +void subskeleton_lib_setup(struct bpf_object *obj)
> +{
> +       struct test_subskeleton_lib *lib = test_subskeleton_lib__open(obj);
> +
> +       ASSERT_OK_PTR(lib, "open subskeleton");

return on failed assert, otherwise SIGSEGV

> +
> +       *lib->data.var1 = 1;
> +       *lib->bss.var2 = 2;
> +       lib->bss.var3->var3_1 = 3;
> +       lib->bss.var3->var3_2 = 4;
> +}
> +
> +int subskeleton_lib_subresult(struct bpf_object *obj)
> +{
> +       struct test_subskeleton_lib *lib = test_subskeleton_lib__open(obj);
> +
> +       ASSERT_OK_PTR(lib, "open subskeleton");
> +
> +       ASSERT_EQ(*lib->bss.libout1, 1 + 2 + 3 + 4, "lib subresult");
> +       return *lib->bss.libout1;
> +}

I'm not sure we really need to have a separate user-space file to
simulate a library code. Let's have this library setup code in the
selftest file itself

> diff --git a/tools/testing/selftests/bpf/progs/test_subskeleton.c b/tools/testing/selftests/bpf/progs/test_subskeleton.c
> new file mode 100644
> index 000000000000..bad3970718cb
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/test_subskeleton.c
> @@ -0,0 +1,20 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (c) 2021 Facebook */
> +
> +#include <linux/bpf.h>
> +#include <bpf/bpf_helpers.h>
> +
> +const int rovar1;
> +int out1;

see below, let's have some shared stuff between skeleton and
subskeleton (.kconfig, variable used from lib, variable defined in
lib, etc). Think creatively on how you could break codegen :)

As we want to add maps, I'd also use extern maps for more coverage

> +
> +extern int lib_routine(void);
> +
> +SEC("raw_tp/sys_enter")
> +int handler1(const void *ctx)
> +{
> +       out1 = lib_routine() * rovar1;
> +       return 0;
> +}
> +
> +char LICENSE[] SEC("license") = "GPL";
> +int VERSION SEC("version") = 1;

see below, no VERSION nowadays

> diff --git a/tools/testing/selftests/bpf/progs/test_subskeleton_lib.c b/tools/testing/selftests/bpf/progs/test_subskeleton_lib.c
> new file mode 100644
> index 000000000000..23c7f24997a7
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/test_subskeleton_lib.c
> @@ -0,0 +1,22 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (c) 2021 Facebook */
> +
> +#include <linux/bpf.h>
> +#include <bpf/bpf_helpers.h>
> +
> +int var1 = -1;
> +int var2;
> +struct {
> +       int var3_1;
> +       __s64 var3_2;
> +} var3;
> +int libout1;

we should also test:

- .kconfig externs
- __weak variables
- .rodata variable (like Alexei already mentioned)
- let's also have an array variable (C uses non-uniform syntax for
pointer to an array)
- extern .data variable defined in another file

> +
> +int lib_routine(void)
> +{
> +       libout1 =  var1 + var2 + var3.var3_1 + var3.var3_2;

nit: extra space after =

> +       return libout1;
> +}
> +
> +char LICENSE[] SEC("license") = "GPL";
> +int VERSION SEC("version") = 1;

VERSION is obsolete, please drop

> --
> 2.34.1

[PATCH bpf-next 1/4] libbpf: expose map elf section name

[Delyan Kratunov]

When generating subskeletons, bpftool needs to know the elf section
name, as that's the stable identifier that will survive into the final
linked bpf_object.

This patch adds bpf_map__section_name in symmetry with
bpf_program__section_name.

Signed-off-by: Delyan Kratunov <delyank@fb.com>
---
 tools/lib/bpf/libbpf.c         | 8 ++++++++
 tools/lib/bpf/libbpf.h         | 2 ++
 tools/lib/bpf/libbpf.map       | 5 +++++
 tools/lib/bpf/libbpf_version.h | 2 +-
 4 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index be6480e260c4..d20ae8f225ee 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -9180,6 +9180,14 @@ const char *bpf_map__name(const struct bpf_map *map)
 	return map->name;
 }
 
+const char *bpf_map__section_name(const struct bpf_map *map)
+{
+	if (!map)
+		return NULL;
+
+	return map->real_name;
+}
+
 enum bpf_map_type bpf_map__type(const struct bpf_map *map)
 {
 	return map->def.type;
diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
index c8d8daad212e..7b66794f1c0a 100644
--- a/tools/lib/bpf/libbpf.h
+++ b/tools/lib/bpf/libbpf.h
@@ -741,6 +741,8 @@ LIBBPF_API LIBBPF_DEPRECATED_SINCE(0, 8, "use appropriate getters or setters ins
 const struct bpf_map_def *bpf_map__def(const struct bpf_map *map);
 /* get map name */
 LIBBPF_API const char *bpf_map__name(const struct bpf_map *map);
+/* get map ELF section name */
+LIBBPF_API const char *bpf_map__section_name(const struct bpf_map *map);
 /* get/set map type */
 LIBBPF_API enum bpf_map_type bpf_map__type(const struct bpf_map *map);
 LIBBPF_API int bpf_map__set_type(struct bpf_map *map, enum bpf_map_type type);
diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
index 47e70c9058d9..5c85d297d955 100644
--- a/tools/lib/bpf/libbpf.map
+++ b/tools/lib/bpf/libbpf.map
@@ -439,3 +439,8 @@ LIBBPF_0.7.0 {
 		libbpf_probe_bpf_prog_type;
 		libbpf_set_memlock_rlim_max;
 } LIBBPF_0.6.0;
+
+LIBBPF_0.8.0 {
+	global:
+    bpf_map__section_name;
+} LIBBPF_0.7.0;
diff --git a/tools/lib/bpf/libbpf_version.h b/tools/lib/bpf/libbpf_version.h
index 0fefefc3500b..61f2039404b6 100644
--- a/tools/lib/bpf/libbpf_version.h
+++ b/tools/lib/bpf/libbpf_version.h
@@ -4,6 +4,6 @@
 #define __LIBBPF_VERSION_H
 
 #define LIBBPF_MAJOR_VERSION 0
-#define LIBBPF_MINOR_VERSION 7
+#define LIBBPF_MINOR_VERSION 8
 
 #endif /* __LIBBPF_VERSION_H */
-- 
2.34.1

Re: [PATCH bpf-next 1/4] libbpf: expose map elf section name

[Andrii Nakryiko]

On Tue, Mar 1, 2022 at 6:49 PM Delyan Kratunov <delyank@fb.com> wrote:
>
> When generating subskeletons, bpftool needs to know the elf section
> name, as that's the stable identifier that will survive into the final
> linked bpf_object.
>
> This patch adds bpf_map__section_name in symmetry with
> bpf_program__section_name.
>
> Signed-off-by: Delyan Kratunov <delyank@fb.com>
> ---
>  tools/lib/bpf/libbpf.c         | 8 ++++++++
>  tools/lib/bpf/libbpf.h         | 2 ++
>  tools/lib/bpf/libbpf.map       | 5 +++++
>  tools/lib/bpf/libbpf_version.h | 2 +-
>  4 files changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index be6480e260c4..d20ae8f225ee 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -9180,6 +9180,14 @@ const char *bpf_map__name(const struct bpf_map *map)
>         return map->name;
>  }
>
> +const char *bpf_map__section_name(const struct bpf_map *map)
> +{
> +       if (!map)
> +               return NULL;
> +
> +       return map->real_name;
> +}
> +

First, "section_name" here is extremely confusing in the face of
bpf_program__section_name() which returns a very different thing for
BPF program. But I think we shouldn't need to do anything extra here.
Using bpf_map__name() and then bpf_object__find_map_by_name() should
just work (there is real_name special-handling for maps that start
with dot). If that real_name special handling doesn't work for
subskeletons, we should fix that special handling instead of adding a
special getter. But I'll need to look at other patches first and maybe
play around locally with subskeletons.


>  enum bpf_map_type bpf_map__type(const struct bpf_map *map)
>  {
>         return map->def.type;
> diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> index c8d8daad212e..7b66794f1c0a 100644
> --- a/tools/lib/bpf/libbpf.h
> +++ b/tools/lib/bpf/libbpf.h
> @@ -741,6 +741,8 @@ LIBBPF_API LIBBPF_DEPRECATED_SINCE(0, 8, "use appropriate getters or setters ins
>  const struct bpf_map_def *bpf_map__def(const struct bpf_map *map);
>  /* get map name */
>  LIBBPF_API const char *bpf_map__name(const struct bpf_map *map);
> +/* get map ELF section name */
> +LIBBPF_API const char *bpf_map__section_name(const struct bpf_map *map);
>  /* get/set map type */
>  LIBBPF_API enum bpf_map_type bpf_map__type(const struct bpf_map *map);
>  LIBBPF_API int bpf_map__set_type(struct bpf_map *map, enum bpf_map_type type);
> diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
> index 47e70c9058d9..5c85d297d955 100644
> --- a/tools/lib/bpf/libbpf.map
> +++ b/tools/lib/bpf/libbpf.map
> @@ -439,3 +439,8 @@ LIBBPF_0.7.0 {
>                 libbpf_probe_bpf_prog_type;
>                 libbpf_set_memlock_rlim_max;
>  } LIBBPF_0.6.0;
> +
> +LIBBPF_0.8.0 {
> +       global:
> +    bpf_map__section_name;
> +} LIBBPF_0.7.0;
> diff --git a/tools/lib/bpf/libbpf_version.h b/tools/lib/bpf/libbpf_version.h
> index 0fefefc3500b..61f2039404b6 100644
> --- a/tools/lib/bpf/libbpf_version.h
> +++ b/tools/lib/bpf/libbpf_version.h
> @@ -4,6 +4,6 @@
>  #define __LIBBPF_VERSION_H
>
>  #define LIBBPF_MAJOR_VERSION 0
> -#define LIBBPF_MINOR_VERSION 7
> +#define LIBBPF_MINOR_VERSION 8
>
>  #endif /* __LIBBPF_VERSION_H */
> --
> 2.34.1

Re: [PATCH bpf-next 1/4] libbpf: expose map elf section name

[Delyan Kratunov]

On Wed, 2022-03-02 at 17:13 -0800, Andrii Nakryiko wrote:
> First, "section_name" here is extremely confusing in the face of
> bpf_program__section_name() which returns a very different thing for
> BPF program. But I think we shouldn't need to do anything extra here.
> Using bpf_map__name() and then bpf_object__find_map_by_name() should
> just work (there is real_name special-handling for maps that start
> with dot). If that real_name special handling doesn't work for
> subskeletons, we should fix that special handling instead of adding a
> special getter.

That special handling did not do the right thing when I first tried it but I'll
poke at it, if that's the preferred approach!

Thanks,
Delyan

[PATCH bpf-next 2/4] bpftool: add support for subskeletons

[Delyan Kratunov]

Subskeletons are headers which require an already loaded program to
operate.

For example, when a BPF library is linked into a larger BPF object file,
the library userspace needs a way to access its own global variables
without requiring knowledge about the larger program at build time.

As a result, subskeletons require a loaded bpf_object to open().
Further, they find their own symbols in the larger program by
walking BTF type data at run time.

At this time, only globals are supported, though non-owning references
to links, programs, and other objects may be added as the needs arise.

Signed-off-by: Delyan Kratunov <delyank@fb.com>
---
 tools/bpf/bpftool/gen.c | 322 ++++++++++++++++++++++++++++++++++++++--
 1 file changed, 313 insertions(+), 9 deletions(-)

diff --git a/tools/bpf/bpftool/gen.c b/tools/bpf/bpftool/gen.c
index 145734b4fe41..ea292e09c17b 100644
--- a/tools/bpf/bpftool/gen.c
+++ b/tools/bpf/bpftool/gen.c
@@ -125,14 +125,14 @@ static int codegen_datasec_def(struct bpf_object *obj,
 			       struct btf *btf,
 			       struct btf_dump *d,
 			       const struct btf_type *sec,
-			       const char *obj_name)
+			       const char *obj_name,
+			       bool subskel)
 {
 	const char *sec_name = btf__name_by_offset(btf, sec->name_off);
 	const struct btf_var_secinfo *sec_var = btf_var_secinfos(sec);
 	int i, err, off = 0, pad_cnt = 0, vlen = btf_vlen(sec);
 	char var_ident[256], sec_ident[256];
 	bool strip_mods = false;
-
 	if (!get_datasec_ident(sec_name, sec_ident, sizeof(sec_ident)))
 		return 0;
 
@@ -183,7 +183,7 @@ static int codegen_datasec_def(struct bpf_object *obj,
 			align = 4;
 
 		align_off = (off + align - 1) / align * align;
-		if (align_off != need_off) {
+		if (align_off != need_off && !subskel) {
 			printf("\t\tchar __pad%d[%d];\n",
 			       pad_cnt, need_off - off);
 			pad_cnt++;
@@ -197,6 +197,15 @@ static int codegen_datasec_def(struct bpf_object *obj,
 		strncat(var_ident, var_name, sizeof(var_ident) - 1);
 		sanitize_identifier(var_ident);
 
+		/* to emit a pointer to the type in the map, we need to
+		 * make sure our btf has that pointer type first.
+		 */
+		if (subskel) {
+			var_type_id = btf__add_ptr(btf, var_type_id);
+			if (var_type_id < 0)
+				return var_type_id;
+		}
+
 		printf("\t\t");
 		err = btf_dump__emit_type_decl(d, var_type_id, &opts);
 		if (err)
@@ -205,7 +214,10 @@ static int codegen_datasec_def(struct bpf_object *obj,
 
 		off = sec_var->offset + sec_var->size;
 	}
-	printf("	} *%s;\n", sec_ident);
+	if (subskel)
+		printf("	} %s;\n", sec_ident);
+	else
+		printf("	} *%s;\n", sec_ident);
 	return 0;
 }
 
@@ -231,7 +243,7 @@ static const struct btf_type *find_type_for_map(struct btf *btf, const char *map
 	return NULL;
 }
 
-static int codegen_datasecs(struct bpf_object *obj, const char *obj_name)
+static int codegen_datasecs(struct bpf_object *obj, const char *obj_name, bool subskel)
 {
 	struct btf *btf = bpf_object__btf(obj);
 	struct btf_dump *d;
@@ -240,6 +252,13 @@ static int codegen_datasecs(struct bpf_object *obj, const char *obj_name)
 	char map_ident[256];
 	int err = 0;
 
+	/* When generating a subskeleton, we need to emit _pointers_
+	 * to the types in the maps. Use a new btf object as storage for these
+	 * new types as they're not guaranteed to already exist.
+	 */
+	if (subskel)
+		btf = btf__new_empty_split(btf);
+
 	d = btf_dump__new(btf, codegen_btf_dump_printf, NULL, NULL);
 	err = libbpf_get_error(d);
 	if (err)
@@ -264,11 +283,11 @@ static int codegen_datasecs(struct bpf_object *obj, const char *obj_name)
 		 * map. It will still be memory-mapped and its contents
 		 * accessible from user-space through BPF skeleton.
 		 */
-		if (!sec) {
+		if (!sec && !subskel) {
 			printf("	struct %s__%s {\n", obj_name, map_ident);
 			printf("	} *%s;\n", map_ident);
-		} else {
-			err = codegen_datasec_def(obj, btf, d, sec, obj_name);
+		} else if (sec) {
+			err = codegen_datasec_def(obj, btf, d, sec, obj_name, subskel);
 			if (err)
 				goto out;
 		}
@@ -276,6 +295,8 @@ static int codegen_datasecs(struct bpf_object *obj, const char *obj_name)
 
 
 out:
+	if (subskel)
+		btf__free(btf);
 	btf_dump__free(d);
 	return err;
 }
@@ -896,7 +917,7 @@ static int do_skeleton(int argc, char **argv)
 
 	btf = bpf_object__btf(obj);
 	if (btf) {
-		err = codegen_datasecs(obj, obj_name);
+		err = codegen_datasecs(obj, obj_name, false);
 		if (err)
 			goto out;
 	}
@@ -1141,6 +1162,287 @@ static int do_skeleton(int argc, char **argv)
 	return err;
 }
 
+/* Subskeletons are like skeletons, except they don't own the bpf_object,
+ * associated maps, links, etc. Instead, they know about the existence of
+ * a certain number of datasec fields and are able to find their locations
+ * _at runtime_ from an already loaded bpf_object.
+ *
+ * This allows for library-like BPF objects to have userspace counterparts
+ * with access to their globals without having to know anything about the
+ * final BPF object that the library was linked into.
+ */
+static int do_subskeleton(int argc, char **argv)
+{
+	char header_guard[MAX_OBJ_NAME_LEN + sizeof("__SKEL_H__")];
+	size_t i, len, file_sz, mmap_sz, sym_sz = 0, sym_idx = 0;
+	DECLARE_LIBBPF_OPTS(bpf_object_open_opts, opts);
+	char obj_name[MAX_OBJ_NAME_LEN] = "", *obj_data;
+	struct bpf_object *obj = NULL;
+	const char *file, *var_name;
+	char ident[256], var_ident[256];
+	int fd, err = -1, map_type_id;
+	const struct bpf_map *map;
+	struct btf *btf;
+	const struct btf_type *map_type, *var_type;
+	const struct btf_var_secinfo *var;
+	struct stat st;
+
+	if (!REQ_ARGS(1)) {
+		usage();
+		return -1;
+	}
+	file = GET_ARG();
+
+	while (argc) {
+		if (!REQ_ARGS(2))
+			return -1;
+
+		if (is_prefix(*argv, "name")) {
+			NEXT_ARG();
+
+			if (obj_name[0] != '\0') {
+				p_err("object name already specified");
+				return -1;
+			}
+
+			strncpy(obj_name, *argv, MAX_OBJ_NAME_LEN - 1);
+			obj_name[MAX_OBJ_NAME_LEN - 1] = '\0';
+		} else {
+			p_err("unknown arg %s", *argv);
+			return -1;
+		}
+
+		NEXT_ARG();
+	}
+
+	if (argc) {
+		p_err("extra unknown arguments");
+		return -1;
+	}
+
+	if (use_loader) {
+		p_err("cannot use loader for subskeletons");
+		return -1;
+	}
+
+	if (stat(file, &st)) {
+		p_err("failed to stat() %s: %s", file, strerror(errno));
+		return -1;
+	}
+	file_sz = st.st_size;
+	mmap_sz = roundup(file_sz, sysconf(_SC_PAGE_SIZE));
+	fd = open(file, O_RDONLY);
+	if (fd < 0) {
+		p_err("failed to open() %s: %s", file, strerror(errno));
+		return -1;
+	}
+	obj_data = mmap(NULL, mmap_sz, PROT_READ, MAP_PRIVATE, fd, 0);
+	if (obj_data == MAP_FAILED) {
+		obj_data = NULL;
+		p_err("failed to mmap() %s: %s", file, strerror(errno));
+		goto out;
+	}
+	if (obj_name[0] == '\0')
+		get_obj_name(obj_name, file);
+	opts.object_name = obj_name;
+	if (verifier_logs)
+		/* log_level1 + log_level2 + stats, but not stable UAPI */
+		opts.kernel_log_level = 1 + 2 + 4;
+	obj = bpf_object__open_mem(obj_data, file_sz, &opts);
+	err = libbpf_get_error(obj);
+	if (err) {
+		char err_buf[256];
+
+		libbpf_strerror(err, err_buf, sizeof(err_buf));
+		p_err("failed to open BPF object file: %s", err_buf);
+		obj = NULL;
+		goto out;
+	}
+
+	btf = bpf_object__btf(obj);
+	err = libbpf_get_error(btf);
+	if (err) {
+		err = -1;
+		p_err("need btf type information for %s", obj_name);
+		goto out;
+	}
+
+	/* First, count how many symbols we have to link. */
+	bpf_object__for_each_map(map, obj) {
+		if (!bpf_map__is_internal(map))
+			continue;
+
+		if (!(bpf_map__map_flags(map) & BPF_F_MMAPABLE))
+			continue;
+
+		if (!get_map_ident(map, ident, sizeof(ident)))
+			continue;
+
+		map_type_id = btf__find_by_name_kind(btf, bpf_map__section_name(map), BTF_KIND_DATASEC);
+		if (map_type_id < 0) {
+			err = map_type_id;
+			goto out;
+		}
+		map_type = btf__type_by_id(btf, map_type_id);
+
+		for (i = 0, var = btf_var_secinfos(map_type), len = btf_vlen(map_type);
+		     i < len;
+		     i++, var++) {
+			sym_sz++;
+		}
+	}
+
+	get_header_guard(header_guard, obj_name);
+	codegen("\
+	\n\
+	/* SPDX-License-Identifier: (LGPL-2.1 OR BSD-2-Clause) */   \n\
+								    \n\
+	/* THIS FILE IS AUTOGENERATED! */			    \n\
+	#ifndef %2$s						    \n\
+	#define %2$s						    \n\
+								    \n\
+	#include <errno.h>					    \n\
+	#include <stdlib.h>					    \n\
+	#include <bpf/libbpf.h>					    \n\
+								    \n\
+	struct %1$s {						    \n\
+		struct bpf_object *obj;				    \n\
+		struct bpf_object_subskeleton *subskel;		    \n\
+	", obj_name, header_guard);
+
+	err = codegen_datasecs(obj, obj_name, true);
+	if (err)
+		goto out;
+
+	/* emit code that will allocate enough storage for all symbols */
+	codegen("\
+		\n\
+									    \n\
+		#ifdef __cplusplus					    \n\
+			static inline struct %1$s *open(const struct bpf_object *src);\n\
+			static inline void %1$s::destroy(struct %1$s *skel);\n\
+		#endif /* __cplusplus */				    \n\
+		};							    \n\
+									    \n\
+		static inline void					    \n\
+		%1$s__destroy(struct %1$s *skel)			    \n\
+		{							    \n\
+			if (!skel)					    \n\
+				return;					    \n\
+			if (skel->subskel)				    \n\
+				bpf_object__destroy_subskeleton(skel->subskel);\n\
+			free(skel);					    \n\
+		}							    \n\
+									    \n\
+		static inline struct %1$s *				    \n\
+		%1$s__open(const struct bpf_object *src)		    \n\
+		{							    \n\
+			struct %1$s *obj;				    \n\
+			struct bpf_object_subskeleton *subskel;		    \n\
+			struct bpf_sym_skeleton *syms;			    \n\
+			int err;					    \n\
+									    \n\
+			obj = (struct %1$s *)calloc(1, sizeof(*obj));	    \n\
+			if (!obj) {					    \n\
+				errno = ENOMEM;				    \n\
+				return NULL;				    \n\
+			}						    \n\
+			subskel = (struct bpf_object_subskeleton *)calloc(1, sizeof(*subskel));\n\
+			if (!subskel) {					    \n\
+				errno = ENOMEM;				    \n\
+				return NULL;				    \n\
+			}						    \n\
+			subskel->sz = sizeof(*subskel);			    \n\
+			subskel->obj = src;				    \n\
+			subskel->sym_skel_sz = sizeof(struct bpf_sym_skeleton); \n\
+			subskel->sym_cnt = %2$d;			    \n\
+			obj->subskel = subskel;				    \n\
+									    \n\
+			syms = (struct bpf_sym_skeleton *)calloc(%2$d, sizeof(*syms));\n\
+			if (!syms) {					    \n\
+				free(subskel);				    \n\
+				errno = ENOMEM;				    \n\
+				return NULL;				    \n\
+			}						    \n\
+			subskel->syms = syms;				    \n\
+									    \n\
+		",
+		obj_name, sym_sz
+	);
+
+	/* walk through each symbol and emit the runtime representation
+	 */
+	bpf_object__for_each_map(map, obj) {
+		if (!bpf_map__is_internal(map))
+			continue;
+
+		if (!(bpf_map__map_flags(map) & BPF_F_MMAPABLE))
+			continue;
+
+		if (!get_map_ident(map, ident, sizeof(ident)))
+			continue;
+
+		map_type_id = btf__find_by_name_kind(btf, bpf_map__section_name(map), BTF_KIND_DATASEC);
+		if (map_type_id < 0) {
+			err = map_type_id;
+			goto out;
+		}
+		map_type = btf__type_by_id(btf, map_type_id);
+
+		for (i = 0, var = btf_var_secinfos(map_type), len = btf_vlen(map_type);
+		     i < len;
+		     i++, var++, sym_idx++) {
+			var_type = btf__type_by_id(btf, var->type);
+			var_name = btf__name_by_offset(btf, var_type->name_off);
+
+			var_ident[0] = '\0';
+			strncat(var_ident, var_name, sizeof(var_ident) - 1);
+			sanitize_identifier(var_ident);
+
+			codegen("\
+			\n\
+				syms[%4$d].name = \"%1$s\";		    \n\
+				syms[%4$d].section = \"%3$s\";		    \n\
+				syms[%4$d].addr = (void**) &obj->%2$s.%1$s; \n\
+			", var_ident, ident, bpf_map__section_name(map), sym_idx);
+		}
+	}
+
+	codegen("\
+		\n\
+									    \n\
+			err = bpf_object__open_subskeleton(subskel);	    \n\
+			if (err) {					    \n\
+				%1$s__destroy(obj);			    \n\
+				errno = err;				    \n\
+				return NULL;				    \n\
+			}						    \n\
+									    \n\
+			return obj;					    \n\
+		}							    \n\
+		",
+		obj_name);
+
+	codegen("\
+		\n\
+									    \n\
+		#ifdef __cplusplus					    \n\
+		struct %1$s *%1$s::open(const struct bpf_object *src) { return %1$s__open(src); }\n\
+		void %1$s::destroy(struct %1$s *skel) { %1$s__destroy(skel); }\n\
+		#endif /* __cplusplus */				    \n\
+									    \n\
+		#endif /* %2$s */					    \n\
+		",
+		obj_name, header_guard);
+	err = 0;
+out:
+	bpf_object__close(obj);
+	if (obj_data)
+		munmap(obj_data, mmap_sz);
+	close(fd);
+	return err;
+}
+
 static int do_object(int argc, char **argv)
 {
 	struct bpf_linker *linker;
@@ -1192,6 +1494,7 @@ static int do_help(int argc, char **argv)
 	fprintf(stderr,
 		"Usage: %1$s %2$s object OUTPUT_FILE INPUT_FILE [INPUT_FILE...]\n"
 		"       %1$s %2$s skeleton FILE [name OBJECT_NAME]\n"
+		"       %1$s %2$s subskeleton FILE [name OBJECT_NAME]\n"
 		"       %1$s %2$s min_core_btf INPUT OUTPUT OBJECT [OBJECT...]\n"
 		"       %1$s %2$s help\n"
 		"\n"
@@ -1788,6 +2091,7 @@ static int do_min_core_btf(int argc, char **argv)
 static const struct cmd cmds[] = {
 	{ "object",		do_object },
 	{ "skeleton",		do_skeleton },
+	{ "subskeleton",	do_subskeleton },
 	{ "min_core_btf",	do_min_core_btf},
 	{ "help",		do_help },
 	{ 0 }
-- 
2.34.1

Re: [PATCH bpf-next 2/4] bpftool: add support for subskeletons

[Andrii Nakryiko]

On Tue, Mar 1, 2022 at 6:49 PM Delyan Kratunov <delyank@fb.com> wrote:
>
> Subskeletons are headers which require an already loaded program to
> operate.
>
> For example, when a BPF library is linked into a larger BPF object file,
> the library userspace needs a way to access its own global variables
> without requiring knowledge about the larger program at build time.
>
> As a result, subskeletons require a loaded bpf_object to open().
> Further, they find their own symbols in the larger program by
> walking BTF type data at run time.
>
> At this time, only globals are supported, though non-owning references
> to links, programs, and other objects may be added as the needs arise.

There is a need (I needed it in libusdt, for example). Let's add it
from the very beginning, especially that it can be done in *exactly*
the same form as for skeletons.

>
> Signed-off-by: Delyan Kratunov <delyank@fb.com>
> ---
>  tools/bpf/bpftool/gen.c | 322 ++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 313 insertions(+), 9 deletions(-)
>
> diff --git a/tools/bpf/bpftool/gen.c b/tools/bpf/bpftool/gen.c
> index 145734b4fe41..ea292e09c17b 100644
> --- a/tools/bpf/bpftool/gen.c
> +++ b/tools/bpf/bpftool/gen.c
> @@ -125,14 +125,14 @@ static int codegen_datasec_def(struct bpf_object *obj,
>                                struct btf *btf,
>                                struct btf_dump *d,
>                                const struct btf_type *sec,
> -                              const char *obj_name)
> +                              const char *obj_name,
> +                              bool subskel)

seems like subskel's datasec codegen is considerably different (and
simpler, really) than skeleton's, I'd add a separate function for it
instead of all this if (subskel) special-casing. Main concern for
skeleton is generating correct memory layout, while for subskel we
just need to generate a list of pointers without any regard to memory
layout.

>  {
>         const char *sec_name = btf__name_by_offset(btf, sec->name_off);
>         const struct btf_var_secinfo *sec_var = btf_var_secinfos(sec);
>         int i, err, off = 0, pad_cnt = 0, vlen = btf_vlen(sec);
>         char var_ident[256], sec_ident[256];
>         bool strip_mods = false;
> -

why? there should be an empty line between variable block and first statement

>         if (!get_datasec_ident(sec_name, sec_ident, sizeof(sec_ident)))
>                 return 0;
>
> @@ -183,7 +183,7 @@ static int codegen_datasec_def(struct bpf_object *obj,
>                         align = 4;
>
>                 align_off = (off + align - 1) / align * align;
> -               if (align_off != need_off) {
> +               if (align_off != need_off && !subskel) {
>                         printf("\t\tchar __pad%d[%d];\n",
>                                pad_cnt, need_off - off);
>                         pad_cnt++;
> @@ -197,6 +197,15 @@ static int codegen_datasec_def(struct bpf_object *obj,
>                 strncat(var_ident, var_name, sizeof(var_ident) - 1);
>                 sanitize_identifier(var_ident);
>
> +               /* to emit a pointer to the type in the map, we need to
> +                * make sure our btf has that pointer type first.
> +                */
> +               if (subskel) {
> +                       var_type_id = btf__add_ptr(btf, var_type_id);
> +                       if (var_type_id < 0)
> +                               return var_type_id;
> +               }
> +

it's unfortunate to have to modify original BTF just to have this '*'
added.  If I remember correctly, C syntax for pointers has special
case only for arrays and func prototypes, so it should work with this
logic (not tested, obviously)

1. if top variable type is array or func_proto, set var_ident to "(*<variable>)"
2. otherwise set var_ident to "*<variable>"

we'd need to test it for array and func_proto, but it definitely
should work for all other cases


>                 printf("\t\t");
>                 err = btf_dump__emit_type_decl(d, var_type_id, &opts);
>                 if (err)
> @@ -205,7 +214,10 @@ static int codegen_datasec_def(struct bpf_object *obj,
>
>                 off = sec_var->offset + sec_var->size;
>         }
> -       printf("        } *%s;\n", sec_ident);
> +       if (subskel)
> +               printf("        } %s;\n", sec_ident);
> +       else
> +               printf("        } *%s;\n", sec_ident);
>         return 0;
>  }
>
> @@ -231,7 +243,7 @@ static const struct btf_type *find_type_for_map(struct btf *btf, const char *map
>         return NULL;
>  }
>
> -static int codegen_datasecs(struct bpf_object *obj, const char *obj_name)
> +static int codegen_datasecs(struct bpf_object *obj, const char *obj_name, bool subskel)
>  {
>         struct btf *btf = bpf_object__btf(obj);
>         struct btf_dump *d;
> @@ -240,6 +252,13 @@ static int codegen_datasecs(struct bpf_object *obj, const char *obj_name)
>         char map_ident[256];
>         int err = 0;
>
> +       /* When generating a subskeleton, we need to emit _pointers_
> +        * to the types in the maps. Use a new btf object as storage for these
> +        * new types as they're not guaranteed to already exist.
> +        */
> +       if (subskel)
> +               btf = btf__new_empty_split(btf);
> +
>         d = btf_dump__new(btf, codegen_btf_dump_printf, NULL, NULL);
>         err = libbpf_get_error(d);
>         if (err)
> @@ -264,11 +283,11 @@ static int codegen_datasecs(struct bpf_object *obj, const char *obj_name)
>                  * map. It will still be memory-mapped and its contents
>                  * accessible from user-space through BPF skeleton.
>                  */
> -               if (!sec) {
> +               if (!sec && !subskel) {
>                         printf("        struct %s__%s {\n", obj_name, map_ident);
>                         printf("        } *%s;\n", map_ident);
> -               } else {
> -                       err = codegen_datasec_def(obj, btf, d, sec, obj_name);
> +               } else if (sec) {
> +                       err = codegen_datasec_def(obj, btf, d, sec, obj_name, subskel);
>                         if (err)
>                                 goto out;
>                 }
> @@ -276,6 +295,8 @@ static int codegen_datasecs(struct bpf_object *obj, const char *obj_name)
>
>
>  out:
> +       if (subskel)
> +               btf__free(btf);
>         btf_dump__free(d);
>         return err;
>  }
> @@ -896,7 +917,7 @@ static int do_skeleton(int argc, char **argv)
>
>         btf = bpf_object__btf(obj);
>         if (btf) {
> -               err = codegen_datasecs(obj, obj_name);
> +               err = codegen_datasecs(obj, obj_name, false);
>                 if (err)
>                         goto out;
>         }
> @@ -1141,6 +1162,287 @@ static int do_skeleton(int argc, char **argv)
>         return err;
>  }
>
> +/* Subskeletons are like skeletons, except they don't own the bpf_object,
> + * associated maps, links, etc. Instead, they know about the existence of
> + * a certain number of datasec fields and are able to find their locations
> + * _at runtime_ from an already loaded bpf_object.
> + *
> + * This allows for library-like BPF objects to have userspace counterparts
> + * with access to their globals without having to know anything about the
> + * final BPF object that the library was linked into.
> + */
> +static int do_subskeleton(int argc, char **argv)
> +{
> +       char header_guard[MAX_OBJ_NAME_LEN + sizeof("__SKEL_H__")];

use __SUBSKEL_H__ here?

> +       size_t i, len, file_sz, mmap_sz, sym_sz = 0, sym_idx = 0;
> +       DECLARE_LIBBPF_OPTS(bpf_object_open_opts, opts);
> +       char obj_name[MAX_OBJ_NAME_LEN] = "", *obj_data;
> +       struct bpf_object *obj = NULL;
> +       const char *file, *var_name;
> +       char ident[256], var_ident[256];
> +       int fd, err = -1, map_type_id;
> +       const struct bpf_map *map;
> +       struct btf *btf;
> +       const struct btf_type *map_type, *var_type;
> +       const struct btf_var_secinfo *var;
> +       struct stat st;
> +
> +       if (!REQ_ARGS(1)) {
> +               usage();
> +               return -1;
> +       }
> +       file = GET_ARG();
> +
> +       while (argc) {
> +               if (!REQ_ARGS(2))
> +                       return -1;
> +
> +               if (is_prefix(*argv, "name")) {
> +                       NEXT_ARG();
> +
> +                       if (obj_name[0] != '\0') {
> +                               p_err("object name already specified");
> +                               return -1;
> +                       }
> +
> +                       strncpy(obj_name, *argv, MAX_OBJ_NAME_LEN - 1);
> +                       obj_name[MAX_OBJ_NAME_LEN - 1] = '\0';

we should probably force obj_name to be an empty string, so that all
map names match their proper section names

> +               } else {
> +                       p_err("unknown arg %s", *argv);
> +                       return -1;
> +               }
> +
> +               NEXT_ARG();
> +       }
> +
> +       if (argc) {
> +               p_err("extra unknown arguments");
> +               return -1;
> +       }
> +
> +       if (use_loader) {
> +               p_err("cannot use loader for subskeletons");
> +               return -1;
> +       }
> +
> +       if (stat(file, &st)) {
> +               p_err("failed to stat() %s: %s", file, strerror(errno));
> +               return -1;
> +       }
> +       file_sz = st.st_size;
> +       mmap_sz = roundup(file_sz, sysconf(_SC_PAGE_SIZE));
> +       fd = open(file, O_RDONLY);
> +       if (fd < 0) {
> +               p_err("failed to open() %s: %s", file, strerror(errno));
> +               return -1;
> +       }
> +       obj_data = mmap(NULL, mmap_sz, PROT_READ, MAP_PRIVATE, fd, 0);
> +       if (obj_data == MAP_FAILED) {
> +               obj_data = NULL;
> +               p_err("failed to mmap() %s: %s", file, strerror(errno));
> +               goto out;
> +       }
> +       if (obj_name[0] == '\0')
> +               get_obj_name(obj_name, file);
> +       opts.object_name = obj_name;
> +       if (verifier_logs)
> +               /* log_level1 + log_level2 + stats, but not stable UAPI */
> +               opts.kernel_log_level = 1 + 2 + 4;

hm.. we shouldn't need this, we are not loading anything into kernel
and don't use light skeleton

> +       obj = bpf_object__open_mem(obj_data, file_sz, &opts);
> +       err = libbpf_get_error(obj);

no need for libbpf_get_error() anymore, bpftool is in libbpf 1.0 mode,
so it will get NULL on error and error itself will be in errno

> +       if (err) {
> +               char err_buf[256];
> +
> +               libbpf_strerror(err, err_buf, sizeof(err_buf));
> +               p_err("failed to open BPF object file: %s", err_buf);
> +               obj = NULL;
> +               goto out;
> +       }
> +
> +       btf = bpf_object__btf(obj);
> +       err = libbpf_get_error(btf);

same

> +       if (err) {
> +               err = -1;
> +               p_err("need btf type information for %s", obj_name);
> +               goto out;
> +       }
> +
> +       /* First, count how many symbols we have to link. */
> +       bpf_object__for_each_map(map, obj) {
> +               if (!bpf_map__is_internal(map))
> +                       continue;
> +
> +               if (!(bpf_map__map_flags(map) & BPF_F_MMAPABLE))
> +                       continue;
> +
> +               if (!get_map_ident(map, ident, sizeof(ident)))
> +                       continue;
> +
> +               map_type_id = btf__find_by_name_kind(btf, bpf_map__section_name(map), BTF_KIND_DATASEC);

if we set obj_name to "", bpf_map__name() should return ELF section
name here, so no need to expose this as an API


oh, but also bpf_map__btf_value_type_id() should give you this ID directly


> +               if (map_type_id < 0) {
> +                       err = map_type_id;
> +                       goto out;
> +               }
> +               map_type = btf__type_by_id(btf, map_type_id);
> +
> +               for (i = 0, var = btf_var_secinfos(map_type), len = btf_vlen(map_type);
> +                    i < len;
> +                    i++, var++) {

nit: move those long one-time assignments out of the for() loop and
keep it single-line?

> +                       sym_sz++;
> +               }
> +       }
> +
> +       get_header_guard(header_guard, obj_name);
> +       codegen("\
> +       \n\
> +       /* SPDX-License-Identifier: (LGPL-2.1 OR BSD-2-Clause) */   \n\
> +                                                                   \n\
> +       /* THIS FILE IS AUTOGENERATED! */                           \n\
> +       #ifndef %2$s                                                \n\
> +       #define %2$s                                                \n\
> +                                                                   \n\
> +       #include <errno.h>                                          \n\
> +       #include <stdlib.h>                                         \n\
> +       #include <bpf/libbpf.h>                                     \n\
> +                                                                   \n\
> +       struct %1$s {                                               \n\
> +               struct bpf_object *obj;                             \n\
> +               struct bpf_object_subskeleton *subskel;             \n\
> +       ", obj_name, header_guard);
> +
> +       err = codegen_datasecs(obj, obj_name, true);
> +       if (err)
> +               goto out;
> +
> +       /* emit code that will allocate enough storage for all symbols */
> +       codegen("\
> +               \n\
> +                                                                           \n\
> +               #ifdef __cplusplus                                          \n\
> +                       static inline struct %1$s *open(const struct bpf_object *src);\n\
> +                       static inline void %1$s::destroy(struct %1$s *skel);\n\
> +               #endif /* __cplusplus */                                    \n\
> +               };                                                          \n\
> +                                                                           \n\
> +               static inline void                                          \n\
> +               %1$s__destroy(struct %1$s *skel)                            \n\
> +               {                                                           \n\
> +                       if (!skel)                                          \n\
> +                               return;                                     \n\
> +                       if (skel->subskel)                                  \n\
> +                               bpf_object__destroy_subskeleton(skel->subskel);\n\
> +                       free(skel);                                         \n\
> +               }                                                           \n\
> +                                                                           \n\
> +               static inline struct %1$s *                                 \n\
> +               %1$s__open(const struct bpf_object *src)                    \n\
> +               {                                                           \n\
> +                       struct %1$s *obj;                                   \n\
> +                       struct bpf_object_subskeleton *subskel;             \n\
> +                       struct bpf_sym_skeleton *syms;                      \n\
> +                       int err;                                            \n\
> +                                                                           \n\
> +                       obj = (struct %1$s *)calloc(1, sizeof(*obj));       \n\
> +                       if (!obj) {                                         \n\
> +                               errno = ENOMEM;                             \n\
> +                               return NULL;                                \n\
> +                       }                                                   \n\
> +                       subskel = (struct bpf_object_subskeleton *)calloc(1, sizeof(*subskel));\n\
> +                       if (!subskel) {                                     \n\
> +                               errno = ENOMEM;                             \n\
> +                               return NULL;                                \n\

leaking obj here

> +                       }                                                   \n\
> +                       subskel->sz = sizeof(*subskel);                     \n\
> +                       subskel->obj = src;                                 \n\
> +                       subskel->sym_skel_sz = sizeof(struct bpf_sym_skeleton); \n\
> +                       subskel->sym_cnt = %2$d;                            \n\
> +                       obj->subskel = subskel;                             \n\
> +                                                                           \n\
> +                       syms = (struct bpf_sym_skeleton *)calloc(%2$d, sizeof(*syms));\n\
> +                       if (!syms) {                                        \n\
> +                               free(subskel);                              \n\
> +                               errno = ENOMEM;                             \n\
> +                               return NULL;                                \n\

same, obj is leaked


> +                       }                                                   \n\
> +                       subskel->syms = syms;                               \n\
> +                                                                           \n\
> +               ",
> +               obj_name, sym_sz
> +       );
> +
> +       /* walk through each symbol and emit the runtime representation
> +        */

nit: keep this comment single-lined?

> +       bpf_object__for_each_map(map, obj) {
> +               if (!bpf_map__is_internal(map))
> +                       continue;
> +
> +               if (!(bpf_map__map_flags(map) & BPF_F_MMAPABLE))
> +                       continue;
> +
> +               if (!get_map_ident(map, ident, sizeof(ident)))
> +                       continue;

this sequence of ifs seems so frequently repeated that it's probably
time to add a helper function?

> +
> +               map_type_id = btf__find_by_name_kind(btf, bpf_map__section_name(map), BTF_KIND_DATASEC);
> +               if (map_type_id < 0) {
> +                       err = map_type_id;
> +                       goto out;
> +               }
> +               map_type = btf__type_by_id(btf, map_type_id);
> +
> +               for (i = 0, var = btf_var_secinfos(map_type), len = btf_vlen(map_type);
> +                    i < len;
> +                    i++, var++, sym_idx++) {

similar as above, var and len assignment doesn't have to inside the for

> +                       var_type = btf__type_by_id(btf, var->type);
> +                       var_name = btf__name_by_offset(btf, var_type->name_off);
> +
> +                       var_ident[0] = '\0';
> +                       strncat(var_ident, var_name, sizeof(var_ident) - 1);
> +                       sanitize_identifier(var_ident);
> +
> +                       codegen("\
> +                       \n\
> +                               syms[%4$d].name = \"%1$s\";                 \n\
> +                               syms[%4$d].section = \"%3$s\";              \n\
> +                               syms[%4$d].addr = (void**) &obj->%2$s.%1$s; \n\
> +                       ", var_ident, ident, bpf_map__section_name(map), sym_idx);
> +               }
> +       }

why not assign subskel->sym_cnt here using sym_idx and avoid that
extra loop over all variables above?


> +
> +       codegen("\
> +               \n\
> +                                                                           \n\
> +                       err = bpf_object__open_subskeleton(subskel);        \n\
> +                       if (err) {                                          \n\
> +                               %1$s__destroy(obj);                         \n\
> +                               errno = err;                                \n\
> +                               return NULL;                                \n\
> +                       }                                                   \n\
> +                                                                           \n\
> +                       return obj;                                         \n\
> +               }                                                           \n\
> +               ",
> +               obj_name);
> +
> +       codegen("\
> +               \n\
> +                                                                           \n\
> +               #ifdef __cplusplus                                          \n\
> +               struct %1$s *%1$s::open(const struct bpf_object *src) { return %1$s__open(src); }\n\
> +               void %1$s::destroy(struct %1$s *skel) { %1$s__destroy(skel); }\n\
> +               #endif /* __cplusplus */                                    \n\
> +                                                                           \n\
> +               #endif /* %2$s */                                           \n\
> +               ",
> +               obj_name, header_guard);
> +       err = 0;
> +out:
> +       bpf_object__close(obj);
> +       if (obj_data)
> +               munmap(obj_data, mmap_sz);
> +       close(fd);
> +       return err;
> +}
> +
>  static int do_object(int argc, char **argv)
>  {
>         struct bpf_linker *linker;
> @@ -1192,6 +1494,7 @@ static int do_help(int argc, char **argv)
>         fprintf(stderr,
>                 "Usage: %1$s %2$s object OUTPUT_FILE INPUT_FILE [INPUT_FILE...]\n"
>                 "       %1$s %2$s skeleton FILE [name OBJECT_NAME]\n"
> +               "       %1$s %2$s subskeleton FILE [name OBJECT_NAME]\n"

Quentin will remind you that you should also update the man page and
bash completion script :)

>                 "       %1$s %2$s min_core_btf INPUT OUTPUT OBJECT [OBJECT...]\n"
>                 "       %1$s %2$s help\n"
>                 "\n"
> @@ -1788,6 +2091,7 @@ static int do_min_core_btf(int argc, char **argv)
>  static const struct cmd cmds[] = {
>         { "object",             do_object },
>         { "skeleton",           do_skeleton },
> +       { "subskeleton",        do_subskeleton },
>         { "min_core_btf",       do_min_core_btf},
>         { "help",               do_help },
>         { 0 }
> --
> 2.34.1

Re: [PATCH bpf-next 2/4] bpftool: add support for subskeletons

[Delyan Kratunov]

Thanks Andrii for taking a thorough look!

On Wed, 2022-03-02 at 17:46 -0800, Andrii Nakryiko wrote:
> There is a need (I needed it in libusdt, for example). Let's add it
> from the very beginning, especially that it can be done in *exactly*
> the same form as for skeletons.

Absolutely, I'll get that into the next version!

> seems like subskel's datasec codegen is considerably different (and
> simpler, really) than skeleton's, I'd add a separate function for it
> instead of all this if (subskel) special-casing. Main concern for
> skeleton is generating correct memory layout, while for subskel we
> just need to generate a list of pointers without any regard to memory
> layout.

I'm not 100% convinced given how much code would end up being duplicated but I
can go in that direction, if you'd prefer it.

> 

> it's unfortunate to have to modify original BTF just to have this '*'
> added.  If I remember correctly, C syntax for pointers has special
> case only for arrays and func prototypes, so it should work with this
> logic (not tested, obviously)
> 
> 1. if top variable type is array or func_proto, set var_ident to "(*<variable>)"
> 2. otherwise set var_ident to "*<variable>"
> 
> we'd need to test it for array and func_proto, but it definitely
> should work for all other cases

A couple of thoughts here:

1. We are not modifing the original BTF, we are layering in-memory-only types on
top of it. This ends up working transparently through btf_dump code, which is
the source of truth of what "correct" is. I think this is strictly better than
the alternative textual modifications to var_ident.

2. I guess we see the change differently - to me it's not just about adding an
asterisk but instead working with derivative types. This might come in handy in
other contexts that we haven't envisioned yet and I feel is a direction worth
supporting overall.

3. We have a full type system with layering and mixed file- and memory-based
storage. Why limit ourselves to templating instead of using it in the codegen?
If I were writing this from scratch, much of codegen_datasecs would instead
create in-memory BTF types and have btf_dump emit them (but that's not the
bikeshed I want to paint here!).

> > +       char header_guard[MAX_OBJ_NAME_LEN + sizeof("__SKEL_H__")];
> 
> use __SUBSKEL_H__ here?

Sure, I can introduce the .subskel.h suffix everywhere.

> 
> > +                       strncpy(obj_name, *argv, MAX_OBJ_NAME_LEN - 1);
> > +                       obj_name[MAX_OBJ_NAME_LEN - 1] = '\0';
> 
> we should probably force obj_name to be an empty string, so that all
> map names match their proper section names

Ah, maybe this is why bpf_map__name was not doing the right thing before. I
don't really like that we're relying on side effects of the empty obj_name but
I'll try it and see if anything breaks (the templating for one will need it
anyway).

> 
> > +       if (verifier_logs)
> > +               /* log_level1 + log_level2 + stats, but not stable UAPI */
> > +               opts.kernel_log_level = 1 + 2 + 4;
> 
> hm.. we shouldn't need this, we are not loading anything into kernel
> and don't use light skeleton

You're right, will remove.

> 
> > +       obj = bpf_object__open_mem(obj_data, file_sz, &opts);
> > +       err = libbpf_get_error(obj);
> 
> no need for libbpf_get_error() anymore, bpftool is in libbpf 1.0 mode,
> so it will get NULL on error and error itself will be in errno

Ah, yes, I won't add new callsites.

> 
> > 
> > +               map_type_id = btf__find_by_name_kind(btf, bpf_map__section_name(map), BTF_KIND_DATASEC);
> 
> if we set obj_name to "", bpf_map__name() should return ELF section
> name here, so no need to expose this as an API
> 
> 
> oh, but also bpf_map__btf_value_type_id() should give you this ID directly

TIL, that's not obvious at all. There's a few places in gen.c that could be
simplified then - find_type_for_map goes through slicing the complete name and
walking over every BTF type to match on the slice. Is there some compatibility
reason to do that or is btf_value_type_id always there?

> 
> > +               for (i = 0, var = btf_var_secinfos(map_type), len = btf_vlen(map_type);
> > +                    i < len;
> > +                    i++, var++) {
> 
> nit: move those long one-time assignments out of the for() loop and
> keep it single-line?

Yeah, I hate that structure too, I'll clean it up.

> 
> > 
> > +                       if (!subskel) {                                     \n\
> > +                               errno = ENOMEM;                             \n\
> > +                               return NULL;                                \n\
> 
> leaking obj here

Yeah, I noticed that I didn't use __destroy in the subskel, will fix for v2.


> > +       /* walk through each symbol and emit the runtime representation
> > +        */
> 
> nit: keep this comment single-lined?

I did initially and checkpatch scolded me :) 

> 
> > +       bpf_object__for_each_map(map, obj) {
> > +               if (!bpf_map__is_internal(map))
> > +                       continue;
> > +
> > +               if (!(bpf_map__map_flags(map) & BPF_F_MMAPABLE))
> > +                       continue;
> > +
> > +               if (!get_map_ident(map, ident, sizeof(ident)))
> > +                       continue;
> 
> this sequence of ifs seems so frequently repeated that it's probably
> time to add a helper function?

It is and it's annoying me too. I'll look at the whole iteration pattern more
closely.

> 
> 
> > +                       codegen("\
> > +                       \n\
> > +                               syms[%4$d].name = \"%1$s\";                 \n\
> > +                               syms[%4$d].section = \"%3$s\";              \n\
> > +                               syms[%4$d].addr = (void**) &obj->%2$s.%1$s; \n\
> > +                       ", var_ident, ident, bpf_map__section_name(map), sym_idx);
> > +               }
> > +       }
> 
> why not assign subskel->sym_cnt here using sym_idx and avoid that
> extra loop over all variables above?

Good call, will do.
> 
> Quentin will remind you that you should also update the man page and
> bash completion script :)

Ah, yes, glad to see it's rst and I don't have to suffer groff/mdoc flashbacks!

Thanks,
Delyan

Re: [PATCH bpf-next 2/4] bpftool: add support for subskeletons

[Delyan Kratunov]

On Thu, 2022-03-03 at 10:52 -0800, Delyan Kratunov wrote:
> > 
> > > 
> > > +               map_type_id = btf__find_by_name_kind(btf,
> > > bpf_map__section_name(map), BTF_KIND_DATASEC);
> > 
> > if we set obj_name to "", bpf_map__name() should return ELF section
> > name here, so no need to expose this as an API
> > 
> > 
> > oh, but also bpf_map__btf_value_type_id() should give you this ID directly
> 
> TIL, that's not obvious at all. There's a few places in gen.c that could be
> simplified then - find_type_for_map goes through slicing the complete name and
> walking over every BTF type to match on the slice. Is there some compatibility
> reason to do that or is btf_value_type_id always there?

Unfortunately, the internal datasec maps have value_type_id = key_value_type_id
= 0 i.e. void, so bpf_map__btf_value_type_id won't work out of the box.

I haven't looked if that's a bug all the way in clang-emitted object or
somewhere further on.

-- Delyan

Re: [PATCH bpf-next 2/4] bpftool: add support for subskeletons

[Andrii Nakryiko]

On Thu, Mar 3, 2022 at 12:54 PM Delyan Kratunov <delyank@fb.com> wrote:
>
> On Thu, 2022-03-03 at 10:52 -0800, Delyan Kratunov wrote:
> > >
> > > >
> > > > +               map_type_id = btf__find_by_name_kind(btf,
> > > > bpf_map__section_name(map), BTF_KIND_DATASEC);
> > >
> > > if we set obj_name to "", bpf_map__name() should return ELF section
> > > name here, so no need to expose this as an API
> > >
> > >
> > > oh, but also bpf_map__btf_value_type_id() should give you this ID directly
> >
> > TIL, that's not obvious at all. There's a few places in gen.c that could be
> > simplified then - find_type_for_map goes through slicing the complete name and
> > walking over every BTF type to match on the slice. Is there some compatibility
> > reason to do that or is btf_value_type_id always there?
>
> Unfortunately, the internal datasec maps have value_type_id = key_value_type_id
> = 0 i.e. void, so bpf_map__btf_value_type_id won't work out of the box.
>
> I haven't looked if that's a bug all the way in clang-emitted object or
> somewhere further on.

Yeah, just replied in another email. We fill it out too late, but it
should be possible to move it earlier into open phase.

>
> -- Delyan

Re: [PATCH bpf-next 2/4] bpftool: add support for subskeletons

[Andrii Nakryiko]

On Thu, Mar 3, 2022 at 10:57 AM Delyan Kratunov <delyank@fb.com> wrote:
>
> Thanks Andrii for taking a thorough look!
>
> On Wed, 2022-03-02 at 17:46 -0800, Andrii Nakryiko wrote:
> > There is a need (I needed it in libusdt, for example). Let's add it
> > from the very beginning, especially that it can be done in *exactly*
> > the same form as for skeletons.
>
> Absolutely, I'll get that into the next version!
>
> > seems like subskel's datasec codegen is considerably different (and
> > simpler, really) than skeleton's, I'd add a separate function for it
> > instead of all this if (subskel) special-casing. Main concern for
> > skeleton is generating correct memory layout, while for subskel we
> > just need to generate a list of pointers without any regard to memory
> > layout.
>
> I'm not 100% convinced given how much code would end up being duplicated but I
> can go in that direction, if you'd prefer it.
>
> >
>
> > it's unfortunate to have to modify original BTF just to have this '*'
> > added.  If I remember correctly, C syntax for pointers has special
> > case only for arrays and func prototypes, so it should work with this
> > logic (not tested, obviously)
> >
> > 1. if top variable type is array or func_proto, set var_ident to "(*<variable>)"
> > 2. otherwise set var_ident to "*<variable>"
> >
> > we'd need to test it for array and func_proto, but it definitely
> > should work for all other cases
>
> A couple of thoughts here:
>
> 1. We are not modifing the original BTF, we are layering in-memory-only types on
> top of it. This ends up working transparently through btf_dump code, which is
> the source of truth of what "correct" is. I think this is strictly better than
> the alternative textual modifications to var_ident.

Yeah, I noticed that you are creating split BTF later. Ok, I don't
mind, let's do it this way (due to the horrible pointer-to-array
syntax inconsistency). Please leave the comment somewhere here to make
it obvious that we are not modifying original BTF

>
> 2. I guess we see the change differently - to me it's not just about adding an
> asterisk but instead working with derivative types. This might come in handy in
> other contexts that we haven't envisioned yet and I feel is a direction worth
> supporting overall.

It's not "just about adding an asterisk", it's about generating a
pointer to the type. Split BTF added on top makes it a bit more
tolerable (though there is still a bunch of unnecessary complexity and
overhead just for that pesky asterisk).

Another alternative would be:

typeof(char[123]) *my_ptr;

This can be done without generating extra BTF. For complex types it's
actually even easier to parse, tbh. I initially didn't like it, but
now I'm thinking maybe for arrays and func_protos we should do just
this? WDYT?

>
> 3. We have a full type system with layering and mixed file- and memory-based
> storage. Why limit ourselves to templating instead of using it in the codegen?
> If I were writing this from scratch, much of codegen_datasecs would instead
> create in-memory BTF types and have btf_dump emit them (but that's not the
> bikeshed I want to paint here!).

Maybe, but at the time this code was written we didn't have either
split BTF nor BTF writing APIs.

>
> > > +       char header_guard[MAX_OBJ_NAME_LEN + sizeof("__SKEL_H__")];
> >
> > use __SUBSKEL_H__ here?
>
> Sure, I can introduce the .subskel.h suffix everywhere.
>
> >
> > > +                       strncpy(obj_name, *argv, MAX_OBJ_NAME_LEN - 1);
> > > +                       obj_name[MAX_OBJ_NAME_LEN - 1] = '\0';
> >
> > we should probably force obj_name to be an empty string, so that all
> > map names match their proper section names
>
> Ah, maybe this is why bpf_map__name was not doing the right thing before. I
> don't really like that we're relying on side effects of the empty obj_name but
> I'll try it and see if anything breaks (the templating for one will need it
> anyway).

Yeah, it's due to the fact that we are using "object name" as part of
.data, .rodata, and .bss maps (and .kconfig, probably). Historic
decision, too bad, but we have what we have. We probably should clean
this up in libbpf 1.0, it's too confusing throughout.

>
> >
> > > +       if (verifier_logs)
> > > +               /* log_level1 + log_level2 + stats, but not stable UAPI */
> > > +               opts.kernel_log_level = 1 + 2 + 4;
> >
> > hm.. we shouldn't need this, we are not loading anything into kernel
> > and don't use light skeleton
>
> You're right, will remove.
>
> >
> > > +       obj = bpf_object__open_mem(obj_data, file_sz, &opts);
> > > +       err = libbpf_get_error(obj);
> >
> > no need for libbpf_get_error() anymore, bpftool is in libbpf 1.0 mode,
> > so it will get NULL on error and error itself will be in errno
>
> Ah, yes, I won't add new callsites.
>
> >
> > >
> > > +               map_type_id = btf__find_by_name_kind(btf, bpf_map__section_name(map), BTF_KIND_DATASEC);
> >
> > if we set obj_name to "", bpf_map__name() should return ELF section
> > name here, so no need to expose this as an API
> >
> >
> > oh, but also bpf_map__btf_value_type_id() should give you this ID directly
>
> TIL, that's not obvious at all. There's a few places in gen.c that could be
> simplified then - find_type_for_map goes through slicing the complete name and
> walking over every BTF type to match on the slice. Is there some compatibility
> reason to do that or is btf_value_type_id always there?

No legacy reason, we should use btf_value_type_id if
necessary/possible (it's going to be DATASEC type for all global var
maps, I think). But I just double checked and it seems that we fill
out btf_value_type_id only during map creation (that is, during
bpf_object__load()). But I don't see any reason to postpone it so
late, so let's just move it to the open phase. See
bpf_map_find_btf_info() and where it's called.

>
> >
> > > +               for (i = 0, var = btf_var_secinfos(map_type), len = btf_vlen(map_type);
> > > +                    i < len;
> > > +                    i++, var++) {
> >
> > nit: move those long one-time assignments out of the for() loop and
> > keep it single-line?
>
> Yeah, I hate that structure too, I'll clean it up.
>
> >
> > >
> > > +                       if (!subskel) {                                     \n\
> > > +                               errno = ENOMEM;                             \n\
> > > +                               return NULL;                                \n\
> >
> > leaking obj here
>
> Yeah, I noticed that I didn't use __destroy in the subskel, will fix for v2.
>
>
> > > +       /* walk through each symbol and emit the runtime representation
> > > +        */
> >
> > nit: keep this comment single-lined?
>
> I did initially and checkpatch scolded me :)

checkpatch still loves 80 character lines, but it was relaxed to 100 a
while ago. Checkpatch.pl is not an authority, it's just a suggestion
for a lot of cases.

>
> >
> > > +       bpf_object__for_each_map(map, obj) {
> > > +               if (!bpf_map__is_internal(map))
> > > +                       continue;
> > > +
> > > +               if (!(bpf_map__map_flags(map) & BPF_F_MMAPABLE))
> > > +                       continue;
> > > +
> > > +               if (!get_map_ident(map, ident, sizeof(ident)))
> > > +                       continue;
> >
> > this sequence of ifs seems so frequently repeated that it's probably
> > time to add a helper function?
>
> It is and it's annoying me too. I'll look at the whole iteration pattern more
> closely.
>
> >
> >
> > > +                       codegen("\
> > > +                       \n\
> > > +                               syms[%4$d].name = \"%1$s\";                 \n\
> > > +                               syms[%4$d].section = \"%3$s\";              \n\
> > > +                               syms[%4$d].addr = (void**) &obj->%2$s.%1$s; \n\
> > > +                       ", var_ident, ident, bpf_map__section_name(map), sym_idx);
> > > +               }
> > > +       }
> >
> > why not assign subskel->sym_cnt here using sym_idx and avoid that
> > extra loop over all variables above?
>
> Good call, will do.
> >
> > Quentin will remind you that you should also update the man page and
> > bash completion script :)
>
> Ah, yes, glad to see it's rst and I don't have to suffer groff/mdoc flashbacks!
>
> Thanks,
> Delyan
>

Re: [PATCH bpf-next 2/4] bpftool: add support for subskeletons

[Delyan Kratunov]

Sorry, missed this question originally.

On Fri, 2022-03-04 at 11:29 -0800, Andrii Nakryiko wrote:
> Split BTF added on top makes it a bit more
> tolerable (though there is still a bunch of unnecessary complexity and
> overhead just for that pesky asterisk).
> 
> Another alternative would be:
> 
> typeof(char[123]) *my_ptr;
> 
> This can be done without generating extra BTF. For complex types it's
> actually even easier to parse, tbh. I initially didn't like it, but
> now I'm thinking maybe for arrays and func_protos we should do just
> this? WDYT?

typeof is _technically_ not standard C (I think it'll make it into C23). GCC and
Clang do both support it but I would guess we'd want the generated userspace
code to be compatible with as many toolchains and configurations as possible?
(e.g. people using c99 instead of gnu99)

Beyond that, I feel that any complexity saved by the typeof is lost in special-
casing arrays and function prototypes when the current code is uniform across
all types.

If you insist, I can go down this path but I'm not super enthusiastic about it
(and it's harder to read on top of everything).

-- Delyan

Re: [PATCH bpf-next 2/4] bpftool: add support for subskeletons

[Andrii Nakryiko]

On Wed, Mar 9, 2022 at 4:10 PM Delyan Kratunov <delyank@fb.com> wrote:
>
> Sorry, missed this question originally.
>
> On Fri, 2022-03-04 at 11:29 -0800, Andrii Nakryiko wrote:
> > Split BTF added on top makes it a bit more
> > tolerable (though there is still a bunch of unnecessary complexity and
> > overhead just for that pesky asterisk).
> >
> > Another alternative would be:
> >
> > typeof(char[123]) *my_ptr;
> >
> > This can be done without generating extra BTF. For complex types it's
> > actually even easier to parse, tbh. I initially didn't like it, but
> > now I'm thinking maybe for arrays and func_protos we should do just
> > this? WDYT?
>
> typeof is _technically_ not standard C (I think it'll make it into C23). GCC and
> Clang do both support it but I would guess we'd want the generated userspace
> code to be compatible with as many toolchains and configurations as possible?
> (e.g. people using c99 instead of gnu99)

that ship has sailed, I'm afraid. btf.h and xsk.h (I'm not even
talking about BPF-side headers) both use typeof()

>
> Beyond that, I feel that any complexity saved by the typeof is lost in special-
> casing arrays and function prototypes when the current code is uniform across
> all types.
>
> If you insist, I can go down this path but I'm not super enthusiastic about it
> (and it's harder to read on top of everything).

I can tell you are not :)

I do think that typeof() makes it much easier to follow complicated
cases. Just look at the example below:

static int blah_fn(int x, int y) { return x + y; }

struct S {
        typeof(char[24]) *my_ptr1;
        char (*my_ptr2)[24];

        int (**my_func1)(int, int);
        typeof(int (*)(int, int)) *my_func2;
};


int main() {
        static struct S s;
        char blah[24];
        int (*fptr)(int, int);

        s.my_ptr1 = &blah;
        s.my_ptr2 = &blah;

        s.my_func1 = &fptr;
        s.my_func2 = &fptr;

        return 0;
}


Which one is easier to follow, my_ptr1 or my_ptr2? func pointer is a
bit more hypothetical (hard to come up with realistic BPF program that
would need func pointer type as global variable type), but I still did
it for completeness.


So it's not like I'm dead set against this split BTF approach, I just
do really think that array variable case is better with typeof().

>
> -- Delyan