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

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

[Kohei Enju]

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")
Signed-off-by: Kohei Enju <enjuk@amazon.com>
---
 net/core/neighbour.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/core/neighbour.c b/net/core/neighbour.c
index d8dd686b5287..344c9cd168ec 100644
--- a/net/core/neighbour.c
+++ b/net/core/neighbour.c
@@ -518,7 +518,7 @@ static struct neigh_hash_table *neigh_hash_alloc(unsigned int shift)
 	if (!ret)
 		return NULL;
 
-	hash_heads = kvzalloc(size, GFP_ATOMIC);
+	hash_heads = kzalloc(size, GFP_ATOMIC);
 	if (!hash_heads) {
 		kfree(ret);
 		return NULL;
@@ -536,7 +536,7 @@ static void neigh_hash_free_rcu(struct rcu_head *head)
 						    struct neigh_hash_table,
 						    rcu);
 
-	kvfree(nht->hash_heads);
+	kfree(nht->hash_heads);
 	kfree(nht);
 }
 
-- 
2.39.5 (Apple Git-154)

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

[Kuniyuki Iwashima]

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.


> Signed-off-by: Kohei Enju <enjuk@amazon.com>
> ---
>  net/core/neighbour.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/net/core/neighbour.c b/net/core/neighbour.c
> index d8dd686b5287..344c9cd168ec 100644
> --- a/net/core/neighbour.c
> +++ b/net/core/neighbour.c
> @@ -518,7 +518,7 @@ static struct neigh_hash_table *neigh_hash_alloc(unsigned int shift)
>  	if (!ret)
>  		return NULL;
>  
> -	hash_heads = kvzalloc(size, GFP_ATOMIC);
> +	hash_heads = kzalloc(size, GFP_ATOMIC);
>  	if (!hash_heads) {
>  		kfree(ret);
>  		return NULL;
> @@ -536,7 +536,7 @@ static void neigh_hash_free_rcu(struct rcu_head *head)
>  						    struct neigh_hash_table,
>  						    rcu);
>  
> -	kvfree(nht->hash_heads);
> +	kfree(nht->hash_heads);
>  	kfree(nht);
>  }
>  
> -- 
> 2.39.5 (Apple Git-154)

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

[Eric Dumazet]

On Sun, Feb 16, 2025 at 8:07 PM Kuniyuki Iwashima <kuniyu@amazon.com> wrote:
>
> 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.

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

[Kohei Enju]

+ 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().

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

Regards,
Kohei

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

[Vlastimil Babka]

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

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

[Kohei Enju]

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

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