Re: [PATCH net-next v1] neighbour: Replace kvzalloc() with kzalloc() when GFP_ATOMIC is specified

[Kohei Enju]

>>> > From: Kohei Enju <enjuk@xxxxxxxxxx>
>>> > Date: Mon, 17 Feb 2025 01:30:16 +0900
>>> > > Replace kvzalloc()/kvfree() with kzalloc()/kfree() when GFP_ATOMIC is
>>> > > specified, since kvzalloc() doesn't support non-sleeping allocations such
>>> > > as GFP_ATOMIC.
>>> > >
>>> > > With incompatible gfp flags, kvzalloc() never falls back to the vmalloc
>>> > > path and returns immediately after the kmalloc path fails.
>>> > > Therefore, using kzalloc() is sufficient in this case.
>>> > >
>>> > > Fixes: 41b3caa7c076 ("neighbour: Add hlist_node to struct neighbour")
>>> >
>>> > This commit followed the old hash_buckets allocation, so I'd add
>>> >
>>> >   Fixes: ab101c553bc1 ("neighbour: use kvzalloc()/kvfree()")
>>> >
>>> > too.
>>> >
>>> > Both commits were introduced in v6.13, so there's no difference in terms
>>> > of backporting though.
>>> >
>>> > Also, it would be nice to CC mm maintainers in case they have some
>>> > comments.
>>> 
>>> Oh well, we need to trigger neigh_hash_grow() from a process context,
>>> or convert net/core/neighbour.c to modern rhashtable.
>> 
>> Hi all, thanks for your comments.
>> 
>> kzalloc() uses page allocator when size is larger than 
>> KMALLOC_MAX_CACHE_SIZE, so I think what commit ab101c553bc1 ("neighbour: 
>> use kvzalloc()/kvfree()") intended could be achieved by using kzalloc().
>
>Indeed, kzalloc() should be equivalent to pre-ab101c553bc1 code. kvmalloc()
>would only be necessary if you need more than order-3 page worth of memory
>and don't want it to fail because of fragmentation (but indeed it's not
>supported in GFP_ATOMIC context). But since you didn't need such large
>allocations before, you probably don't need them now too?

Yes, from pre-ab101c553bc1 code, it looks like we don't need the vmalloc 
path for large allocations now too.

>> As mentioned, when using GFP_ATOMIC, kvzalloc() only tries the kmalloc 
>> path, since the vmalloc path doesn't support the flag.
>> In this case, kvzalloc() is equivalent to kzalloc() in that neither try 
>> the vmalloc path, so there is no functional change between this patch and 
>> either commit ab101c553bc1 ("neighbour: use kvzalloc()/kvfree()") or 
>
>Agreed.
>
>> commit 41b3caa7c076 ("neighbour: Add hlist_node to struct neighbour").
>> 
>> Actually there's no real bug in the current code so the Fixes tag was not 
>> appropriate. I shall remove the tag.
>
>True, the code is just more clear.

Thank you for looking at the patches and providing your comments.

I'll send out v2 with the revised commit message, removing the Fixes tag.

Regards,
Kohei