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.
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 | 10 ----- 17 files changed, 31 insertions(+), 97 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 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 Mon, 2025-06-09 at 09:25 -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).
Cc: Michael Chan michael.chan@broadcom.com Suggested-by: Jakub Kicinski kuba@kernel.org Signed-off-by: Stanislav Fomichev stfomichev@gmail.com
Reviewed-by: Cosmin Ratiu cratiu@nvidia.com
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,
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 Signed-off-by: Stanislav Fomichev stfomichev@gmail.com --- drivers/net/netdevsim/netdevsim.h | 2 -- drivers/net/netdevsim/udp_tunnels.c | 8 -------- .../selftests/drivers/net/netdevsim/udp_tunnel_nic.sh | 10 ---------- 3 files changed, 20 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..8c5fe7bdf1ce 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,7 +349,6 @@ 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`
@@ -428,7 +426,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 +483,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 +539,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 +568,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 +628,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 +683,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 +742,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 +800,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)
-----Original Message----- From: Intel-wired-lan intel-wired-lan-bounces@osuosl.org On Behalf Of Stanislav Fomichev Sent: Monday, June 9, 2025 6:26 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; mheib@redhat.com; ruanjinjie@huawei.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 v2 3/4] netdevsim: remove udp_ports_sleep
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 Signed-off-by: Stanislav Fomichev stfomichev@gmail.com
Reviewed-by: Aleksandr Loktionov aleksandr.loktionov@intel.com
drivers/net/netdevsim/netdevsim.h | 2 -- drivers/net/netdevsim/udp_tunnels.c | 8 -------- .../selftests/drivers/net/netdevsim/udp_tunnel_nic.sh | 10 ---------
3 files changed, 20 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;@@ -342,7 +341,6 @@ struct nsim_dev { bool ipv4_only; bool shared; bool static_iana_vxlan;
u32 sleep;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);@@ -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..8c5fe7bdf1ce 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,7 +349,6 @@
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`
@@ -428,7 +426,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 +483,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 +539,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 +568,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 +628,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 +683,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 +742,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 +800,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)
2.49.0
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);
On Mon, 9 Jun 2025 09:25:37 -0700 Stanislav Fomichev wrote:
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.
Appears to break the selftest, unfortunately: https://netdev.bots.linux.dev/contest.html?test=udp-tunnel-nic-sh&branch...
On 06/09, Jakub Kicinski wrote:
On Mon, 9 Jun 2025 09:25:37 -0700 Stanislav Fomichev wrote:
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.
Appears to break the selftest, unfortunately: https://netdev.bots.linux.dev/contest.html?test=udp-tunnel-nic-sh&branch...
Argh, should have run it locally first :-( Looks like there is a test that sets up pretty high sleep time (1 sec) and expects entry to not appear during next 'ethtool --show-tunnels' run.
Gonna double check and remove the case if my understanding is correct. Don't think there is much value in keeping the debugfs knob just for the sake of this test? LMK if you disagree; otherwise gonna repost tomorrow.
On Mon, 9 Jun 2025 16:19:34 -0700 Stanislav Fomichev wrote:
On 06/09, Jakub Kicinski wrote:
On Mon, 9 Jun 2025 09:25:37 -0700 Stanislav Fomichev wrote:
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.
Appears to break the selftest, unfortunately: https://netdev.bots.linux.dev/contest.html?test=udp-tunnel-nic-sh&branch...
Argh, should have run it locally first :-( Looks like there is a test that sets up pretty high sleep time (1 sec) and expects entry to not appear during next 'ethtool --show-tunnels' run.
Gonna double check and remove the case if my understanding is correct. Don't think there is much value in keeping the debugfs knob just for the sake of this test? LMK if you disagree; otherwise gonna repost tomorrow.
Hm, I see you partially deleted the sleep support in patch 3. Maybe it's easier to keep it since we now always sleep? No strong preference, tho.
linux-kselftest-mirror@lists.linaro.org
-
Cosmin Ratiu -
Jakub Kicinski -
Loktionov, Aleksandr -
Stanislav Fomichev