| Message ID | 20250324224537.248800-8-kuba@kernel.org (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | b52458652eca5a551ddb55605201b136f091b04d |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: skip taking rtnl_lock for queue GET | expand |
On Mon, Mar 24, 2025 at 3:47 PM Jakub Kicinski <kuba@kernel.org> wrote: > > Ensure that all accesses to mp_params are under the netdev > instance lock. The only change we need is to move > dev_memory_provider_uninstall() under the lock. > > Appropriately swap the asserts. > > Signed-off-by: Jakub Kicinski <kuba@kernel.org> Reviewed-by: Mina Almasry <almasrymina@google.com> > --- > net/core/dev.c | 4 ++-- > net/core/page_pool.c | 7 ++----- > 2 files changed, 4 insertions(+), 7 deletions(-) > > diff --git a/net/core/dev.c b/net/core/dev.c > index 690d46497b2f..652f2c6f5674 100644 > --- a/net/core/dev.c > +++ b/net/core/dev.c > @@ -10353,7 +10353,7 @@ u32 dev_get_min_mp_channel_count(const struct net_device *dev) > { > int i; > > - ASSERT_RTNL(); > + netdev_ops_assert_locked(dev); > > for (i = dev->real_num_rx_queues - 1; i >= 0; i--) > if (dev->_rx[i].mp_params.mp_priv) > @@ -11957,9 +11957,9 @@ void unregister_netdevice_many_notify(struct list_head *head, > dev_tcx_uninstall(dev); > netdev_lock_ops(dev); > dev_xdp_uninstall(dev); > + dev_memory_provider_uninstall(dev); > netdev_unlock_ops(dev); > bpf_dev_bound_netdev_unregister(dev); > - dev_memory_provider_uninstall(dev); So initially I thought this may be wrong because netdev_lock_ops() only locks if there are queue_mgmt_ops, but access to mp_params should be locked anyway. But I guess you're relying on the fact that if the device doesn't support queue_mgmt_ops memory providers don't work anyway.
On Mon, 24 Mar 2025 22:34:43 -0700 Mina Almasry wrote: > > @@ -11957,9 +11957,9 @@ void unregister_netdevice_many_notify(struct list_head *head, > > dev_tcx_uninstall(dev); > > netdev_lock_ops(dev); > > dev_xdp_uninstall(dev); > > + dev_memory_provider_uninstall(dev); > > netdev_unlock_ops(dev); > > bpf_dev_bound_netdev_unregister(dev); > > - dev_memory_provider_uninstall(dev); > > So initially I thought this may be wrong because netdev_lock_ops() > only locks if there are queue_mgmt_ops, but access to mp_params should > be locked anyway. But I guess you're relying on the fact that if the > device doesn't support queue_mgmt_ops memory providers don't work > anyway. Right, my expectation is that they must be NULL if device is not ops-locked. Not sure if that's what textbooks would consider "correct" but I think KCSAN will not complain :)
diff --git a/net/core/dev.c b/net/core/dev.c index 690d46497b2f..652f2c6f5674 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -10353,7 +10353,7 @@ u32 dev_get_min_mp_channel_count(const struct net_device *dev) { int i; - ASSERT_RTNL(); + netdev_ops_assert_locked(dev); for (i = dev->real_num_rx_queues - 1; i >= 0; i--) if (dev->_rx[i].mp_params.mp_priv) @@ -11957,9 +11957,9 @@ void unregister_netdevice_many_notify(struct list_head *head, dev_tcx_uninstall(dev); netdev_lock_ops(dev); dev_xdp_uninstall(dev); + dev_memory_provider_uninstall(dev); netdev_unlock_ops(dev); bpf_dev_bound_netdev_unregister(dev); - dev_memory_provider_uninstall(dev); netdev_offload_xstats_disable_all(dev); diff --git a/net/core/page_pool.c b/net/core/page_pool.c index acef1fcd8ddc..7745ad924ae2 100644 --- a/net/core/page_pool.c +++ b/net/core/page_pool.c @@ -11,6 +11,7 @@ #include <linux/slab.h> #include <linux/device.h> +#include <net/netdev_lock.h> #include <net/netdev_rx_queue.h> #include <net/page_pool/helpers.h> #include <net/page_pool/memory_provider.h> @@ -279,11 +280,7 @@ static int page_pool_init(struct page_pool *pool, get_device(pool->p.dev); if (pool->slow.flags & PP_FLAG_ALLOW_UNREADABLE_NETMEM) { - /* We rely on rtnl_lock()ing to make sure netdev_rx_queue - * configuration doesn't change while we're initializing - * the page_pool. - */ - ASSERT_RTNL(); + netdev_assert_locked(pool->slow.netdev); rxq = __netif_get_rx_queue(pool->slow.netdev, pool->slow.queue_idx); pool->mp_priv = rxq->mp_params.mp_priv;
Ensure that all accesses to mp_params are under the netdev instance lock. The only change we need is to move dev_memory_provider_uninstall() under the lock. Appropriately swap the asserts. Signed-off-by: Jakub Kicinski <kuba@kernel.org> --- net/core/dev.c | 4 ++-- net/core/page_pool.c | 7 ++----- 2 files changed, 4 insertions(+), 7 deletions(-)