Re: [PATCH v2 bpf-next 04/11] libbpf: refactor map initialization

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, 26 Jun 2019 at 19:29, Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx> wrote:
>
> On Wed, Jun 26, 2019 at 7:48 AM Matt Hart <matthew.hart@xxxxxxxxxx> 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.

Thanks, I see that patch has hit linux-next so perf is building again.

> >
> >
> >
> > On Tue, 25 Jun 2019 at 16:53, Andrii Nakryiko <andriin@xxxxxx> 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@xxxxxx>
> > > ---
> > >  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
> > >




[Index of Archives]     [Linux Samsung SoC]     [Linux Rockchip SoC]     [Linux Actions SoC]     [Linux for Synopsys ARC Processors]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]


  Powered by Linux