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

From: Song Liu <liu.song.a23@gmail.com>
To: Andrii Nakryiko <andriin@fb.com>
Cc: Andrii Nakryiko <andrii.nakryiko@gmail.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: Sat, 15 Jun 2019 14:07:53 -0700	[thread overview]
Message-ID: <CAPhsuW6iicoRN3Sk6Uv-ten4xjjmqG1qmfmXyKngqVSYC9qbEQ@mail.gmail.com> (raw)
In-Reply-To: <20190611043505.14664-5-andriin@fb.com>

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(-)
>
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index c931ee7e1fd2..5e7ea7dac958 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -207,7 +207,8 @@ static const char * const libbpf_type_to_btf_name[] = {
>  struct bpf_map {
>         int fd;
>         char *name;
> -       size_t offset;
> +       int sec_idx;
> +       size_t sec_offset;
>         int map_ifindex;
>         int inner_map_fd;
>         struct bpf_map_def def;
> @@ -647,7 +648,9 @@ static int compare_bpf_map(const void *_a, const void *_b)
>         const struct bpf_map *a = _a;
>         const struct bpf_map *b = _b;
>
> -       return a->offset - b->offset;
> +       if (a->sec_idx != b->sec_idx)
> +               return a->sec_idx - b->sec_idx;
> +       return a->sec_offset - b->sec_offset;
>  }
>
>  static bool bpf_map_type__is_map_in_map(enum bpf_map_type type)
> @@ -800,14 +803,15 @@ static struct bpf_map *bpf_object__add_map(struct bpf_object *obj)
>
>  static int
>  bpf_object__init_internal_map(struct bpf_object *obj, struct bpf_map *map,
> -                             enum libbpf_map_type type, Elf_Data *data,
> -                             void **data_buff)
> +                             enum libbpf_map_type type, int sec_idx,
> +                             Elf_Data *data, void **data_buff)
>  {
>         struct bpf_map_def *def = &map->def;
>         char map_name[BPF_OBJ_NAME_LEN];
>
>         map->libbpf_type = type;
> -       map->offset = ~(typeof(map->offset))0;
> +       map->sec_idx = sec_idx;
> +       map->sec_offset = 0;
>         snprintf(map_name, sizeof(map_name), "%.8s%.7s", obj->name,
>                  libbpf_type_to_btf_name[type]);
>         map->name = strdup(map_name);
> @@ -815,6 +819,8 @@ bpf_object__init_internal_map(struct bpf_object *obj, struct bpf_map *map,
>                 pr_warning("failed to alloc map name\n");
>                 return -ENOMEM;
>         }
> +       pr_debug("map '%s' (global data): at sec_idx %d, offset %zu.\n",
> +                map_name, map->sec_idx, map->sec_offset);
>
>         def->type = BPF_MAP_TYPE_ARRAY;
>         def->key_size = sizeof(int);
> @@ -850,6 +856,7 @@ static int bpf_object__init_global_data_maps(struct bpf_object *obj)
>                 if (IS_ERR(map))
>                         return PTR_ERR(map);
>                 err = bpf_object__init_internal_map(obj, map, LIBBPF_MAP_DATA,
> +                                                   obj->efile.data_shndx,
>                                                     obj->efile.data,
>                                                     &obj->sections.data);
>                 if (err)
> @@ -860,6 +867,7 @@ static int bpf_object__init_global_data_maps(struct bpf_object *obj)
>                 if (IS_ERR(map))
>                         return PTR_ERR(map);
>                 err = bpf_object__init_internal_map(obj, map, LIBBPF_MAP_RODATA,
> +                                                   obj->efile.rodata_shndx,
>                                                     obj->efile.rodata,
>                                                     &obj->sections.rodata);
>                 if (err)
> @@ -870,6 +878,7 @@ static int bpf_object__init_global_data_maps(struct bpf_object *obj)
>                 if (IS_ERR(map))
>                         return PTR_ERR(map);
>                 err = bpf_object__init_internal_map(obj, map, LIBBPF_MAP_BSS,
> +                                                   obj->efile.bss_shndx,
>                                                     obj->efile.bss, NULL);
>                 if (err)
>                         return err;
> @@ -953,7 +962,10 @@ static int bpf_object__init_user_maps(struct bpf_object *obj, bool strict)
>                 }
>
>                 map->libbpf_type = LIBBPF_MAP_UNSPEC;
> -               map->offset = sym.st_value;
> +               map->sec_idx = sym.st_shndx;
> +               map->sec_offset = sym.st_value;
> +               pr_debug("map '%s' (legacy): at sec_idx %d, offset %zu.\n",
> +                        map_name, map->sec_idx, map->sec_offset);
>                 if (sym.st_value + map_def_sz > data->d_size) {
>                         pr_warning("corrupted maps section in %s: last map \"%s\" too small\n",
>                                    obj->path, map_name);
> @@ -1453,9 +1465,13 @@ bpf_program__collect_reloc(struct bpf_program *prog, GElf_Shdr *shdr,
>                                 if (maps[map_idx].libbpf_type != type)
>                                         continue;
>                                 if (type != LIBBPF_MAP_UNSPEC ||
> -                                   maps[map_idx].offset == sym.st_value) {
> -                                       pr_debug("relocation: find map %zd (%s) for insn %u\n",
> -                                                map_idx, maps[map_idx].name, insn_idx);
> +                                   (maps[map_idx].sec_idx == sym.st_shndx &&
> +                                    maps[map_idx].sec_offset == sym.st_value)) {
> +                                       pr_debug("relocation: found map %zd (%s, sec_idx %d, offset %zu) for insn %u\n",
> +                                                map_idx, maps[map_idx].name,
> +                                                maps[map_idx].sec_idx,
> +                                                maps[map_idx].sec_offset,
> +                                                insn_idx);
>                                         break;
>                                 }
>                         }
> @@ -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?

Thanks,
Song

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