On Wed, Mar 2, 2022 at 8:33 PM Andrii Nakryiko
<andrii.nakryiko@xxxxxxxxx> wrote:
>
> On Tue, Mar 1, 2022 at 6:49 PM Delyan Kratunov <delyank@xxxxxx> 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@xxxxxx>
> > ---
> > 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
