Re: [RFC PATCH bpf-next 4/8] libbpf: identify maps by section index in addition to offset

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

On Sat, Jun 15, 2019 at 2:08 PM Song Liu <liu.song.a23@gmail.com> wrote:
>
> On Mon, Jun 10, 2019 at 9:37 PM Andrii Nakryiko <andriin@fb.com> wrote:
> >
> > To support maps to be defined in multiple sections, it's important to
> > identify map not just by offset within its section, but section index as
> > well. This patch adds tracking of section index.
> >
> > For global data, we record section index of corresponding
> > .data/.bss/.rodata ELF section for uniformity, and thus don't need
> > a special value of offset for those maps.
> >
> > Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> > ---
> >  tools/lib/bpf/libbpf.c | 42 ++++++++++++++++++++++++++----------------
> >  1 file changed, 26 insertions(+), 16 deletions(-)
> >

<snip>

> > @@ -3472,13 +3488,7 @@ bpf_object__find_map_fd_by_name(struct bpf_object *obj, const char *name)
> >  struct bpf_map *
> >  bpf_object__find_map_by_offset(struct bpf_object *obj, size_t offset)
> >  {
> > -       int i;
> > -
> > -       for (i = 0; i < obj->nr_maps; i++) {
> > -               if (obj->maps[i].offset == offset)
> > -                       return &obj->maps[i];
> > -       }
> > -       return ERR_PTR(-ENOENT);
> > +       return ERR_PTR(-ENOTSUP);
>
> I probably missed some discussion. But is it OK to stop supporting
> this function?

This function was added long time ago for some perf (the tool)
specific use case. But I haven't found any uses of that in kernel
code, as well as anywhere on github/internal FB code base. It appears
it's not used anywhere.

Also, this function makes bad assumption that map can be identified by
single offset, while we are going to support maps in two (or more, if
necessary) different ELF sections, so offset is not unique anymore.
It's not clear what's the intended use case for this API is, looking
up by name should be the way to do this. Given it's not used, but we
still need to preserve ABI, I switched it to return -ENOTSUP.

>
> Thanks,
> Song
>
> >  }
> >
> >  long libbpf_get_error(const void *ptr)
> > --
> > 2.17.1
> >