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

From: Song Liu <songliubraving@fb.com>
To: Andrii Nakryiko <andrii.nakryiko@gmail.com>
Cc: Song Liu <liu.song.a23@gmail.com>,
	Andrii Nakryiko <andriin@fb.com>, bpf <bpf@vger.kernel.org>,
	Networking <netdev@vger.kernel.org>,
	"Alexei Starovoitov" <ast@fb.com>,
	Daniel Borkmann <daniel@iogearbox.net>,
	"Kernel Team" <Kernel-team@fb.com>
Subject: Re: [RFC PATCH bpf-next 4/8] libbpf: identify maps by section index in addition to offset
Date: Mon, 17 Jun 2019 18:15:30 +0000	[thread overview]
Message-ID: <4B62AA63-EA75-4C8B-B225-47930BC0812A@fb.com> (raw)
In-Reply-To: <CAEf4BzYKtA9Hk5oswZVD_pZ-VxjXXd_OV_bRs+42cfgf8dqodw@mail.gmail.com>

> On Jun 17, 2019, at 11:06 AM, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
> 
> 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.

Good survey of the use cases! Yeah, I agree returning -ENOTSUP is the 
best option here. 

Acked-by: Song Liu <songliubraving@fb.com>