From: Andrii Nakryiko <andrii.nakryiko@gmail.com>
To: Matt Hart <matthew.hart@linaro.org>
Cc: Andrii Nakryiko <andriin@fb.com>,
linux-perf-users@vger.kernel.org, Alexei Starovoitov <ast@fb.com>,
Daniel Borkmann <daniel@iogearbox.net>,
Networking <netdev@vger.kernel.org>, bpf <bpf@vger.kernel.org>,
Kernel Team <kernel-team@fb.com>
Subject: Re: [PATCH v2 bpf-next 04/11] libbpf: refactor map initialization
Date: Wed, 26 Jun 2019 11:28:50 -0700 [thread overview]
Message-ID: <CAEf4BzY8wMtq9EmUcsqeLrNOkduKwzuYrweKhhVWKdAb57ax+Q@mail.gmail.com> (raw)
In-Reply-To: <CAH+k93H5ddp7cMBGtyn9DNPvv1MymGCsShJ1-3tezLPUnxt6vw@mail.gmail.com>
On Wed, Jun 26, 2019 at 7:48 AM Matt Hart <matthew.hart@linaro.org> wrote:
>
> Hi all,
>
> I noticed perf fails to build for armv7 on linux next, due to this
> compile error:
> $ make -C tools/perf ARCH=arm CROSS_COMPILE=arm-linux-gnueabihf-
>
> CC libbpf_probes.o
> In file included from libbpf.c:27:
> libbpf.c: In function ‘bpf_object__add_map’:
> /home/matt/git/linux-next/tools/include/linux/kernel.h:45:17: error:
> comparison of distinct pointer types lacks a cast [-Werror]
> (void) (&_max1 == &_max2); \
> ^~
> libbpf.c:776:12: note: in expansion of macro ‘max’
> new_cap = max(4ul, obj->maps_cap * 3 / 2);
> ^~~
>
> So I bisected it and came down to this patch.
> Commit bf82927125dd25003d76ed5541da704df21de57a
>
> Full verbose bisect script https://hastebin.com/odoyujofav.coffeescript
>
> Is this a case that perf code needs updating to match the change, or
> is the change broken?
Hi Matt,
Thanks for reporting. This issue was already fixed in
https://patchwork.ozlabs.org/patch/1122673/, so just pull latest
bpf-next.
>
>
>
> On Tue, 25 Jun 2019 at 16:53, Andrii Nakryiko <andriin@fb.com> wrote:
> >
> > User and global data maps initialization has gotten pretty complicated
> > and unnecessarily convoluted. This patch splits out the logic for global
> > data map and user-defined map initialization. It also removes the
> > restriction of pre-calculating how many maps will be initialized,
> > instead allowing to keep adding new maps as they are discovered, which
> > will be used later for BTF-defined map definitions.
> >
> > Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> > ---
> > tools/lib/bpf/libbpf.c | 247 ++++++++++++++++++++++-------------------
> > 1 file changed, 133 insertions(+), 114 deletions(-)
> >
> > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > index 7ee44d8877c5..88609dca4f7d 100644
> > --- a/tools/lib/bpf/libbpf.c
> > +++ b/tools/lib/bpf/libbpf.c
> > @@ -234,6 +234,7 @@ struct bpf_object {
> > size_t nr_programs;
> > struct bpf_map *maps;
> > size_t nr_maps;
> > + size_t maps_cap;
> > struct bpf_secdata sections;
> >
> > bool loaded;
> > @@ -763,21 +764,51 @@ int bpf_object__variable_offset(const struct bpf_object *obj, const char *name,
> > return -ENOENT;
> > }
> >
> > -static bool bpf_object__has_maps(const struct bpf_object *obj)
> > +static struct bpf_map *bpf_object__add_map(struct bpf_object *obj)
> > {
> > - return obj->efile.maps_shndx >= 0 ||
> > - obj->efile.data_shndx >= 0 ||
> > - obj->efile.rodata_shndx >= 0 ||
> > - obj->efile.bss_shndx >= 0;
> > + struct bpf_map *new_maps;
> > + size_t new_cap;
> > + int i;
> > +
> > + if (obj->nr_maps < obj->maps_cap)
> > + return &obj->maps[obj->nr_maps++];
> > +
> > + new_cap = max(4ul, obj->maps_cap * 3 / 2);
> > + new_maps = realloc(obj->maps, new_cap * sizeof(*obj->maps));
> > + if (!new_maps) {
> > + pr_warning("alloc maps for object failed\n");
> > + return ERR_PTR(-ENOMEM);
> > + }
> > +
> > + obj->maps_cap = new_cap;
> > + obj->maps = new_maps;
> > +
> > + /* zero out new maps */
> > + memset(obj->maps + obj->nr_maps, 0,
> > + (obj->maps_cap - obj->nr_maps) * sizeof(*obj->maps));
> > + /*
> > + * fill all fd with -1 so won't close incorrect fd (fd=0 is stdin)
> > + * when failure (zclose won't close negative fd)).
> > + */
> > + for (i = obj->nr_maps; i < obj->maps_cap; i++) {
> > + obj->maps[i].fd = -1;
> > + obj->maps[i].inner_map_fd = -1;
> > + }
> > +
> > + return &obj->maps[obj->nr_maps++];
> > }
> >
> > 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)
> > +bpf_object__init_internal_map(struct bpf_object *obj, enum libbpf_map_type type,
> > + Elf_Data *data, void **data_buff)
> > {
> > - struct bpf_map_def *def = &map->def;
> > char map_name[BPF_OBJ_NAME_LEN];
> > + struct bpf_map_def *def;
> > + struct bpf_map *map;
> > +
> > + map = bpf_object__add_map(obj);
> > + if (IS_ERR(map))
> > + return PTR_ERR(map);
> >
> > map->libbpf_type = type;
> > map->offset = ~(typeof(map->offset))0;
> > @@ -789,6 +820,7 @@ bpf_object__init_internal_map(struct bpf_object *obj, struct bpf_map *map,
> > return -ENOMEM;
> > }
> >
> > + def = &map->def;
> > def->type = BPF_MAP_TYPE_ARRAY;
> > def->key_size = sizeof(int);
> > def->value_size = data->d_size;
> > @@ -808,29 +840,58 @@ bpf_object__init_internal_map(struct bpf_object *obj, struct bpf_map *map,
> > return 0;
> > }
> >
> > -static int bpf_object__init_maps(struct bpf_object *obj, int flags)
> > +static int bpf_object__init_global_data_maps(struct bpf_object *obj)
> > +{
> > + int err;
> > +
> > + if (!obj->caps.global_data)
> > + return 0;
> > + /*
> > + * Populate obj->maps with libbpf internal maps.
> > + */
> > + if (obj->efile.data_shndx >= 0) {
> > + err = bpf_object__init_internal_map(obj, LIBBPF_MAP_DATA,
> > + obj->efile.data,
> > + &obj->sections.data);
> > + if (err)
> > + return err;
> > + }
> > + if (obj->efile.rodata_shndx >= 0) {
> > + err = bpf_object__init_internal_map(obj, LIBBPF_MAP_RODATA,
> > + obj->efile.rodata,
> > + &obj->sections.rodata);
> > + if (err)
> > + return err;
> > + }
> > + if (obj->efile.bss_shndx >= 0) {
> > + err = bpf_object__init_internal_map(obj, LIBBPF_MAP_BSS,
> > + obj->efile.bss, NULL);
> > + if (err)
> > + return err;
> > + }
> > + return 0;
> > +}
> > +
> > +static int bpf_object__init_user_maps(struct bpf_object *obj, bool strict)
> > {
> > - int i, map_idx, map_def_sz = 0, nr_syms, nr_maps = 0, nr_maps_glob = 0;
> > - bool strict = !(flags & MAPS_RELAX_COMPAT);
> > Elf_Data *symbols = obj->efile.symbols;
> > + int i, map_def_sz = 0, nr_maps = 0, nr_syms;
> > Elf_Data *data = NULL;
> > - int ret = 0;
> > + Elf_Scn *scn;
> > +
> > + if (obj->efile.maps_shndx < 0)
> > + return 0;
> >
> > if (!symbols)
> > return -EINVAL;
> > - nr_syms = symbols->d_size / sizeof(GElf_Sym);
> > -
> > - if (obj->efile.maps_shndx >= 0) {
> > - Elf_Scn *scn = elf_getscn(obj->efile.elf,
> > - obj->efile.maps_shndx);
> >
> > - if (scn)
> > - data = elf_getdata(scn, NULL);
> > - if (!scn || !data) {
> > - pr_warning("failed to get Elf_Data from map section %d\n",
> > - obj->efile.maps_shndx);
> > - return -EINVAL;
> > - }
> > + scn = elf_getscn(obj->efile.elf, obj->efile.maps_shndx);
> > + if (scn)
> > + data = elf_getdata(scn, NULL);
> > + if (!scn || !data) {
> > + pr_warning("failed to get Elf_Data from map section %d\n",
> > + obj->efile.maps_shndx);
> > + return -EINVAL;
> > }
> >
> > /*
> > @@ -840,16 +901,8 @@ static int bpf_object__init_maps(struct bpf_object *obj, int flags)
> > *
> > * TODO: Detect array of map and report error.
> > */
> > - if (obj->caps.global_data) {
> > - if (obj->efile.data_shndx >= 0)
> > - nr_maps_glob++;
> > - if (obj->efile.rodata_shndx >= 0)
> > - nr_maps_glob++;
> > - if (obj->efile.bss_shndx >= 0)
> > - nr_maps_glob++;
> > - }
> > -
> > - for (i = 0; data && i < nr_syms; i++) {
> > + nr_syms = symbols->d_size / sizeof(GElf_Sym);
> > + for (i = 0; i < nr_syms; i++) {
> > GElf_Sym sym;
> >
> > if (!gelf_getsym(symbols, i, &sym))
> > @@ -858,79 +911,56 @@ static int bpf_object__init_maps(struct bpf_object *obj, int flags)
> > continue;
> > nr_maps++;
> > }
> > -
> > - if (!nr_maps && !nr_maps_glob)
> > - return 0;
> > -
> > /* Assume equally sized map definitions */
> > - if (data) {
> > - pr_debug("maps in %s: %d maps in %zd bytes\n", obj->path,
> > - nr_maps, data->d_size);
> > -
> > - map_def_sz = data->d_size / nr_maps;
> > - if (!data->d_size || (data->d_size % nr_maps) != 0) {
> > - pr_warning("unable to determine map definition size "
> > - "section %s, %d maps in %zd bytes\n",
> > - obj->path, nr_maps, data->d_size);
> > - return -EINVAL;
> > - }
> > - }
> > -
> > - nr_maps += nr_maps_glob;
> > - obj->maps = calloc(nr_maps, sizeof(obj->maps[0]));
> > - if (!obj->maps) {
> > - pr_warning("alloc maps for object failed\n");
> > - return -ENOMEM;
> > - }
> > - obj->nr_maps = nr_maps;
> > -
> > - for (i = 0; i < nr_maps; i++) {
> > - /*
> > - * fill all fd with -1 so won't close incorrect
> > - * fd (fd=0 is stdin) when failure (zclose won't close
> > - * negative fd)).
> > - */
> > - obj->maps[i].fd = -1;
> > - obj->maps[i].inner_map_fd = -1;
> > + pr_debug("maps in %s: %d maps in %zd bytes\n",
> > + obj->path, nr_maps, data->d_size);
> > +
> > + map_def_sz = data->d_size / nr_maps;
> > + if (!data->d_size || (data->d_size % nr_maps) != 0) {
> > + pr_warning("unable to determine map definition size "
> > + "section %s, %d maps in %zd bytes\n",
> > + obj->path, nr_maps, data->d_size);
> > + return -EINVAL;
> > }
> >
> > - /*
> > - * Fill obj->maps using data in "maps" section.
> > - */
> > - for (i = 0, map_idx = 0; data && i < nr_syms; i++) {
> > + /* Fill obj->maps using data in "maps" section. */
> > + for (i = 0; i < nr_syms; i++) {
> > GElf_Sym sym;
> > const char *map_name;
> > struct bpf_map_def *def;
> > + struct bpf_map *map;
> >
> > if (!gelf_getsym(symbols, i, &sym))
> > continue;
> > if (sym.st_shndx != obj->efile.maps_shndx)
> > continue;
> >
> > - map_name = elf_strptr(obj->efile.elf,
> > - obj->efile.strtabidx,
> > + map = bpf_object__add_map(obj);
> > + if (IS_ERR(map))
> > + return PTR_ERR(map);
> > +
> > + map_name = elf_strptr(obj->efile.elf, obj->efile.strtabidx,
> > sym.st_name);
> > if (!map_name) {
> > pr_warning("failed to get map #%d name sym string for obj %s\n",
> > - map_idx, obj->path);
> > + i, obj->path);
> > return -LIBBPF_ERRNO__FORMAT;
> > }
> >
> > - obj->maps[map_idx].libbpf_type = LIBBPF_MAP_UNSPEC;
> > - obj->maps[map_idx].offset = sym.st_value;
> > + map->libbpf_type = LIBBPF_MAP_UNSPEC;
> > + map->offset = sym.st_value;
> > 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);
> > return -EINVAL;
> > }
> >
> > - obj->maps[map_idx].name = strdup(map_name);
> > - if (!obj->maps[map_idx].name) {
> > + map->name = strdup(map_name);
> > + if (!map->name) {
> > pr_warning("failed to alloc map name\n");
> > return -ENOMEM;
> > }
> > - pr_debug("map %d is \"%s\"\n", map_idx,
> > - obj->maps[map_idx].name);
> > + pr_debug("map %d is \"%s\"\n", i, map->name);
> > def = (struct bpf_map_def *)(data->d_buf + sym.st_value);
> > /*
> > * If the definition of the map in the object file fits in
> > @@ -939,7 +969,7 @@ static int bpf_object__init_maps(struct bpf_object *obj, int flags)
> > * calloc above.
> > */
> > if (map_def_sz <= sizeof(struct bpf_map_def)) {
> > - memcpy(&obj->maps[map_idx].def, def, map_def_sz);
> > + memcpy(&map->def, def, map_def_sz);
> > } else {
> > /*
> > * Here the map structure being read is bigger than what
> > @@ -959,37 +989,30 @@ static int bpf_object__init_maps(struct bpf_object *obj, int flags)
> > return -EINVAL;
> > }
> > }
> > - memcpy(&obj->maps[map_idx].def, def,
> > - sizeof(struct bpf_map_def));
> > + memcpy(&map->def, def, sizeof(struct bpf_map_def));
> > }
> > - map_idx++;
> > }
> > + return 0;
> > +}
> >
> > - if (!obj->caps.global_data)
> > - goto finalize;
> > +static int bpf_object__init_maps(struct bpf_object *obj, int flags)
> > +{
> > + bool strict = !(flags & MAPS_RELAX_COMPAT);
> > + int err;
> >
> > - /*
> > - * Populate rest of obj->maps with libbpf internal maps.
> > - */
> > - if (obj->efile.data_shndx >= 0)
> > - ret = bpf_object__init_internal_map(obj, &obj->maps[map_idx++],
> > - LIBBPF_MAP_DATA,
> > - obj->efile.data,
> > - &obj->sections.data);
> > - if (!ret && obj->efile.rodata_shndx >= 0)
> > - ret = bpf_object__init_internal_map(obj, &obj->maps[map_idx++],
> > - LIBBPF_MAP_RODATA,
> > - obj->efile.rodata,
> > - &obj->sections.rodata);
> > - if (!ret && obj->efile.bss_shndx >= 0)
> > - ret = bpf_object__init_internal_map(obj, &obj->maps[map_idx++],
> > - LIBBPF_MAP_BSS,
> > - obj->efile.bss, NULL);
> > -finalize:
> > - if (!ret)
> > + err = bpf_object__init_user_maps(obj, strict);
> > + if (err)
> > + return err;
> > +
> > + err = bpf_object__init_global_data_maps(obj);
> > + if (err)
> > + return err;
> > +
> > + if (obj->nr_maps) {
> > qsort(obj->maps, obj->nr_maps, sizeof(obj->maps[0]),
> > compare_bpf_map);
> > - return ret;
> > + }
> > + return 0;
> > }
> >
> > static bool section_have_execinstr(struct bpf_object *obj, int idx)
> > @@ -1262,14 +1285,10 @@ static int bpf_object__elf_collect(struct bpf_object *obj, int flags)
> > return -LIBBPF_ERRNO__FORMAT;
> > }
> > err = bpf_object__load_btf(obj, btf_data, btf_ext_data);
> > - if (err)
> > - return err;
> > - if (bpf_object__has_maps(obj)) {
> > + if (!err)
> > err = bpf_object__init_maps(obj, flags);
> > - if (err)
> > - return err;
> > - }
> > - err = bpf_object__init_prog_names(obj);
> > + if (!err)
> > + err = bpf_object__init_prog_names(obj);
> > return err;
> > }
> >
> > --
> > 2.17.1
> >
next prev parent reply other threads:[~2019-06-26 18:29 UTC|newest]
Thread overview: 29+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-06-17 19:26 [PATCH v2 bpf-next 00/11] BTF-defined BPF map definitions Andrii Nakryiko
2019-06-17 19:26 ` [PATCH v2 bpf-next 01/11] libbpf: add common min/max macro to libbpf_internal.h Andrii Nakryiko
2019-06-17 19:26 ` [PATCH v2 bpf-next 02/11] libbpf: extract BTF loading logic Andrii Nakryiko
2019-06-17 19:40 ` Song Liu
2019-06-17 19:26 ` [PATCH v2 bpf-next 03/11] libbpf: streamline ELF parsing error-handling Andrii Nakryiko
2019-06-17 19:46 ` Song Liu
2019-06-17 19:26 ` [PATCH v2 bpf-next 04/11] libbpf: refactor map initialization Andrii Nakryiko
2019-06-17 19:39 ` Song Liu
2019-06-26 14:48 ` Matt Hart
2019-06-26 18:28 ` Andrii Nakryiko [this message]
2019-06-27 15:11 ` Matt Hart
2019-06-17 19:26 ` [PATCH v2 bpf-next 05/11] libbpf: identify maps by section index in addition to offset Andrii Nakryiko
2019-06-17 19:26 ` [PATCH v2 bpf-next 06/11] libbpf: split initialization and loading of BTF Andrii Nakryiko
2019-06-17 19:26 ` [PATCH v2 bpf-next 07/11] libbpf: allow specifying map definitions using BTF Andrii Nakryiko
2019-06-17 19:43 ` Song Liu
2019-06-17 19:26 ` [PATCH v2 bpf-next 08/11] selftests/bpf: add test for BTF-defined maps Andrii Nakryiko
2019-06-17 19:26 ` [PATCH v2 bpf-next 09/11] selftests/bpf: switch BPF_ANNOTATE_KV_PAIR tests to " Andrii Nakryiko
2019-06-17 21:41 ` Song Liu
2019-06-17 22:20 ` Daniel Borkmann
2019-06-17 19:26 ` [PATCH v2 bpf-next 10/11] selftests/bpf: convert tests w/ custom values " Andrii Nakryiko
2019-06-17 19:27 ` [PATCH v2 bpf-next 11/11] selftests/bpf: convert remaining selftests " Andrii Nakryiko
2019-06-17 21:17 ` [PATCH v2 bpf-next 00/11] BTF-defined BPF map definitions Daniel Borkmann
2019-06-17 22:17 ` Daniel Borkmann
2019-06-18 21:37 ` Andrii Nakryiko
2019-06-20 14:49 ` Lorenz Bauer
2019-06-21 4:19 ` Andrii Nakryiko
2019-06-21 10:29 ` Lorenz Bauer
2019-06-21 17:56 ` Andrii Nakryiko
2019-06-25 18:14 ` Andrii Nakryiko
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAEf4BzY8wMtq9EmUcsqeLrNOkduKwzuYrweKhhVWKdAb57ax+Q@mail.gmail.com \
--to=andrii.nakryiko@gmail.com \
--cc=andriin@fb.com \
--cc=ast@fb.com \
--cc=bpf@vger.kernel.org \
--cc=daniel@iogearbox.net \
--cc=kernel-team@fb.com \
--cc=linux-perf-users@vger.kernel.org \
--cc=matthew.hart@linaro.org \
--cc=netdev@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).