* [PATCH v2 net-next] gro_cells: avoid using synchronize_rcu() in gro_cells_destroy()
@ 2022-02-20 4:11 Eric Dumazet
2022-02-21 8:23 ` Paolo Abeni
2022-02-22 20:00 ` patchwork-bot+netdevbpf
0 siblings, 2 replies; 5+ messages in thread
From: Eric Dumazet @ 2022-02-20 4:11 UTC (permalink / raw)
To: David S . Miller, Jakub Kicinski; +Cc: netdev, Eric Dumazet, Eric Dumazet
From: Eric Dumazet <edumazet@google.com>
Another thing making netns dismantles potentially very slow is located
in gro_cells_destroy(),
whenever cleanup_net() has to remove a device using gro_cells framework.
RTNL is not held at this stage, so synchronize_net()
is calling synchronize_rcu():
netdev_run_todo()
ip_tunnel_dev_free()
gro_cells_destroy()
synchronize_net()
synchronize_rcu() // Ouch.
This patch uses call_rcu(), and gave me a 25x performance improvement
in my tests.
cleanup_net() is no longer blocked ~10 ms per synchronize_rcu()
call.
In the case we could not allocate the memory needed to queue the
deferred free, use synchronize_rcu_expedited()
v2: made percpu_free_defer_callback() static
Signed-off-by: Eric Dumazet <edumazet@google.com>
---
net/core/gro_cells.c | 36 +++++++++++++++++++++++++++++++-----
1 file changed, 31 insertions(+), 5 deletions(-)
diff --git a/net/core/gro_cells.c b/net/core/gro_cells.c
index 6eb2e5ec2c5068e1d798557e55d084b785187a9b..8462f926ab457322a12510a4d294ecca617948f0 100644
--- a/net/core/gro_cells.c
+++ b/net/core/gro_cells.c
@@ -89,8 +89,23 @@ int gro_cells_init(struct gro_cells *gcells, struct net_device *dev)
}
EXPORT_SYMBOL(gro_cells_init);
+struct percpu_free_defer {
+ struct rcu_head rcu;
+ void __percpu *ptr;
+};
+
+static void percpu_free_defer_callback(struct rcu_head *head)
+{
+ struct percpu_free_defer *defer;
+
+ defer = container_of(head, struct percpu_free_defer, rcu);
+ free_percpu(defer->ptr);
+ kfree(defer);
+}
+
void gro_cells_destroy(struct gro_cells *gcells)
{
+ struct percpu_free_defer *defer;
int i;
if (!gcells->cells)
@@ -102,12 +117,23 @@ void gro_cells_destroy(struct gro_cells *gcells)
__netif_napi_del(&cell->napi);
__skb_queue_purge(&cell->napi_skbs);
}
- /* This barrier is needed because netpoll could access dev->napi_list
- * under rcu protection.
+ /* We need to observe an rcu grace period before freeing ->cells,
+ * because netpoll could access dev->napi_list under rcu protection.
+ * Try hard using call_rcu() instead of synchronize_rcu(),
+ * because we might be called from cleanup_net(), and we
+ * definitely do not want to block this critical task.
*/
- synchronize_net();
-
- free_percpu(gcells->cells);
+ defer = kmalloc(sizeof(*defer), GFP_KERNEL | __GFP_NOWARN);
+ if (likely(defer)) {
+ defer->ptr = gcells->cells;
+ call_rcu(&defer->rcu, percpu_free_defer_callback);
+ } else {
+ /* We do not hold RTNL at this point, synchronize_net()
+ * would not be able to expedite this sync.
+ */
+ synchronize_rcu_expedited();
+ free_percpu(gcells->cells);
+ }
gcells->cells = NULL;
}
EXPORT_SYMBOL(gro_cells_destroy);
--
2.35.1.473.g83b2b277ed-goog
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v2 net-next] gro_cells: avoid using synchronize_rcu() in gro_cells_destroy()
2022-02-20 4:11 [PATCH v2 net-next] gro_cells: avoid using synchronize_rcu() in gro_cells_destroy() Eric Dumazet
@ 2022-02-21 8:23 ` Paolo Abeni
2022-02-21 14:21 ` Eric Dumazet
2022-02-22 20:00 ` patchwork-bot+netdevbpf
1 sibling, 1 reply; 5+ messages in thread
From: Paolo Abeni @ 2022-02-21 8:23 UTC (permalink / raw)
To: Eric Dumazet, David S . Miller, Jakub Kicinski; +Cc: netdev, Eric Dumazet
Hello,
On Sat, 2022-02-19 at 20:11 -0800, Eric Dumazet wrote:
> From: Eric Dumazet <edumazet@google.com>
>
> Another thing making netns dismantles potentially very slow is located
> in gro_cells_destroy(),
> whenever cleanup_net() has to remove a device using gro_cells framework.
>
> RTNL is not held at this stage, so synchronize_net()
> is calling synchronize_rcu():
>
> netdev_run_todo()
> ip_tunnel_dev_free()
> gro_cells_destroy()
> synchronize_net()
> synchronize_rcu() // Ouch.
>
> This patch uses call_rcu(), and gave me a 25x performance improvement
> in my tests.
>
> cleanup_net() is no longer blocked ~10 ms per synchronize_rcu()
> call.
>
> In the case we could not allocate the memory needed to queue the
> deferred free, use synchronize_rcu_expedited()
>
> v2: made percpu_free_defer_callback() static
>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
I'm sorry for the late feedback. I'm wondering if you considered
placing the 'defer' pointer inside 'gro_cells' and allocating it at
gro_cells_init() init time?
Thanks!
Paolo
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2 net-next] gro_cells: avoid using synchronize_rcu() in gro_cells_destroy()
2022-02-21 8:23 ` Paolo Abeni
@ 2022-02-21 14:21 ` Eric Dumazet
2022-02-21 16:45 ` Paolo Abeni
0 siblings, 1 reply; 5+ messages in thread
From: Eric Dumazet @ 2022-02-21 14:21 UTC (permalink / raw)
To: Paolo Abeni; +Cc: Eric Dumazet, David S . Miller, Jakub Kicinski, netdev
On Mon, Feb 21, 2022 at 12:24 AM Paolo Abeni <pabeni@redhat.com> wrote:
>
> Hello,
>
> On Sat, 2022-02-19 at 20:11 -0800, Eric Dumazet wrote:
> > From: Eric Dumazet <edumazet@google.com>
> >
> > Another thing making netns dismantles potentially very slow is located
> > in gro_cells_destroy(),
> > whenever cleanup_net() has to remove a device using gro_cells framework.
> >
> > RTNL is not held at this stage, so synchronize_net()
> > is calling synchronize_rcu():
> >
> > netdev_run_todo()
> > ip_tunnel_dev_free()
> > gro_cells_destroy()
> > synchronize_net()
> > synchronize_rcu() // Ouch.
> >
> > This patch uses call_rcu(), and gave me a 25x performance improvement
> > in my tests.
> >
> > cleanup_net() is no longer blocked ~10 ms per synchronize_rcu()
> > call.
> >
> > In the case we could not allocate the memory needed to queue the
> > deferred free, use synchronize_rcu_expedited()
> >
> > v2: made percpu_free_defer_callback() static
> >
> > Signed-off-by: Eric Dumazet <edumazet@google.com>
>
> I'm sorry for the late feedback. I'm wondering if you considered
> placing the 'defer' pointer inside 'gro_cells' and allocating it at
> gro_cells_init() init time?
I did consider this, but I chose not to risk changing structure
layouts and adding regression in fast paths,
with extra cache line misses.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2 net-next] gro_cells: avoid using synchronize_rcu() in gro_cells_destroy()
2022-02-21 14:21 ` Eric Dumazet
@ 2022-02-21 16:45 ` Paolo Abeni
0 siblings, 0 replies; 5+ messages in thread
From: Paolo Abeni @ 2022-02-21 16:45 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Eric Dumazet, David S . Miller, Jakub Kicinski, netdev
On Mon, 2022-02-21 at 06:21 -0800, Eric Dumazet wrote:
> On Mon, Feb 21, 2022 at 12:24 AM Paolo Abeni <pabeni@redhat.com> wrote:
> >
> > Hello,
> >
> > On Sat, 2022-02-19 at 20:11 -0800, Eric Dumazet wrote:
> > > From: Eric Dumazet <edumazet@google.com>
> > >
> > > Another thing making netns dismantles potentially very slow is located
> > > in gro_cells_destroy(),
> > > whenever cleanup_net() has to remove a device using gro_cells framework.
> > >
> > > RTNL is not held at this stage, so synchronize_net()
> > > is calling synchronize_rcu():
> > >
> > > netdev_run_todo()
> > > ip_tunnel_dev_free()
> > > gro_cells_destroy()
> > > synchronize_net()
> > > synchronize_rcu() // Ouch.
> > >
> > > This patch uses call_rcu(), and gave me a 25x performance improvement
> > > in my tests.
> > >
> > > cleanup_net() is no longer blocked ~10 ms per synchronize_rcu()
> > > call.
> > >
> > > In the case we could not allocate the memory needed to queue the
> > > deferred free, use synchronize_rcu_expedited()
> > >
> > > v2: made percpu_free_defer_callback() static
> > >
> > > Signed-off-by: Eric Dumazet <edumazet@google.com>
> >
> > I'm sorry for the late feedback. I'm wondering if you considered
> > placing the 'defer' pointer inside 'gro_cells' and allocating it at
> > gro_cells_init() init time?
>
> I did consider this, but I chose not to risk changing structure
> layouts and adding regression in fast paths,
> with extra cache line misses.
Understood, thanks!
Acked-by: Paolo Abeni <pabeni@redhat.com>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2 net-next] gro_cells: avoid using synchronize_rcu() in gro_cells_destroy()
2022-02-20 4:11 [PATCH v2 net-next] gro_cells: avoid using synchronize_rcu() in gro_cells_destroy() Eric Dumazet
2022-02-21 8:23 ` Paolo Abeni
@ 2022-02-22 20:00 ` patchwork-bot+netdevbpf
1 sibling, 0 replies; 5+ messages in thread
From: patchwork-bot+netdevbpf @ 2022-02-22 20:00 UTC (permalink / raw)
To: Eric Dumazet; +Cc: davem, kuba, netdev, edumazet
Hello:
This patch was applied to netdev/net-next.git (master)
by Jakub Kicinski <kuba@kernel.org>:
On Sat, 19 Feb 2022 20:11:55 -0800 you wrote:
> From: Eric Dumazet <edumazet@google.com>
>
> Another thing making netns dismantles potentially very slow is located
> in gro_cells_destroy(),
> whenever cleanup_net() has to remove a device using gro_cells framework.
>
> RTNL is not held at this stage, so synchronize_net()
> is calling synchronize_rcu():
>
> [...]
Here is the summary with links:
- [v2,net-next] gro_cells: avoid using synchronize_rcu() in gro_cells_destroy()
https://git.kernel.org/netdev/net-next/c/ee8f97efa7a5
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-02-22 20:00 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-02-20 4:11 [PATCH v2 net-next] gro_cells: avoid using synchronize_rcu() in gro_cells_destroy() Eric Dumazet
2022-02-21 8:23 ` Paolo Abeni
2022-02-21 14:21 ` Eric Dumazet
2022-02-21 16:45 ` Paolo Abeni
2022-02-22 20:00 ` patchwork-bot+netdevbpf
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.