Re: [PATCH] objpool: fix choosing allocation for percpu slots
From: Andrii Nakryiko <hidden>
Date: 2024-08-22 21:30:17
Also in:
bpf
On Thu, Aug 22, 2024 at 1:27 AM Viktor Malik [off-list ref] wrote:
objpool intends to use vmalloc for default (non-atomic) allocations of percpu slots and objects. However, the condition checking if GFP flags are equal to GFP_ATOMIC is wrong and causes kmalloc to be used in most
I was confused by this, because original code has no equality and it looks like correct code. But in reality GFP_ATOMIC is a collection of bits (__GFP_HIGH|__GFP_KSWAPD_RECLAIM), and so `pool->gfp & GFP_ATOMIC` will be true if either bit is set, hence your change. Also, GFP_ATOMIC and GFP_KERNEL share ___GFP_KSWAPD_RECLAIM bit specifically, which is what causes the use of kmalloc_node(), always. It would be nice to expand on that in the commit. Other than that LGTM Acked-by: Andrii Nakryiko <andrii@kernel.org>
quoted hunk ↗ jump to hunk
cases (even if GFP_KERNEL is requested). Since kmalloc cannot allocate large amounts of memory, this may lead to unexpected OOM errors. For instance, objpool is used by fprobe rethook which in turn is used by BPF kretprobe.multi and kprobe.session probe types. Trying to attach these to all kernel functions with libbpf using SEC("kprobe.session/*") int kprobe(struct pt_regs *ctx) { [...] } fails on objpool slot allocation with ENOMEM. Fix the condition to truly use vmalloc by default. Fixes: b4edb8d2d464 ("lib: objpool added: ring-array based lockless MPMC") Signed-off-by: Viktor Malik <redacted> --- lib/objpool.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)diff --git a/lib/objpool.c b/lib/objpool.c index 234f9d0bd081..fd108fe0d095 100644 --- a/lib/objpool.c +++ b/lib/objpool.c@@ -76,7 +76,7 @@ objpool_init_percpu_slots(struct objpool_head *pool, int nr_objs, * mimimal size of vmalloc is one page since vmalloc would * always align the requested size to page size */ - if (pool->gfp & GFP_ATOMIC) + if ((pool->gfp & GFP_ATOMIC) == GFP_ATOMIC) slot = kmalloc_node(size, pool->gfp, cpu_to_node(i)); else slot = __vmalloc_node(size, sizeof(void *), pool->gfp, --2.46.0