| Message ID | 20190611043505.14664-5-andriin@fb.com |
|---|---|
| State | RFC |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | BTF-defined BPF map definitions | expand |
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 >
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 > >
> 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>
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); } long libbpf_get_error(const void *ptr)
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(-)