| Message ID | 20240905084909.2082486-1-edumazet@google.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 9a95eedc81deb86af1ac56f2c2bfe8306b27b82a |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] netpoll: remove netpoll_srcu | expand |
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.
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!
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;
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(-)