| Message ID | 20240822082519.216070-1-vmalik@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | objpool: fix choosing allocation for percpu slots | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On Thu, Aug 22, 2024 at 1:27 AM Viktor Malik <vmalik@redhat.com> 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> > 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 <vmalik@redhat.com> > --- > 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 > >
On 2024/8/22 16:25, Viktor Malik 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 > cases (even if GFP_KERNEL is requested). Since kmalloc cannot allocate > large amounts of memory, this may lead to unexpected OOM errors. Sure, good catch. Don't notice that GFP_ATOMIC is not atomic. My original intention is using kmalloc only if GFP_ATOMIC is specified and other flags should go with vmalloc, but (pool->gfp == GFP_ATOMIC) is not accurate. Masami, please help review and include this patch into your patch set if it's appropriate to you. Thanks. Reviewed-by: Matt Wu <wuqiang.matt@bytedance.com> > 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 <vmalik@redhat.com> > --- > 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,
On 8/22/24 11:30 PM, Andrii Nakryiko wrote: > On Thu, Aug 22, 2024 at 1:27 AM Viktor Malik <vmalik@redhat.com> 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 Right, the commit message could use a better explanation, thanks! I'll update it, add the acks, and send v2. Viktor > > Acked-by: Andrii Nakryiko <andrii@kernel.org> > >> 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 <vmalik@redhat.com> >> --- >> 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 >> >> >
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,
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 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 <vmalik@redhat.com> --- lib/objpool.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)