Re: [PATCH] objpool: fix choosing allocation for percpu slots

[Viktor Malik <vmalik@redhat.com>]

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
>>
>>
>