[PATCH net-next] netpoll: remove netpoll_srcu

[PATCH net-next] netpoll: remove netpoll_srcu

[Eric Dumazet]

netpoll_srcu is currently used from netpoll_poll_disable() and
__netpoll_cleanup()

Both functions run under RTNL, using netpoll_srcu adds confusion
and no additional protection.

Moreover the synchronize_srcu() call in __netpoll_cleanup() is
performed before clearing np->dev->npinfo, which violates RCU rules.

After this patch, netpoll_poll_disable() and netpoll_poll_enable()
simply use rtnl_dereference().

This saves a big chunk of memory (more than 192KB on platforms
with 512 cpus)

Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Breno Leitao <leitao@debian.org>
---
 net/core/netpoll.c | 15 ++++-----------
 1 file changed, 4 insertions(+), 11 deletions(-)

diff --git a/net/core/netpoll.c b/net/core/netpoll.c
index e0720ee6fa6206c44f996065f11261012da0fb6f..ca52cbe0f63cf6ae394da3cf1ed4cc7cd8a7254e 100644
--- a/net/core/netpoll.c
+++ b/net/core/netpoll.c
@@ -48,8 +48,6 @@
 
 static struct sk_buff_head skb_pool;
 
-DEFINE_STATIC_SRCU(netpoll_srcu);
-
 #define USEC_PER_POLL	50
 
 #define MAX_SKB_SIZE							\
@@ -220,23 +218,20 @@ EXPORT_SYMBOL(netpoll_poll_dev);
 void netpoll_poll_disable(struct net_device *dev)
 {
 	struct netpoll_info *ni;
-	int idx;
+
 	might_sleep();
-	idx = srcu_read_lock(&netpoll_srcu);
-	ni = srcu_dereference(dev->npinfo, &netpoll_srcu);
+	ni = rtnl_dereference(dev->npinfo);
 	if (ni)
 		down(&ni->dev_lock);
-	srcu_read_unlock(&netpoll_srcu, idx);
 }
 
 void netpoll_poll_enable(struct net_device *dev)
 {
 	struct netpoll_info *ni;
-	rcu_read_lock();
-	ni = rcu_dereference(dev->npinfo);
+
+	ni = rtnl_dereference(dev->npinfo);
 	if (ni)
 		up(&ni->dev_lock);
-	rcu_read_unlock();
 }
 
 static void refill_skbs(void)
@@ -829,8 +824,6 @@ void __netpoll_cleanup(struct netpoll *np)
 	if (!npinfo)
 		return;
 
-	synchronize_srcu(&netpoll_srcu);
-
 	if (refcount_dec_and_test(&npinfo->refcnt)) {
 		const struct net_device_ops *ops;
 
-- 
2.46.0.469.g59c65b2a67-goog

Re: [PATCH net-next] netpoll: remove netpoll_srcu

[Breno Leitao]

Hello Eric,

On Thu, Sep 05, 2024 at 08:49:09AM +0000, Eric Dumazet wrote:
> netpoll_srcu is currently used from netpoll_poll_disable() and
> __netpoll_cleanup()
> 
> Both functions run under RTNL, using netpoll_srcu adds confusion
> and no additional protection.

Thanks for fixing it. I have never understood that SRCU in netpoll.

Reading this code now again, I have the impression that netpoll_srcu was
created to be able to dereference ->npinfo using an RCU primitive.

> Moreover the synchronize_srcu() call in __netpoll_cleanup() is
> performed before clearing np->dev->npinfo, which violates RCU rules.

I think the goal was to make sure that all the readers are not in the
critical section anymore, and wait for them. But it is unclear why it is
mixing srcu_read_lock() and rcu_read_lock() together.

Anyway, thanks for solving this.

> After this patch, netpoll_poll_disable() and netpoll_poll_enable()
> simply use rtnl_dereference().
> 
> This saves a big chunk of memory (more than 192KB on platforms
> with 512 cpus)
> 
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: Breno Leitao <leitao@debian.org>

Reviewed-by: Breno Leitao <leitao@debian.org>

I've double checked that netpoll_poll_disable() and
netpoll_poll_enable() is always called with rtnl held, so, the change is
safe.

Re: [PATCH net-next] netpoll: remove netpoll_srcu

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Thu,  5 Sep 2024 08:49:09 +0000 you wrote:
> netpoll_srcu is currently used from netpoll_poll_disable() and
> __netpoll_cleanup()
> 
> Both functions run under RTNL, using netpoll_srcu adds confusion
> and no additional protection.
> 
> Moreover the synchronize_srcu() call in __netpoll_cleanup() is
> performed before clearing np->dev->npinfo, which violates RCU rules.
> 
> [...]

Here is the summary with links:
  - [net-next] netpoll: remove netpoll_srcu
    https://git.kernel.org/netdev/net-next/c/9a95eedc81de

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html