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

[Andrii Nakryiko <andrii.nakryiko@gmail.com>]

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