Re: [PATCH bpf-next v3 12/13] tools: libbpf: allow map reuse

[Andrey Ignatov <rdna@fb.com>]

Jakub Kicinski <jakub.kicinski@netronome.com> [Tue, 2018-07-10 14:43 -0700]:
> More advanced applications may want to only replace programs without
> destroying associated maps.  Allow libbpf users to achieve that.
> Instead of always creating all of the maps at load time, expose to
> users an API to reconstruct the map object from already existing
> map.
> 
> The map parameters are read from the kernel and replace the parameters
> of the ELF map.  libbpf does not restrict the map replacement, i.e.
> the reused map does not have to be compatible with the ELF map
> definition.  We relay on the verifier for checking the compatibility
> between maps and programs.  The ELF map definition is completely
> overwritten by the information read from the kernel, to make sure
> libbpf's view of map object corresponds to the actual map.
> 
> Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> Reviewed-by: Quentin Monnet <quentin.monnet@netronome.com>
> ---
> v3 (Andrey):
>   - use dup3();
>   - close existing fd;
>   - handle errors better;
>   - depend on fd value to determine pre-set.
> ---
>  tools/lib/bpf/libbpf.c | 53 ++++++++++++++++++++++++++++++++++++++++++
>  tools/lib/bpf/libbpf.h |  1 +
>  2 files changed, 54 insertions(+)
> 
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index fd2c4433863d..955f8eafbf41 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -1035,6 +1035,53 @@ static int bpf_map_find_btf_info(struct bpf_map *map, const struct btf *btf)
>  	return 0;
>  }
>  
> +int bpf_map__reuse_fd(struct bpf_map *map, int fd)
> +{
> +	struct bpf_map_info info = {};
> +	__u32 len = sizeof(info);
> +	int new_fd, err;
> +	char *new_name;
> +
> +	err = bpf_obj_get_info_by_fd(fd, &info, &len);
> +	if (err)
> +		return err;
> +
> +	new_name = strdup(info.name);
> +	if (!new_name)
> +		return -errno;
> +
> +	new_fd = open("/", O_RDONLY | O_CLOEXEC);
> +	if (new_fd < 0)
> +		goto err_free_new_name;
> +
> +	new_fd = dup3(fd, new_fd, O_CLOEXEC);
> +	if (new_fd < 0)
> +		goto err_close_new_fd;
> +
> +	err = zclose(map->fd);
> +	if (err)
> +		goto err_close_new_fd;
> +	free(map->name);
> +
> +	map->fd = new_fd;
> +	map->name = new_name;
> +	map->def.type = info.type;
> +	map->def.key_size = info.key_size;
> +	map->def.value_size = info.value_size;
> +	map->def.max_entries = info.max_entries;
> +	map->def.map_flags = info.map_flags;
> +	map->btf_key_type_id = info.btf_key_type_id;
> +	map->btf_value_type_id = info.btf_value_type_id;
> +
> +	return 0;
> +
> +err_close_new_fd:
> +	close(new_fd);
> +err_free_new_name:
> +	free(new_name);
> +	return -errno;
> +}
> +
>  static int
>  bpf_object__create_maps(struct bpf_object *obj)
>  {
> @@ -1047,6 +1094,12 @@ bpf_object__create_maps(struct bpf_object *obj)
>  		struct bpf_map_def *def = &map->def;
>  		int *pfd = &map->fd;
>  
> +		if (map->fd >= 0) {
> +			pr_debug("skip map create (preset) %s: fd=%d\n",
> +				 map->name, map->fd);
> +			continue;
> +		}
> +
>  		create_attr.name = map->name;
>  		create_attr.map_ifindex = map->map_ifindex;
>  		create_attr.map_type = def->type;
> diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> index e911ad32d02e..1f8fc2060460 100644
> --- a/tools/lib/bpf/libbpf.h
> +++ b/tools/lib/bpf/libbpf.h
> @@ -261,6 +261,7 @@ typedef void (*bpf_map_clear_priv_t)(struct bpf_map *, void *);
>  int bpf_map__set_priv(struct bpf_map *map, void *priv,
>  		      bpf_map_clear_priv_t clear_priv);
>  void *bpf_map__priv(struct bpf_map *map);
> +int bpf_map__reuse_fd(struct bpf_map *map, int fd);
>  bool bpf_map__is_offload_neutral(struct bpf_map *map);
>  void bpf_map__set_ifindex(struct bpf_map *map, __u32 ifindex);
>  int bpf_map__pin(struct bpf_map *map, const char *path);
> -- 
> 2.17.1
> 

Acked-by: Andrey Ignatov <rdna@fb.com>

Thanks for all the changes Jakub! Sorry, it forced you to deal with
strerror_r().

One thing, I'm not really sure, is if there is a reason not to copy
ifindex as well from the map that corresponds to passed fd. It's fine
with me to follow-up separately though if it's needed.


-- 
Andrey Ignatov