Recently bnxt had to grow back a bunch of rtnl dependencies because of udp_tunnel's infra. Add separate (global) mutext to protect udp_tunnel state.
v3: - fix 2 test failures (Jakub NIPA)
v2: - move the lock into udp_tunnel_nic (Jakub) - reorder the lock ordering (Jakub) - move udp_ports_sleep removal into separate patch and update the test (Jakub)
Cc: Michael Chan michael.chan@broadcom.com
Stanislav Fomichev (4): udp_tunnel: remove rtnl_lock dependency net: remove redundant ASSERT_RTNL() in queue setup functions netdevsim: remove udp_ports_sleep Revert "bnxt_en: bring back rtnl_lock() in the bnxt_open() path"
.../net/ethernet/broadcom/bnx2x/bnx2x_main.c | 3 +- drivers/net/ethernet/broadcom/bnxt/bnxt.c | 42 ++++--------------- drivers/net/ethernet/emulex/benet/be_main.c | 3 +- drivers/net/ethernet/intel/i40e/i40e_main.c | 1 - drivers/net/ethernet/intel/ice/ice_main.c | 1 - .../net/ethernet/mellanox/mlx4/en_netdev.c | 3 +- .../net/ethernet/mellanox/mlx5/core/en_main.c | 3 +- .../ethernet/netronome/nfp/nfp_net_common.c | 3 +- .../net/ethernet/qlogic/qede/qede_filter.c | 3 -- .../net/ethernet/qlogic/qlcnic/qlcnic_main.c | 1 - drivers/net/ethernet/sfc/ef10.c | 1 - drivers/net/netdevsim/netdevsim.h | 2 - drivers/net/netdevsim/udp_tunnels.c | 12 ------ include/net/udp_tunnel.h | 8 ++-- net/core/dev.c | 2 - net/ipv4/udp_tunnel_nic.c | 30 +++++++------ .../drivers/net/netdevsim/udp_tunnel_nic.sh | 23 +--------- 17 files changed, 32 insertions(+), 109 deletions(-)
Drivers that are using ops lock and don't depend on RTNL lock still need to manage it because udp_tunnel's RTNL dependency. Introduce new udp_tunnel_nic_lock and use it instead of rtnl_lock. Drop non-UDP_TUNNEL_NIC_INFO_MAY_SLEEP mode from udp_tunnel infra (udp_tunnel_nic_device_sync_work needs to grab udp_tunnel_nic_lock mutex and might sleep).
Cc: Michael Chan michael.chan@broadcom.com Reviewed-by: Cosmin Ratiu cratiu@nvidia.com Suggested-by: Jakub Kicinski kuba@kernel.org Signed-off-by: Stanislav Fomichev stfomichev@gmail.com --- .../net/ethernet/broadcom/bnx2x/bnx2x_main.c | 3 +- drivers/net/ethernet/broadcom/bnxt/bnxt.c | 6 ++-- drivers/net/ethernet/emulex/benet/be_main.c | 3 +- drivers/net/ethernet/intel/i40e/i40e_main.c | 1 - drivers/net/ethernet/intel/ice/ice_main.c | 1 - .../net/ethernet/mellanox/mlx4/en_netdev.c | 3 +- .../net/ethernet/mellanox/mlx5/core/en_main.c | 3 +- .../ethernet/netronome/nfp/nfp_net_common.c | 3 +- .../net/ethernet/qlogic/qede/qede_filter.c | 3 -- .../net/ethernet/qlogic/qlcnic/qlcnic_main.c | 1 - drivers/net/ethernet/sfc/ef10.c | 1 - drivers/net/netdevsim/udp_tunnels.c | 4 --- include/net/udp_tunnel.h | 8 ++--- net/ipv4/udp_tunnel_nic.c | 30 +++++++++---------- 14 files changed, 24 insertions(+), 46 deletions(-)
diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c index f522ca8ff66b..fa7e5adf9804 100644 --- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c +++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c @@ -10219,8 +10219,7 @@ static int bnx2x_udp_tunnel_sync(struct net_device *netdev, unsigned int table)
static const struct udp_tunnel_nic_info bnx2x_udp_tunnels = { .sync_table = bnx2x_udp_tunnel_sync, - .flags = UDP_TUNNEL_NIC_INFO_MAY_SLEEP | - UDP_TUNNEL_NIC_INFO_OPEN_ONLY, + .flags = UDP_TUNNEL_NIC_INFO_OPEN_ONLY, .tables = { { .n_entries = 1, .tunnel_types = UDP_TUNNEL_TYPE_VXLAN, }, { .n_entries = 1, .tunnel_types = UDP_TUNNEL_TYPE_GENEVE, }, diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c index d5495762c945..a3dadde65b8d 100644 --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c @@ -15573,8 +15573,7 @@ static int bnxt_udp_tunnel_unset_port(struct net_device *netdev, unsigned int ta static const struct udp_tunnel_nic_info bnxt_udp_tunnels = { .set_port = bnxt_udp_tunnel_set_port, .unset_port = bnxt_udp_tunnel_unset_port, - .flags = UDP_TUNNEL_NIC_INFO_MAY_SLEEP | - UDP_TUNNEL_NIC_INFO_OPEN_ONLY, + .flags = UDP_TUNNEL_NIC_INFO_OPEN_ONLY, .tables = { { .n_entries = 1, .tunnel_types = UDP_TUNNEL_TYPE_VXLAN, }, { .n_entries = 1, .tunnel_types = UDP_TUNNEL_TYPE_GENEVE, }, @@ -15582,8 +15581,7 @@ static const struct udp_tunnel_nic_info bnxt_udp_tunnels = { }, bnxt_udp_tunnels_p7 = { .set_port = bnxt_udp_tunnel_set_port, .unset_port = bnxt_udp_tunnel_unset_port, - .flags = UDP_TUNNEL_NIC_INFO_MAY_SLEEP | - UDP_TUNNEL_NIC_INFO_OPEN_ONLY, + .flags = UDP_TUNNEL_NIC_INFO_OPEN_ONLY, .tables = { { .n_entries = 1, .tunnel_types = UDP_TUNNEL_TYPE_VXLAN, }, { .n_entries = 1, .tunnel_types = UDP_TUNNEL_TYPE_GENEVE, }, diff --git a/drivers/net/ethernet/emulex/benet/be_main.c b/drivers/net/ethernet/emulex/benet/be_main.c index 3d2e21592119..f49400ba9729 100644 --- a/drivers/net/ethernet/emulex/benet/be_main.c +++ b/drivers/net/ethernet/emulex/benet/be_main.c @@ -4031,8 +4031,7 @@ static int be_vxlan_unset_port(struct net_device *netdev, unsigned int table, static const struct udp_tunnel_nic_info be_udp_tunnels = { .set_port = be_vxlan_set_port, .unset_port = be_vxlan_unset_port, - .flags = UDP_TUNNEL_NIC_INFO_MAY_SLEEP | - UDP_TUNNEL_NIC_INFO_OPEN_ONLY, + .flags = UDP_TUNNEL_NIC_INFO_OPEN_ONLY, .tables = { { .n_entries = 1, .tunnel_types = UDP_TUNNEL_TYPE_VXLAN, }, }, diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c index 120d68654e3f..3d3da9d15348 100644 --- a/drivers/net/ethernet/intel/i40e/i40e_main.c +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c @@ -15891,7 +15891,6 @@ static int i40e_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
pf->udp_tunnel_nic.set_port = i40e_udp_tunnel_set_port; pf->udp_tunnel_nic.unset_port = i40e_udp_tunnel_unset_port; - pf->udp_tunnel_nic.flags = UDP_TUNNEL_NIC_INFO_MAY_SLEEP; pf->udp_tunnel_nic.shared = &pf->udp_tunnel_shared; pf->udp_tunnel_nic.tables[0].n_entries = I40E_MAX_PF_UDP_OFFLOAD_PORTS; pf->udp_tunnel_nic.tables[0].tunnel_types = UDP_TUNNEL_TYPE_VXLAN | diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c index d97d4b25b30d..e309531f8c9d 100644 --- a/drivers/net/ethernet/intel/ice/ice_main.c +++ b/drivers/net/ethernet/intel/ice/ice_main.c @@ -4764,7 +4764,6 @@ int ice_init_dev(struct ice_pf *pf)
pf->hw.udp_tunnel_nic.set_port = ice_udp_tunnel_set_port; pf->hw.udp_tunnel_nic.unset_port = ice_udp_tunnel_unset_port; - pf->hw.udp_tunnel_nic.flags = UDP_TUNNEL_NIC_INFO_MAY_SLEEP; pf->hw.udp_tunnel_nic.shared = &pf->hw.udp_tunnel_shared; if (pf->hw.tnl.valid_count[TNL_VXLAN]) { pf->hw.udp_tunnel_nic.tables[0].n_entries = diff --git a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c index 281b34af0bb4..d2071aff7b8f 100644 --- a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c +++ b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c @@ -2670,8 +2670,7 @@ static int mlx4_udp_tunnel_sync(struct net_device *dev, unsigned int table)
static const struct udp_tunnel_nic_info mlx4_udp_tunnels = { .sync_table = mlx4_udp_tunnel_sync, - .flags = UDP_TUNNEL_NIC_INFO_MAY_SLEEP | - UDP_TUNNEL_NIC_INFO_IPV4_ONLY, + .flags = UDP_TUNNEL_NIC_INFO_IPV4_ONLY, .tables = { { .n_entries = 1, .tunnel_types = UDP_TUNNEL_TYPE_VXLAN, }, }, diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c index ea822c69d137..ccde53f94045 100644 --- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c @@ -5303,8 +5303,7 @@ void mlx5e_vxlan_set_netdev_info(struct mlx5e_priv *priv)
priv->nic_info.set_port = mlx5e_vxlan_set_port; priv->nic_info.unset_port = mlx5e_vxlan_unset_port; - priv->nic_info.flags = UDP_TUNNEL_NIC_INFO_MAY_SLEEP | - UDP_TUNNEL_NIC_INFO_STATIC_IANA_VXLAN; + priv->nic_info.flags = UDP_TUNNEL_NIC_INFO_STATIC_IANA_VXLAN; priv->nic_info.tables[0].tunnel_types = UDP_TUNNEL_TYPE_VXLAN; /* Don't count the space hard-coded to the IANA port */ priv->nic_info.tables[0].n_entries = diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net_common.c b/drivers/net/ethernet/netronome/nfp/nfp_net_common.c index 28997ddab966..14ba38bcb07b 100644 --- a/drivers/net/ethernet/netronome/nfp/nfp_net_common.c +++ b/drivers/net/ethernet/netronome/nfp/nfp_net_common.c @@ -2394,8 +2394,7 @@ static int nfp_udp_tunnel_sync(struct net_device *netdev, unsigned int table)
static const struct udp_tunnel_nic_info nfp_udp_tunnels = { .sync_table = nfp_udp_tunnel_sync, - .flags = UDP_TUNNEL_NIC_INFO_MAY_SLEEP | - UDP_TUNNEL_NIC_INFO_OPEN_ONLY, + .flags = UDP_TUNNEL_NIC_INFO_OPEN_ONLY, .tables = { { .n_entries = NFP_NET_N_VXLAN_PORTS, diff --git a/drivers/net/ethernet/qlogic/qede/qede_filter.c b/drivers/net/ethernet/qlogic/qede/qede_filter.c index 985026dd816f..7e341e026489 100644 --- a/drivers/net/ethernet/qlogic/qede/qede_filter.c +++ b/drivers/net/ethernet/qlogic/qede/qede_filter.c @@ -987,20 +987,17 @@ static int qede_udp_tunnel_sync(struct net_device *dev, unsigned int table)
static const struct udp_tunnel_nic_info qede_udp_tunnels_both = { .sync_table = qede_udp_tunnel_sync, - .flags = UDP_TUNNEL_NIC_INFO_MAY_SLEEP, .tables = { { .n_entries = 1, .tunnel_types = UDP_TUNNEL_TYPE_VXLAN, }, { .n_entries = 1, .tunnel_types = UDP_TUNNEL_TYPE_GENEVE, }, }, }, qede_udp_tunnels_vxlan = { .sync_table = qede_udp_tunnel_sync, - .flags = UDP_TUNNEL_NIC_INFO_MAY_SLEEP, .tables = { { .n_entries = 1, .tunnel_types = UDP_TUNNEL_TYPE_VXLAN, }, }, }, qede_udp_tunnels_geneve = { .sync_table = qede_udp_tunnel_sync, - .flags = UDP_TUNNEL_NIC_INFO_MAY_SLEEP, .tables = { { .n_entries = 1, .tunnel_types = UDP_TUNNEL_TYPE_GENEVE, }, }, diff --git a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c index eb69121df726..53cdd36c4123 100644 --- a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c +++ b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c @@ -486,7 +486,6 @@ static int qlcnic_udp_tunnel_sync(struct net_device *dev, unsigned int table)
static const struct udp_tunnel_nic_info qlcnic_udp_tunnels = { .sync_table = qlcnic_udp_tunnel_sync, - .flags = UDP_TUNNEL_NIC_INFO_MAY_SLEEP, .tables = { { .n_entries = 1, .tunnel_types = UDP_TUNNEL_TYPE_VXLAN, }, }, diff --git a/drivers/net/ethernet/sfc/ef10.c b/drivers/net/ethernet/sfc/ef10.c index 47349c148c0c..fcec81f862ec 100644 --- a/drivers/net/ethernet/sfc/ef10.c +++ b/drivers/net/ethernet/sfc/ef10.c @@ -3985,7 +3985,6 @@ static int efx_ef10_udp_tnl_unset_port(struct net_device *dev, static const struct udp_tunnel_nic_info efx_ef10_udp_tunnels = { .set_port = efx_ef10_udp_tnl_set_port, .unset_port = efx_ef10_udp_tnl_unset_port, - .flags = UDP_TUNNEL_NIC_INFO_MAY_SLEEP, .tables = { { .n_entries = 16, diff --git a/drivers/net/netdevsim/udp_tunnels.c b/drivers/net/netdevsim/udp_tunnels.c index 640b4983a9a0..10cbbf1c584b 100644 --- a/drivers/net/netdevsim/udp_tunnels.c +++ b/drivers/net/netdevsim/udp_tunnels.c @@ -112,12 +112,10 @@ nsim_udp_tunnels_info_reset_write(struct file *file, const char __user *data, struct net_device *dev = file->private_data; struct netdevsim *ns = netdev_priv(dev);
- rtnl_lock(); if (dev->reg_state == NETREG_REGISTERED) { memset(ns->udp_ports.ports, 0, sizeof(ns->udp_ports.__ports)); udp_tunnel_nic_reset_ntf(dev); } - rtnl_unlock();
return count; } @@ -181,8 +179,6 @@ int nsim_udp_tunnels_info_create(struct nsim_dev *nsim_dev, info->sync_table = NULL; }
- if (ns->udp_ports.sleep) - info->flags |= UDP_TUNNEL_NIC_INFO_MAY_SLEEP; if (nsim_dev->udp_ports.open_only) info->flags |= UDP_TUNNEL_NIC_INFO_OPEN_ONLY; if (nsim_dev->udp_ports.ipv4_only) diff --git a/include/net/udp_tunnel.h b/include/net/udp_tunnel.h index 2df3b8344eb5..606168b0b64a 100644 --- a/include/net/udp_tunnel.h +++ b/include/net/udp_tunnel.h @@ -221,19 +221,17 @@ static inline void udp_tunnel_encap_enable(struct sock *sk) #define UDP_TUNNEL_NIC_MAX_TABLES 4
enum udp_tunnel_nic_info_flags { - /* Device callbacks may sleep */ - UDP_TUNNEL_NIC_INFO_MAY_SLEEP = BIT(0), /* Device only supports offloads when it's open, all ports * will be removed before close and re-added after open. */ - UDP_TUNNEL_NIC_INFO_OPEN_ONLY = BIT(1), + UDP_TUNNEL_NIC_INFO_OPEN_ONLY = BIT(0), /* Device supports only IPv4 tunnels */ - UDP_TUNNEL_NIC_INFO_IPV4_ONLY = BIT(2), + UDP_TUNNEL_NIC_INFO_IPV4_ONLY = BIT(1), /* Device has hard-coded the IANA VXLAN port (4789) as VXLAN. * This port must not be counted towards n_entries of any table. * Driver will not receive any callback associated with port 4789. */ - UDP_TUNNEL_NIC_INFO_STATIC_IANA_VXLAN = BIT(3), + UDP_TUNNEL_NIC_INFO_STATIC_IANA_VXLAN = BIT(2), };
struct udp_tunnel_nic; diff --git a/net/ipv4/udp_tunnel_nic.c b/net/ipv4/udp_tunnel_nic.c index b6d2d16189c0..956a04c4bed4 100644 --- a/net/ipv4/udp_tunnel_nic.c +++ b/net/ipv4/udp_tunnel_nic.c @@ -34,6 +34,7 @@ struct udp_tunnel_nic_table_entry { * @work_pending: @work is currently scheduled * @n_tables: number of tables under @entries * @missed: bitmap of tables which overflown + * @lock: protects entries * @entries: table of tables of ports currently offloaded */ struct udp_tunnel_nic { @@ -47,6 +48,7 @@ struct udp_tunnel_nic {
unsigned int n_tables; unsigned long missed; + struct mutex lock; struct udp_tunnel_nic_table_entry *entries[] __counted_by(n_tables); };
@@ -298,22 +300,11 @@ __udp_tunnel_nic_device_sync(struct net_device *dev, struct udp_tunnel_nic *utn) static void udp_tunnel_nic_device_sync(struct net_device *dev, struct udp_tunnel_nic *utn) { - const struct udp_tunnel_nic_info *info = dev->udp_tunnel_nic_info; - bool may_sleep; - if (!utn->need_sync) return;
- /* Drivers which sleep in the callback need to update from - * the workqueue, if we come from the tunnel driver's notification. - */ - may_sleep = info->flags & UDP_TUNNEL_NIC_INFO_MAY_SLEEP; - if (!may_sleep) - __udp_tunnel_nic_device_sync(dev, utn); - if (may_sleep || utn->need_replay) { - queue_work(udp_tunnel_nic_workqueue, &utn->work); - utn->work_pending = 1; - } + queue_work(udp_tunnel_nic_workqueue, &utn->work); + utn->work_pending = 1; }
static bool @@ -554,12 +545,12 @@ static void __udp_tunnel_nic_reset_ntf(struct net_device *dev) struct udp_tunnel_nic *utn; unsigned int i, j;
- ASSERT_RTNL(); - utn = dev->udp_tunnel_nic; if (!utn) return;
+ mutex_lock(&utn->lock); + utn->need_sync = false; for (i = 0; i < utn->n_tables; i++) for (j = 0; j < info->tables[i].n_entries; j++) { @@ -569,7 +560,7 @@ static void __udp_tunnel_nic_reset_ntf(struct net_device *dev)
entry->flags &= ~(UDP_TUNNEL_NIC_ENTRY_DEL | UDP_TUNNEL_NIC_ENTRY_OP_FAIL); - /* We don't release rtnl across ops */ + /* We don't release utn lock across ops */ WARN_ON(entry->flags & UDP_TUNNEL_NIC_ENTRY_FROZEN); if (!entry->use_cnt) continue; @@ -579,6 +570,8 @@ static void __udp_tunnel_nic_reset_ntf(struct net_device *dev) }
__udp_tunnel_nic_device_sync(dev, utn); + + mutex_unlock(&utn->lock); }
static size_t @@ -710,11 +703,15 @@ static void udp_tunnel_nic_device_sync_work(struct work_struct *work) container_of(work, struct udp_tunnel_nic, work);
rtnl_lock(); + mutex_lock(&utn->lock); + utn->work_pending = 0; __udp_tunnel_nic_device_sync(utn->dev, utn);
if (utn->need_replay) udp_tunnel_nic_replay(utn->dev, utn); + + mutex_unlock(&utn->lock); rtnl_unlock(); }
@@ -730,6 +727,7 @@ udp_tunnel_nic_alloc(const struct udp_tunnel_nic_info *info, return NULL; utn->n_tables = n_tables; INIT_WORK(&utn->work, udp_tunnel_nic_device_sync_work); + mutex_init(&utn->lock);
for (i = 0; i < n_tables; i++) { utn->entries[i] = kcalloc(info->tables[i].n_entries,
On Tue, 10 Jun 2025 10:15:19 -0700 Stanislav Fomichev wrote:
Drivers that are using ops lock and don't depend on RTNL lock still need to manage it because udp_tunnel's RTNL dependency. Introduce new udp_tunnel_nic_lock and use it instead of rtnl_lock. Drop non-UDP_TUNNEL_NIC_INFO_MAY_SLEEP mode from udp_tunnel infra (udp_tunnel_nic_device_sync_work needs to grab udp_tunnel_nic_lock mutex and might sleep).
There are multiple entry points to this code, basically each member of struct udp_tunnel_nic_ops and the netdev notifiers. In this patch only reset and work are locked. I'm a bit confused as to what is the new lock protecting :S
On 06/11, Jakub Kicinski wrote:
On Tue, 10 Jun 2025 10:15:19 -0700 Stanislav Fomichev wrote:
Drivers that are using ops lock and don't depend on RTNL lock still need to manage it because udp_tunnel's RTNL dependency. Introduce new udp_tunnel_nic_lock and use it instead of rtnl_lock. Drop non-UDP_TUNNEL_NIC_INFO_MAY_SLEEP mode from udp_tunnel infra (udp_tunnel_nic_device_sync_work needs to grab udp_tunnel_nic_lock mutex and might sleep).
There are multiple entry points to this code, basically each member of struct udp_tunnel_nic_ops and the netdev notifiers. In this patch only reset and work are locked. I'm a bit confused as to what is the new lock protecting :S
I though that most of the callers are from do_setlink and there we have rtnl and we grab rtnl+lock during the sync. But that doesn't address the suspend/resume vs do_setlink race, that's true :-(
Did not look deep into the notifiers, assuming they are a way to push the info down to the devices (under rtnl) plus trigger the sync work, will take a closer look.
On Wed, 11 Jun 2025 19:47:54 -0700 Stanislav Fomichev wrote:
There are multiple entry points to this code, basically each member of struct udp_tunnel_nic_ops and the netdev notifiers. In this patch only reset and work are locked. I'm a bit confused as to what is the new lock protecting :S
I though that most of the callers are from do_setlink and there we have rtnl and we grab rtnl+lock during the sync. But that doesn't address the suspend/resume vs do_setlink race, that's true :-(
It's the UDP tunnels that add and remove the ports usually.
The existing netdev_ops_assert_locked() already asserts that either the RTNL lock or the per-device lock is held, making the explicit ASSERT_RTNL() redundant.
Cc: Michael Chan michael.chan@broadcom.com Signed-off-by: Stanislav Fomichev stfomichev@gmail.com --- net/core/dev.c | 2 -- 1 file changed, 2 deletions(-)
diff --git a/net/core/dev.c b/net/core/dev.c index be97c440ecd5..72997636b8ec 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -3179,7 +3179,6 @@ int netif_set_real_num_tx_queues(struct net_device *dev, unsigned int txq)
if (dev->reg_state == NETREG_REGISTERED || dev->reg_state == NETREG_UNREGISTERING) { - ASSERT_RTNL(); netdev_ops_assert_locked(dev);
rc = netdev_queue_update_kobjects(dev, dev->real_num_tx_queues, @@ -3229,7 +3228,6 @@ int netif_set_real_num_rx_queues(struct net_device *dev, unsigned int rxq) return -EINVAL;
if (dev->reg_state == NETREG_REGISTERED) { - ASSERT_RTNL(); netdev_ops_assert_locked(dev);
rc = net_rx_queue_update_kobjects(dev, dev->real_num_rx_queues,
-----Original Message----- From: Intel-wired-lan intel-wired-lan-bounces@osuosl.org On Behalf Of Stanislav Fomichev Sent: Tuesday, June 10, 2025 7:15 PM To: netdev@vger.kernel.org Cc: davem@davemloft.net; edumazet@google.com; kuba@kernel.org; pabeni@redhat.com; skalluru@marvell.com; manishc@marvell.com; andrew+netdev@lunn.ch; michael.chan@broadcom.com; pavan.chebbi@broadcom.com; ajit.khaparde@broadcom.com; sriharsha.basavapatna@broadcom.com; somnath.kotur@broadcom.com; Nguyen, Anthony L anthony.l.nguyen@intel.com; Kitszel, Przemyslaw przemyslaw.kitszel@intel.com; tariqt@nvidia.com; saeedm@nvidia.com; louis.peens@corigine.com; shshaikh@marvell.com; GR-Linux-NIC- Dev@marvell.com; ecree.xilinx@gmail.com; horms@kernel.org; dsahern@kernel.org; shuah@kernel.org; ruanjinjie@huawei.com; mheib@redhat.com; stfomichev@gmail.com; linux-kernel@vger.kernel.org; intel-wired-lan@lists.osuosl.org; linux-rdma@vger.kernel.org; oss- drivers@corigine.com; linux-net-drivers@amd.com; linux- kselftest@vger.kernel.org; leon@kernel.org Subject: [Intel-wired-lan] [PATCH net-next v3 2/4] net: remove redundant ASSERT_RTNL() in queue setup functions
The existing netdev_ops_assert_locked() already asserts that either the RTNL lock or the per-device lock is held, making the explicit ASSERT_RTNL() redundant.
Cc: Michael Chan michael.chan@broadcom.com Signed-off-by: Stanislav Fomichev stfomichev@gmail.com
Reviewed-by: Aleksandr Loktionov aleksandr.loktionov@intel.com
net/core/dev.c | 2 -- 1 file changed, 2 deletions(-)
diff --git a/net/core/dev.c b/net/core/dev.c index be97c440ecd5..72997636b8ec 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -3179,7 +3179,6 @@ int netif_set_real_num_tx_queues(struct net_device *dev, unsigned int txq)
if (dev->reg_state == NETREG_REGISTERED || dev->reg_state == NETREG_UNREGISTERING) {
ASSERT_RTNL();netdev_ops_assert_locked(dev);
rc = netdev_queue_update_kobjects(dev, dev-
real_num_tx_queues, @@ -3229,7 +3228,6 @@ int
netif_set_real_num_rx_queues(struct net_device *dev, unsigned int rxq) return -EINVAL;
if (dev->reg_state == NETREG_REGISTERED) {
ASSERT_RTNL();netdev_ops_assert_locked(dev);
rc = net_rx_queue_update_kobjects(dev, dev-
real_num_rx_queues,
-- 2.49.0
Now that there is only one path in udp_tunnel, there is no need to have udp_ports_sleep knob. Remove it and adjust the test.
Cc: Michael Chan michael.chan@broadcom.com Reviewed-by: Aleksandr Loktionov aleksandr.loktionov@intel.com Signed-off-by: Stanislav Fomichev stfomichev@gmail.com --- drivers/net/netdevsim/netdevsim.h | 2 -- drivers/net/netdevsim/udp_tunnels.c | 8 ------- .../drivers/net/netdevsim/udp_tunnel_nic.sh | 23 +------------------ 3 files changed, 1 insertion(+), 32 deletions(-)
diff --git a/drivers/net/netdevsim/netdevsim.h b/drivers/net/netdevsim/netdevsim.h index d04401f0bdf7..511ed72a93ce 100644 --- a/drivers/net/netdevsim/netdevsim.h +++ b/drivers/net/netdevsim/netdevsim.h @@ -131,7 +131,6 @@ struct netdevsim { struct nsim_macsec macsec; struct { u32 inject_error; - u32 sleep; u32 __ports[2][NSIM_UDP_TUNNEL_N_PORTS]; u32 (*ports)[NSIM_UDP_TUNNEL_N_PORTS]; struct dentry *ddir; @@ -342,7 +341,6 @@ struct nsim_dev { bool ipv4_only; bool shared; bool static_iana_vxlan; - u32 sleep; } udp_ports; struct nsim_dev_psample *psample; u16 esw_mode; diff --git a/drivers/net/netdevsim/udp_tunnels.c b/drivers/net/netdevsim/udp_tunnels.c index 10cbbf1c584b..89fff76e51cf 100644 --- a/drivers/net/netdevsim/udp_tunnels.c +++ b/drivers/net/netdevsim/udp_tunnels.c @@ -18,9 +18,6 @@ nsim_udp_tunnel_set_port(struct net_device *dev, unsigned int table, ret = -ns->udp_ports.inject_error; ns->udp_ports.inject_error = 0;
- if (ns->udp_ports.sleep) - msleep(ns->udp_ports.sleep); - if (!ret) { if (ns->udp_ports.ports[table][entry]) { WARN(1, "entry already in use\n"); @@ -47,8 +44,6 @@ nsim_udp_tunnel_unset_port(struct net_device *dev, unsigned int table, ret = -ns->udp_ports.inject_error; ns->udp_ports.inject_error = 0;
- if (ns->udp_ports.sleep) - msleep(ns->udp_ports.sleep); if (!ret) { u32 val = be16_to_cpu(ti->port) << 16 | ti->type;
@@ -170,7 +165,6 @@ int nsim_udp_tunnels_info_create(struct nsim_dev *nsim_dev, GFP_KERNEL); if (!info) return -ENOMEM; - ns->udp_ports.sleep = nsim_dev->udp_ports.sleep;
if (nsim_dev->udp_ports.sync_all) { info->set_port = NULL; @@ -213,6 +207,4 @@ void nsim_udp_tunnels_debugfs_create(struct nsim_dev *nsim_dev) &nsim_dev->udp_ports.shared); debugfs_create_bool("udp_ports_static_iana_vxlan", 0600, nsim_dev->ddir, &nsim_dev->udp_ports.static_iana_vxlan); - debugfs_create_u32("udp_ports_sleep", 0600, nsim_dev->ddir, - &nsim_dev->udp_ports.sleep); } diff --git a/tools/testing/selftests/drivers/net/netdevsim/udp_tunnel_nic.sh b/tools/testing/selftests/drivers/net/netdevsim/udp_tunnel_nic.sh index 92c2f0376c08..4c859ecdad94 100755 --- a/tools/testing/selftests/drivers/net/netdevsim/udp_tunnel_nic.sh +++ b/tools/testing/selftests/drivers/net/netdevsim/udp_tunnel_nic.sh @@ -266,7 +266,6 @@ for port in 0 1; do echo $NSIM_ID > /sys/bus/netdevsim/new_device else echo 1 > $NSIM_DEV_DFS/udp_ports_open_only - echo 1 > $NSIM_DEV_DFS/udp_ports_sleep echo 1 > $NSIM_DEV_SYS/new_port fi NSIM_NETDEV=`get_netdev_name old_netdevs` @@ -350,23 +349,11 @@ old_netdevs=$(ls /sys/class/net) port=0 echo $NSIM_ID > /sys/bus/netdevsim/new_device echo 0 > $NSIM_DEV_SYS/del_port -echo 1000 > $NSIM_DEV_DFS/udp_ports_sleep echo 0 > $NSIM_DEV_SYS/new_port NSIM_NETDEV=`get_netdev_name old_netdevs`
msg="create VxLANs" -exp0=( 0 0 0 0 ) # sleep is longer than out wait -new_vxlan vxlan0 10000 $NSIM_NETDEV - -modprobe -r vxlan -modprobe -r udp_tunnel - -msg="remove tunnels" -exp0=( 0 0 0 0 ) -check_tables - -msg="create VxLANs" -exp0=( 0 0 0 0 ) # sleep is longer than out wait +exp0=( `mke 10000 1` 0 0 0 ) new_vxlan vxlan0 10000 $NSIM_NETDEV
exp0=( 0 0 0 0 ) @@ -428,7 +415,6 @@ echo 0 > $NSIM_DEV_SYS/del_port for port in 0 1; do if [ $port -ne 0 ]; then echo 1 > $NSIM_DEV_DFS/udp_ports_open_only - echo 1 > $NSIM_DEV_DFS/udp_ports_sleep fi
echo $port > $NSIM_DEV_SYS/new_port @@ -486,7 +472,6 @@ echo 1 > $NSIM_DEV_DFS/udp_ports_sync_all for port in 0 1; do if [ $port -ne 0 ]; then echo 1 > $NSIM_DEV_DFS/udp_ports_open_only - echo 1 > $NSIM_DEV_DFS/udp_ports_sleep fi
echo $port > $NSIM_DEV_SYS/new_port @@ -543,7 +528,6 @@ echo 0 > $NSIM_DEV_SYS/del_port for port in 0 1; do if [ $port -ne 0 ]; then echo 1 > $NSIM_DEV_DFS/udp_ports_open_only - echo 1 > $NSIM_DEV_DFS/udp_ports_sleep fi
echo $port > $NSIM_DEV_SYS/new_port @@ -573,7 +557,6 @@ echo 1 > $NSIM_DEV_DFS/udp_ports_ipv4_only for port in 0 1; do if [ $port -ne 0 ]; then echo 1 > $NSIM_DEV_DFS/udp_ports_open_only - echo 1 > $NSIM_DEV_DFS/udp_ports_sleep fi
echo $port > $NSIM_DEV_SYS/new_port @@ -634,7 +617,6 @@ echo 0 > $NSIM_DEV_SYS/del_port for port in 0 1; do if [ $port -ne 0 ]; then echo 1 > $NSIM_DEV_DFS/udp_ports_open_only - echo 1 > $NSIM_DEV_DFS/udp_ports_sleep fi
echo $port > $NSIM_DEV_SYS/new_port @@ -690,7 +672,6 @@ echo 0 > $NSIM_DEV_SYS/del_port for port in 0 1; do if [ $port -ne 0 ]; then echo 1 > $NSIM_DEV_DFS/udp_ports_open_only - echo 1 > $NSIM_DEV_DFS/udp_ports_sleep fi
echo $port > $NSIM_DEV_SYS/new_port @@ -750,7 +731,6 @@ echo 0 > $NSIM_DEV_SYS/del_port for port in 0 1; do if [ $port -ne 0 ]; then echo 1 > $NSIM_DEV_DFS/udp_ports_open_only - echo 1 > $NSIM_DEV_DFS/udp_ports_sleep fi
echo $port > $NSIM_DEV_SYS/new_port @@ -809,7 +789,6 @@ echo $NSIM_ID > /sys/bus/netdevsim/new_device echo 0 > $NSIM_DEV_SYS/del_port
echo 0 > $NSIM_DEV_DFS/udp_ports_open_only -echo 1 > $NSIM_DEV_DFS/udp_ports_sleep echo 1 > $NSIM_DEV_DFS/udp_ports_shared
old_netdevs=$(ls /sys/class/net)
This reverts commit 325eb217e41fa14f307c7cc702bd18d0bb38fe84.
udp_tunnel infra doesn't need RTNL, should be safe to get back to only netdev instance lock.
Cc: Michael Chan michael.chan@broadcom.com Reviewed-by: Aleksandr Loktionov aleksandr.loktionov@intel.com Signed-off-by: Stanislav Fomichev stfomichev@gmail.com --- drivers/net/ethernet/broadcom/bnxt/bnxt.c | 36 +++++------------------ 1 file changed, 7 insertions(+), 29 deletions(-)
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c index a3dadde65b8d..1da208c36572 100644 --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c @@ -14055,28 +14055,13 @@ static void bnxt_unlock_sp(struct bnxt *bp) netdev_unlock(bp->dev); }
-/* Same as bnxt_lock_sp() with additional rtnl_lock */ -static void bnxt_rtnl_lock_sp(struct bnxt *bp) -{ - clear_bit(BNXT_STATE_IN_SP_TASK, &bp->state); - rtnl_lock(); - netdev_lock(bp->dev); -} - -static void bnxt_rtnl_unlock_sp(struct bnxt *bp) -{ - set_bit(BNXT_STATE_IN_SP_TASK, &bp->state); - netdev_unlock(bp->dev); - rtnl_unlock(); -} - /* Only called from bnxt_sp_task() */ static void bnxt_reset(struct bnxt *bp, bool silent) { - bnxt_rtnl_lock_sp(bp); + bnxt_lock_sp(bp); if (test_bit(BNXT_STATE_OPEN, &bp->state)) bnxt_reset_task(bp, silent); - bnxt_rtnl_unlock_sp(bp); + bnxt_unlock_sp(bp); }
/* Only called from bnxt_sp_task() */ @@ -14084,9 +14069,9 @@ static void bnxt_rx_ring_reset(struct bnxt *bp) { int i;
- bnxt_rtnl_lock_sp(bp); + bnxt_lock_sp(bp); if (!test_bit(BNXT_STATE_OPEN, &bp->state)) { - bnxt_rtnl_unlock_sp(bp); + bnxt_unlock_sp(bp); return; } /* Disable and flush TPA before resetting the RX ring */ @@ -14125,7 +14110,7 @@ static void bnxt_rx_ring_reset(struct bnxt *bp) } if (bp->flags & BNXT_FLAG_TPA) bnxt_set_tpa(bp, true); - bnxt_rtnl_unlock_sp(bp); + bnxt_unlock_sp(bp); }
static void bnxt_fw_fatal_close(struct bnxt *bp) @@ -15017,17 +15002,15 @@ static void bnxt_fw_reset_task(struct work_struct *work) bp->fw_reset_state = BNXT_FW_RESET_STATE_OPENING; fallthrough; case BNXT_FW_RESET_STATE_OPENING: - while (!rtnl_trylock()) { + while (!netdev_trylock(bp->dev)) { bnxt_queue_fw_reset_work(bp, HZ / 10); return; } - netdev_lock(bp->dev); rc = bnxt_open(bp->dev); if (rc) { netdev_err(bp->dev, "bnxt_open() failed during FW reset\n"); bnxt_fw_reset_abort(bp, rc); netdev_unlock(bp->dev); - rtnl_unlock(); goto ulp_start; }
@@ -15047,7 +15030,6 @@ static void bnxt_fw_reset_task(struct work_struct *work) bnxt_dl_health_fw_status_update(bp, true); } netdev_unlock(bp->dev); - rtnl_unlock(); bnxt_ulp_start(bp, 0); bnxt_reenable_sriov(bp); netdev_lock(bp->dev); @@ -15996,7 +15978,7 @@ static int bnxt_queue_start(struct net_device *dev, void *qmem, int idx) rc); napi_enable_locked(&bnapi->napi); bnxt_db_nq_arm(bp, &cpr->cp_db, cpr->cp_raw_cons); - netif_close(dev); + bnxt_reset_task(bp, true); return rc; }
@@ -16812,7 +16794,6 @@ static int bnxt_resume(struct device *device) struct bnxt *bp = netdev_priv(dev); int rc = 0;
- rtnl_lock(); netdev_lock(dev); rc = pci_enable_device(bp->pdev); if (rc) { @@ -16857,7 +16838,6 @@ static int bnxt_resume(struct device *device)
resume_exit: netdev_unlock(bp->dev); - rtnl_unlock(); bnxt_ulp_start(bp, rc); if (!rc) bnxt_reenable_sriov(bp); @@ -17023,7 +17003,6 @@ static void bnxt_io_resume(struct pci_dev *pdev) int err;
netdev_info(bp->dev, "PCI Slot Resume\n"); - rtnl_lock(); netdev_lock(netdev);
err = bnxt_hwrm_func_qcaps(bp); @@ -17041,7 +17020,6 @@ static void bnxt_io_resume(struct pci_dev *pdev) netif_device_attach(netdev);
netdev_unlock(netdev); - rtnl_unlock(); bnxt_ulp_start(bp, err); if (!err) bnxt_reenable_sriov(bp);
linux-kselftest-mirror@lists.linaro.org
-
Jakub Kicinski -
Loktionov, Aleksandr -
Stanislav Fomichev