Re: [PATCH bpf 1/2] bpf/memalloc: Non-atomically allocate freelist during prefill

[Hou Tao <houtao@huaweicloud.com>]

On 7/21/2023 4:44 AM, YiFei Zhu wrote:
> Sometimes during prefill all precpu chunks are full and atomic
> __alloc_percpu_gfp would not allocate new chunks. This will cause
> -ENOMEM immediately upon next unit_alloc.
>
> Prefill phase does not actually run in atomic context, so we can
> use this fact to allocate non-atomically with GFP_KERNEL instead
> of GFP_NOWAIT. This avoids the immediate -ENOMEM. Unfortunately
> unit_alloc runs in atomic context, even from map item allocation in
> syscalls, due to rcu_read_lock, so we can't do non-atomic
> workarounds in unit_alloc.
>
> Fixes: 4ab67149f3c6 ("bpf: Add percpu allocation support to bpf_mem_alloc.")
> Signed-off-by: YiFei Zhu <zhuyifei@google.com>

Make sense to me, so

Acked-by: Hou Tao <houtao1@huawei.com>

But I don't know whether or not it is suitable for bpf tree.
> ---
>  kernel/bpf/memalloc.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/kernel/bpf/memalloc.c b/kernel/bpf/memalloc.c
> index 0668bcd7c926..016249672b43 100644
> --- a/kernel/bpf/memalloc.c
> +++ b/kernel/bpf/memalloc.c
> @@ -154,13 +154,17 @@ static struct mem_cgroup *get_memcg(const struct bpf_mem_cache *c)
>  }
>  
>  /* Mostly runs from irq_work except __init phase. */
> -static void alloc_bulk(struct bpf_mem_cache *c, int cnt, int node)
> +static void alloc_bulk(struct bpf_mem_cache *c, int cnt, int node, bool atomic)
>  {
>  	struct mem_cgroup *memcg = NULL, *old_memcg;
>  	unsigned long flags;
> +	gfp_t gfp;
>  	void *obj;
>  	int i;
>  
> +	gfp = __GFP_NOWARN | __GFP_ACCOUNT;
> +	gfp |= atomic ? GFP_NOWAIT : GFP_KERNEL;
> +
>  	memcg = get_memcg(c);
>  	old_memcg = set_active_memcg(memcg);
>  	for (i = 0; i < cnt; i++) {
> @@ -183,7 +187,7 @@ static void alloc_bulk(struct bpf_mem_cache *c, int cnt, int node)
>  			 * will allocate from the current numa node which is what we
>  			 * want here.
>  			 */
> -			obj = __alloc(c, node, GFP_NOWAIT | __GFP_NOWARN | __GFP_ACCOUNT);
> +			obj = __alloc(c, node, gfp);
>  			if (!obj)
>  				break;
>  		}
> @@ -321,7 +325,7 @@ static void bpf_mem_refill(struct irq_work *work)
>  		/* irq_work runs on this cpu and kmalloc will allocate
>  		 * from the current numa node which is what we want here.
>  		 */
> -		alloc_bulk(c, c->batch, NUMA_NO_NODE);
> +		alloc_bulk(c, c->batch, NUMA_NO_NODE, true);
>  	else if (cnt > c->high_watermark)
>  		free_bulk(c);
>  }
> @@ -367,7 +371,7 @@ static void prefill_mem_cache(struct bpf_mem_cache *c, int cpu)
>  	 * prog won't be doing more than 4 map_update_elem from
>  	 * irq disabled region
>  	 */
> -	alloc_bulk(c, c->unit_size <= 256 ? 4 : 1, cpu_to_node(cpu));
> +	alloc_bulk(c, c->unit_size <= 256 ? 4 : 1, cpu_to_node(cpu), false);
>  }
>  
>  /* When size != 0 bpf_mem_cache for each cpu.