Re: [RFC PATCH bpf-next 05/15] bpf: Introduce helpers for container of struct bpf_map

[Alexei Starovoitov <alexei.starovoitov@gmail.com>]

On Fri, Jul 29, 2022 at 03:23:06PM +0000, Yafang Shao wrote:
> Currently bpf_map_area_alloc() is used to allocate a container of struct
> bpf_map or members in this container. To distinguish the map creation
> and other members, let split it into two different helpers,
>   - bpf_map_container_alloc()
>     Used to allocate a container of struct bpf_map, the container is as
>     follows,
>       struct bpf_map_container {
>         struct bpf_map map;  // the map must be the first member
>         ....
>       };
>     Pls. note that the struct bpf_map_contianer is a abstract one, which
>     can be struct bpf_array, struct bpf_bloom_filter and etc.
> 
>     In this helper, it will call bpf_map_save_memcg() to init memcg
>     relevant data in the bpf map. And these data will be cleared in
>     bpf_map_container_free().
> 
>   - bpf_map_area_alloc()
>     Now it is used to allocate the members in a contianer only.
> 
> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> ---
>  include/linux/bpf.h  |  4 ++++
>  kernel/bpf/syscall.c | 56 ++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 60 insertions(+)
> 
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index 20c26aed7896..2d971b0eb24b 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -1634,9 +1634,13 @@ void bpf_map_inc_with_uref(struct bpf_map *map);
>  struct bpf_map * __must_check bpf_map_inc_not_zero(struct bpf_map *map);
>  void bpf_map_put_with_uref(struct bpf_map *map);
>  void bpf_map_put(struct bpf_map *map);
> +void *bpf_map_container_alloc(u64 size, int numa_node);
> +void *bpf_map_container_mmapable_alloc(u64 size, int numa_node,
> +				       u32 align, u32 offset);
>  void *bpf_map_area_alloc(u64 size, int numa_node);
>  void *bpf_map_area_mmapable_alloc(u64 size, int numa_node);
>  void bpf_map_area_free(void *base);
> +void bpf_map_container_free(void *base);
>  bool bpf_map_write_active(const struct bpf_map *map);
>  void bpf_map_init_from_attr(struct bpf_map *map, union bpf_attr *attr);
>  int  generic_map_lookup_batch(struct bpf_map *map,
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index 83c7136c5788..1a1a81a11b37 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -495,6 +495,62 @@ static void bpf_map_release_memcg(struct bpf_map *map)
>  }
>  #endif
>  
> +/*
> + * The return pointer is a bpf_map container, as follow,
> + *   struct bpf_map_container {
> + *       struct bpf_map map;
> + *       ...
> + *   };
> + *
> + * It is used in map creation path.
> + */
> +void *bpf_map_container_alloc(u64 size, int numa_node)
> +{
> +	struct bpf_map *map;
> +	void *container;
> +
> +	container = __bpf_map_area_alloc(size, numa_node, false);
> +	if (!container)
> +		return NULL;
> +
> +	map = (struct bpf_map *)container;
> +	bpf_map_save_memcg(map);
> +
> +	return container;
> +}
> +
> +void *bpf_map_container_mmapable_alloc(u64 size, int numa_node, u32 align,
> +				       u32 offset)
> +{
> +	struct bpf_map *map;
> +	void *container;
> +	void *ptr;
> +
> +	/* kmalloc'ed memory can't be mmap'ed, use explicit vmalloc */
> +	ptr = __bpf_map_area_alloc(size, numa_node, true);
> +	if (!ptr)
> +		return NULL;
> +
> +	container = ptr + align - offset;
> +	map = (struct bpf_map *)container;
> +	bpf_map_save_memcg(map);

This is very error prone.
I don't think the container concept is necessary.
bpf_map_area_alloc() can just take extra memcg_fd argument.

> +
> +	return ptr;
> +}
> +
> +void bpf_map_container_free(void *container)
> +{
> +	struct bpf_map *map;
> +
> +	if (!container)
> +		return;
> +
> +	map = (struct bpf_map *)container;
> +	bpf_map_release_memcg(map);
> +
> +	kvfree(container);
> +}
> +
>  static int bpf_map_kptr_off_cmp(const void *a, const void *b)
>  {
>  	const struct bpf_map_value_off_desc *off_desc1 = a, *off_desc2 = b;
> -- 
> 2.17.1
>