Date:   Mon, 17 Jun 2019 19:39:24 +0000
From:   Song Liu <songliubraving@...com>
To:     Andrii Nakryiko <andriin@...com>
CC:     Andrii Nakryiko <andrii.nakryiko@...il.com>,
        Alexei Starovoitov <ast@...com>,
        "daniel@...earbox.net" <daniel@...earbox.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "bpf@...r.kernel.org" <bpf@...r.kernel.org>,
        Kernel Team <Kernel-team@...com>
Subject: Re: [PATCH v2 bpf-next 04/11] libbpf: refactor map initialization



> On Jun 17, 2019, at 12:26 PM, Andrii Nakryiko <andriin@...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@...com>

Acked-by: Song Liu <songliubraving@...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
> 

Powered by blists - more mailing lists