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

[Vlastimil Babka <vbabka@suse.cz>]

On 2/17/25 17:52, Kohei Enju wrote:
> + SLAB ALLOCATOR maintainers and reviewers
> 
>> > From: Kohei Enju <enjuk@amazon.com>
>> > 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?

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

Thanks,
Vlastimil

> Regards,
> Kohei