[PATCH net-next 0/2] net/mlx5e: Batch RCU grace periods on channel open and close

[PATCH net-next 0/2] net/mlx5e: Batch RCU grace periods on channel open and close

[Tariq Toukan]

Hi,

This series by Carolina reduces the time mlx5e spends waiting for RCU
grace periods when bringing an interface up or down with many channels.

Today the channel open and close flows call synchronize_net() per
object. On close, RQ, ICOSQ, TX SQ and NAPI teardown each require a
grace period, so the number of waits scales with the number of channels.
TX SQ teardown adds a wait per traffic class, and XDP and QoS can add
more. On activation, mlx5e_activate_channels() triggers the ICOSQ of
every channel, and each trigger waits for its own grace period. With a
large channel count this makes link down and link up slow.

Patch 1 batches the waits on channel close. Channel deactivate and close
are split into pre_sync and post_sync phases: the ENABLED state is first
cleared on all queues of all channels, a single grace period is awaited,
and only then does the teardown continue. NAPI instances are likewise
unlinked with __netif_napi_del_locked(), followed by one
synchronize_net() before the channels are freed. The existing per object
synchronize_net() calls are kept for single queue paths. The change
touches the regular channels as well as the PTP, QoS and XSK queues.

Patch 2 does the same for the ICOSQ trigger on activation: all channel
ICOSQs are first marked as requiring locking, one grace period is
awaited, and then all of them are triggered. mlx5e_trigger_napi_icosq()
keeps its existing semantics for standalone callers.

Results with 248 channels and rcu_normal=1:
- ip link set eth2 down: 10.183s before, 0.715s after.
- ip link set eth2 up: 3.095s before, 1.794s after.

Regards,
Tariq

Carolina Jubran (2):
  net/mlx5e: Batch NAPI RCU grace periods on channel close
  net/mlx5e: Batch ICOSQ trigger RCU synchronization

 drivers/net/ethernet/mellanox/mlx5/core/en.h  |   5 +
 .../net/ethernet/mellanox/mlx5/core/en/ptp.c  |  19 +-
 .../net/ethernet/mellanox/mlx5/core/en/ptp.h  |   4 +-
 .../net/ethernet/mellanox/mlx5/core/en/qos.c  |  69 +++++-
 .../net/ethernet/mellanox/mlx5/core/en/qos.h  |   4 +-
 .../net/ethernet/mellanox/mlx5/core/en/xdp.h  |   4 +-
 .../mellanox/mlx5/core/en/xsk/setup.c         |  27 +-
 .../mellanox/mlx5/core/en/xsk/setup.h         |   3 +
 .../net/ethernet/mellanox/mlx5/core/en_main.c | 232 +++++++++++++++---
 9 files changed, 304 insertions(+), 63 deletions(-)


base-commit: d8674294aefef02266c4d47ad10131f1bffbe534
-- 
2.44.0

[PATCH net-next 1/2] net/mlx5e: Batch NAPI RCU grace periods on channel close

[Tariq Toukan]

From: Carolina Jubran <cjubran@nvidia.com>

Channel close currently invokes synchronize_net() repeatedly while
tearing down each channel. RQ, ICOSQ, TX SQ and NAPI teardown each
require a grace period, causing the number of waits to scale with the
number of channels. TX SQ teardown adds a wait for each traffic class,
while XDP and QoS can add further calls.

Batch these waits across all channels. Split channel deactivate and
close into pre_sync and post_sync phases. First clear the ENABLED state
on all queues, then wait for a single grace period before continuing
with the teardown.

Similarly, unlink all NAPI instances with
__netif_napi_del_locked(), issue one synchronize_net() after all of
them have been unlinked, and only then free the channels.

Keep the existing per object synchronize_net() calls for single queue
paths.

248 channels, rcu_normal=1, ip link set eth2 down:
Before: 10.183s
After:   0.715s (~14x faster)

Signed-off-by: Carolina Jubran <cjubran@nvidia.com>
Signed-off-by: Tariq Toukan <tariqt@nvidia.com>
---
 drivers/net/ethernet/mellanox/mlx5/core/en.h  |   5 +
 .../net/ethernet/mellanox/mlx5/core/en/ptp.c  |  19 +-
 .../net/ethernet/mellanox/mlx5/core/en/ptp.h  |   4 +-
 .../net/ethernet/mellanox/mlx5/core/en/qos.c  |  69 +++++-
 .../net/ethernet/mellanox/mlx5/core/en/qos.h  |   4 +-
 .../net/ethernet/mellanox/mlx5/core/en/xdp.h  |   4 +-
 .../mellanox/mlx5/core/en/xsk/setup.c         |  27 ++-
 .../mellanox/mlx5/core/en/xsk/setup.h         |   3 +
 .../net/ethernet/mellanox/mlx5/core/en_main.c | 204 +++++++++++++++---
 9 files changed, 282 insertions(+), 57 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en.h b/drivers/net/ethernet/mellanox/mlx5/core/en.h
index a3945195f454..1a169fce075a 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en.h
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en.h
@@ -1103,6 +1103,7 @@ int mlx5e_open_xdpsq(struct mlx5e_channel *c, struct mlx5e_params *params,
 		     struct mlx5e_sq_param *param, struct xsk_buff_pool *xsk_pool,
 		     struct mlx5e_xdpsq *sq, bool is_redirect);
 void mlx5e_close_xdpsq(struct mlx5e_xdpsq *sq);
+void mlx5e_close_xdpsq_post_sync(struct mlx5e_xdpsq *sq);
 
 struct mlx5e_create_cq_param {
 	struct net_device *netdev;
@@ -1159,8 +1160,10 @@ int mlx5e_ptp_rx_manage_fs_ctx(struct mlx5e_priv *priv, void *ctx);
 int mlx5e_flush_rq(struct mlx5e_rq *rq, int curr_state);
 void mlx5e_activate_rq(struct mlx5e_rq *rq);
 void mlx5e_deactivate_rq(struct mlx5e_rq *rq);
+void mlx5e_deactivate_rq_pre_sync(struct mlx5e_rq *rq);
 void mlx5e_activate_icosq(struct mlx5e_icosq *icosq);
 void mlx5e_deactivate_icosq(struct mlx5e_icosq *icosq);
+void mlx5e_deactivate_icosq_pre_sync(struct mlx5e_icosq *icosq);
 
 int mlx5e_modify_sq(struct mlx5_core_dev *mdev, u32 sqn,
 		    struct mlx5e_modify_sq_param *p);
@@ -1170,6 +1173,8 @@ int mlx5e_open_txqsq(struct mlx5e_channel *c, u32 tisn, int txq_ix,
 		     struct mlx5e_sq_stats *sq_stats);
 void mlx5e_activate_txqsq(struct mlx5e_txqsq *sq);
 void mlx5e_deactivate_txqsq(struct mlx5e_txqsq *sq);
+void mlx5e_deactivate_txqsq_pre_sync(struct mlx5e_txqsq *sq);
+void mlx5e_deactivate_txqsq_post_sync(struct mlx5e_txqsq *sq);
 void mlx5e_free_txqsq(struct mlx5e_txqsq *sq);
 void mlx5e_tx_disable_queue(struct netdev_queue *txq);
 int mlx5e_alloc_txqsq_db(struct mlx5e_txqsq *sq, int numa);
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/ptp.c b/drivers/net/ethernet/mellanox/mlx5/core/en/ptp.c
index 17084d76209e..3fa629228878 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en/ptp.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en/ptp.c
@@ -775,7 +775,7 @@ static int mlx5e_ptp_open_queues(struct mlx5e_ptp *c,
 	return err;
 }
 
-static void mlx5e_ptp_close_queues(struct mlx5e_ptp *c)
+void mlx5e_ptp_close_queues(struct mlx5e_ptp *c)
 {
 	if (test_bit(MLX5E_PTP_STATE_RX, c->state)) {
 		mlx5e_close_rq(&c->rq);
@@ -932,7 +932,6 @@ void mlx5e_ptp_close(struct mlx5e_ptp *c)
 {
 	mlx5e_ptp_close_queues(c);
 	netif_napi_del_locked(&c->napi);
-
 	kvfree(c);
 }
 
@@ -954,18 +953,28 @@ void mlx5e_ptp_activate_channel(struct mlx5e_ptp *c)
 	mlx5e_trigger_napi_sched(&c->napi);
 }
 
-void mlx5e_ptp_deactivate_channel(struct mlx5e_ptp *c)
+void mlx5e_ptp_deactivate_channel_pre_sync(struct mlx5e_ptp *c)
 {
 	int tc;
 
 	if (test_bit(MLX5E_PTP_STATE_RX, c->state)) {
 		netif_queue_set_napi(c->netdev, c->rq.ix, NETDEV_QUEUE_TYPE_RX, NULL);
-		mlx5e_deactivate_rq(&c->rq);
+		mlx5e_deactivate_rq_pre_sync(&c->rq);
+	}
+
+	if (test_bit(MLX5E_PTP_STATE_TX, c->state)) {
+		for (tc = 0; tc < c->num_tc; tc++)
+			mlx5e_deactivate_txqsq_pre_sync(&c->ptpsq[tc].txqsq);
 	}
+}
+
+void mlx5e_ptp_deactivate_channel_post_sync(struct mlx5e_ptp *c)
+{
+	int tc;
 
 	if (test_bit(MLX5E_PTP_STATE_TX, c->state)) {
 		for (tc = 0; tc < c->num_tc; tc++)
-			mlx5e_deactivate_txqsq(&c->ptpsq[tc].txqsq);
+			mlx5e_deactivate_txqsq_post_sync(&c->ptpsq[tc].txqsq);
 	}
 
 	napi_disable_locked(&c->napi);
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/ptp.h b/drivers/net/ethernet/mellanox/mlx5/core/en/ptp.h
index 2a457a2ed707..b42ce33635a0 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en/ptp.h
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en/ptp.h
@@ -129,8 +129,10 @@ static inline bool mlx5e_ptpsq_metadata_freelist_empty(struct mlx5e_ptpsq *ptpsq
 int mlx5e_ptp_open(struct mlx5e_priv *priv, struct mlx5e_params *params,
 		   u8 lag_port, struct mlx5e_ptp **cp);
 void mlx5e_ptp_close(struct mlx5e_ptp *c);
+void mlx5e_ptp_close_queues(struct mlx5e_ptp *c);
 void mlx5e_ptp_activate_channel(struct mlx5e_ptp *c);
-void mlx5e_ptp_deactivate_channel(struct mlx5e_ptp *c);
+void mlx5e_ptp_deactivate_channel_pre_sync(struct mlx5e_ptp *c);
+void mlx5e_ptp_deactivate_channel_post_sync(struct mlx5e_ptp *c);
 int mlx5e_ptp_get_rqn(struct mlx5e_ptp *c, u32 *rqn);
 int mlx5e_ptp_alloc_rx_fs(struct mlx5e_flow_steering *fs,
 			  const struct mlx5e_profile *profile);
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/qos.c b/drivers/net/ethernet/mellanox/mlx5/core/en/qos.c
index ac4979087e0d..3e59862a0e60 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en/qos.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en/qos.c
@@ -233,15 +233,20 @@ void mlx5e_close_qos_sq(struct mlx5e_priv *priv, u16 qid)
 	kfree(sq);
 }
 
-void mlx5e_qos_close_queues(struct mlx5e_channel *c)
+static struct mlx5e_txqsq __rcu **
+mlx5e_qos_unpublish_queues(struct mlx5e_channel *c)
+{
+	return rcu_replace_pointer(c->qos_sqs, NULL,
+				   lockdep_is_held(&c->priv->state_lock));
+}
+
+static void mlx5e_qos_free_queues(struct mlx5e_channel *c,
+				  struct mlx5e_txqsq __rcu **qos_sqs)
 {
-	struct mlx5e_txqsq __rcu **qos_sqs;
 	int i;
 
-	qos_sqs = rcu_replace_pointer(c->qos_sqs, NULL, lockdep_is_held(&c->priv->state_lock));
 	if (!qos_sqs)
 		return;
-	synchronize_rcu(); /* Sync with NAPI. */
 
 	for (i = 0; i < c->qos_sqs_size; i++) {
 		struct mlx5e_txqsq *sq;
@@ -258,6 +263,27 @@ void mlx5e_qos_close_queues(struct mlx5e_channel *c)
 	kvfree(qos_sqs);
 }
 
+void mlx5e_qos_close_queues_post_sync(struct mlx5e_channel *c)
+{
+	struct mlx5e_txqsq __rcu **qos_sqs;
+
+	qos_sqs = mlx5e_qos_unpublish_queues(c);
+	/* Caller guarantees that NAPI is stopped or was never enabled. */
+	mlx5e_qos_free_queues(c, qos_sqs);
+}
+
+void mlx5e_qos_close_queues(struct mlx5e_channel *c)
+{
+	struct mlx5e_txqsq __rcu **qos_sqs;
+
+	qos_sqs = mlx5e_qos_unpublish_queues(c);
+	if (!qos_sqs)
+		return;
+
+	synchronize_rcu(); /* Sync with NAPI. */
+	mlx5e_qos_free_queues(c, qos_sqs);
+}
+
 void mlx5e_qos_close_all_queues(struct mlx5e_channels *chs)
 {
 	int i;
@@ -326,11 +352,10 @@ void mlx5e_qos_activate_queues(struct mlx5e_priv *priv)
 	mlx5e_htb_enumerate_leaves(priv->htb, mlx5e_activate_qos_sq, priv);
 }
 
-void mlx5e_qos_deactivate_queues(struct mlx5e_channel *c)
+void mlx5e_qos_deactivate_queues_pre_sync(struct mlx5e_channel *c)
 {
 	struct mlx5e_params *params = &c->priv->channels.params;
 	struct mlx5e_txqsq __rcu **qos_sqs;
-	u16 txq_ix;
 	int i;
 
 	qos_sqs = mlx5e_state_dereference(c->priv, c->qos_sqs);
@@ -346,7 +371,30 @@ void mlx5e_qos_deactivate_queues(struct mlx5e_channel *c)
 			continue;
 
 		qos_dbg(c->mdev, "Deactivate QoS SQ qid %u\n", qid);
-		mlx5e_deactivate_txqsq(sq);
+		mlx5e_deactivate_txqsq_pre_sync(sq);
+	}
+}
+
+void mlx5e_qos_deactivate_queues_post_sync(struct mlx5e_channel *c)
+{
+	struct mlx5e_params *params = &c->priv->channels.params;
+	struct mlx5e_txqsq __rcu **qos_sqs;
+	int i;
+
+	qos_sqs = mlx5e_state_dereference(c->priv, c->qos_sqs);
+	if (!qos_sqs)
+		return;
+
+	for (i = 0; i < c->qos_sqs_size; i++) {
+		u16 qid = params->num_channels * i + c->ix;
+		struct mlx5e_txqsq *sq;
+		u16 txq_ix;
+
+		sq = mlx5e_state_dereference(c->priv, qos_sqs[i]);
+		if (!sq)
+			continue;
+
+		mlx5e_deactivate_txqsq_post_sync(sq);
 
 		txq_ix = mlx5e_qid_from_qos(&c->priv->channels, qid);
 
@@ -361,7 +409,12 @@ void mlx5e_qos_deactivate_all_queues(struct mlx5e_channels *chs)
 	int i;
 
 	for (i = 0; i < chs->num; i++)
-		mlx5e_qos_deactivate_queues(chs->c[i]);
+		mlx5e_qos_deactivate_queues_pre_sync(chs->c[i]);
+
+	synchronize_net();
+
+	for (i = 0; i < chs->num; i++)
+		mlx5e_qos_deactivate_queues_post_sync(chs->c[i]);
 }
 
 void mlx5e_reactivate_qos_sq(struct mlx5e_priv *priv, u16 qid, struct netdev_queue *txq)
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/qos.h b/drivers/net/ethernet/mellanox/mlx5/core/en/qos.h
index 4947afa23b73..8d171a1079e9 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en/qos.h
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en/qos.h
@@ -28,9 +28,11 @@ void mlx5e_reset_qdisc(struct net_device *dev, u16 qid);
 
 int mlx5e_qos_open_queues(struct mlx5e_priv *priv, struct mlx5e_channels *chs);
 void mlx5e_qos_activate_queues(struct mlx5e_priv *priv);
-void mlx5e_qos_deactivate_queues(struct mlx5e_channel *c);
+void mlx5e_qos_deactivate_queues_pre_sync(struct mlx5e_channel *c);
+void mlx5e_qos_deactivate_queues_post_sync(struct mlx5e_channel *c);
 void mlx5e_qos_deactivate_all_queues(struct mlx5e_channels *chs);
 void mlx5e_qos_close_queues(struct mlx5e_channel *c);
+void mlx5e_qos_close_queues_post_sync(struct mlx5e_channel *c);
 void mlx5e_qos_close_all_queues(struct mlx5e_channels *chs);
 int mlx5e_qos_alloc_queues(struct mlx5e_priv *priv, struct mlx5e_channels *chs);
 
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/xdp.h b/drivers/net/ethernet/mellanox/mlx5/core/en/xdp.h
index 5b8f4094d553..ea557a8310e5 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en/xdp.h
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en/xdp.h
@@ -130,14 +130,12 @@ static inline void mlx5e_xdp_tx_enable(struct mlx5e_priv *priv)
 		set_bit(MLX5E_STATE_XDP_ACTIVE, &priv->state);
 }
 
-static inline void mlx5e_xdp_tx_disable(struct mlx5e_priv *priv)
+static inline void mlx5e_xdp_tx_disable_pre_sync(struct mlx5e_priv *priv)
 {
 	if (priv->channels.params.xdp_prog)
 		clear_bit(MLX5E_STATE_XDP_ACTIVE, &priv->state);
 
 	clear_bit(MLX5E_STATE_XDP_TX_ENABLED, &priv->state);
-	/* Let other device's napi(s) and XSK wakeups see our new state. */
-	synchronize_net();
 }
 
 static inline bool mlx5e_xdp_tx_is_enabled(struct mlx5e_priv *priv)
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/xsk/setup.c b/drivers/net/ethernet/mellanox/mlx5/core/en/xsk/setup.c
index 11500fd213a5..deb84ef29ca1 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en/xsk/setup.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en/xsk/setup.c
@@ -160,23 +160,35 @@ int mlx5e_open_xsk(struct mlx5e_priv *priv, struct mlx5e_params *params,
 err_close_rx_cq:
 	mlx5e_close_cq(&c->xskrq.cq);
 
+	c->xskrq.xsk_pool = NULL;
+	c->xsksq.xsk_pool = NULL;
+
 	return err;
 }
 
-void mlx5e_close_xsk(struct mlx5e_channel *c)
+void mlx5e_close_xsk_pre_sync(struct mlx5e_channel *c)
 {
 	clear_bit(MLX5E_CHANNEL_STATE_XSK, c->state);
-	synchronize_net(); /* Sync with NAPI. */
+}
 
+void mlx5e_close_xsk_post_sync(struct mlx5e_channel *c)
+{
 	mlx5e_close_rq(&c->xskrq);
 	mlx5e_close_cq(&c->xskrq.cq);
-	mlx5e_close_xdpsq(&c->xsksq);
+	mlx5e_close_xdpsq_post_sync(&c->xsksq);
 	mlx5e_close_cq(&c->xsksq.cq);
 
 	memset(&c->xskrq, 0, sizeof(c->xskrq));
 	memset(&c->xsksq, 0, sizeof(c->xsksq));
 }
 
+void mlx5e_close_xsk(struct mlx5e_channel *c)
+{
+	mlx5e_close_xsk_pre_sync(c);
+	synchronize_net(); /* Sync with XSK wakeup and NAPI. */
+	mlx5e_close_xsk_post_sync(c);
+}
+
 void mlx5e_activate_xsk(struct mlx5e_channel *c)
 {
 	/* ICOSQ recovery deactivates RQs. Suspend the recovery to avoid
@@ -189,7 +201,7 @@ void mlx5e_activate_xsk(struct mlx5e_channel *c)
 	/* TX queue is created active. */
 }
 
-void mlx5e_deactivate_xsk(struct mlx5e_channel *c)
+void mlx5e_deactivate_xsk_pre_sync(struct mlx5e_channel *c)
 {
 	/* ICOSQ recovery may reactivate XSKRQ if clear_bit is called in the
 	 * middle of recovery. Suspend the recovery to avoid it.
@@ -197,7 +209,12 @@ void mlx5e_deactivate_xsk(struct mlx5e_channel *c)
 	mlx5e_reporter_icosq_suspend_recovery(c);
 	clear_bit(MLX5E_RQ_STATE_ENABLED, &c->xskrq.state);
 	mlx5e_reporter_icosq_resume_recovery(c);
-	synchronize_net(); /* Sync with NAPI to prevent mlx5e_post_rx_wqes. */
 
 	/* TX queue is disabled on close. */
 }
+
+void mlx5e_deactivate_xsk(struct mlx5e_channel *c)
+{
+	mlx5e_deactivate_xsk_pre_sync(c);
+	synchronize_net(); /* Sync with NAPI to prevent mlx5e_post_rx_wqes. */
+}
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/xsk/setup.h b/drivers/net/ethernet/mellanox/mlx5/core/en/xsk/setup.h
index 664ec78192c3..66a0b09452f7 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en/xsk/setup.h
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en/xsk/setup.h
@@ -17,7 +17,10 @@ int mlx5e_open_xsk(struct mlx5e_priv *priv, struct mlx5e_params *params,
 		   struct xsk_buff_pool *pool,
 		   struct mlx5e_channel *c);
 void mlx5e_close_xsk(struct mlx5e_channel *c);
+void mlx5e_close_xsk_pre_sync(struct mlx5e_channel *c);
+void mlx5e_close_xsk_post_sync(struct mlx5e_channel *c);
 void mlx5e_activate_xsk(struct mlx5e_channel *c);
 void mlx5e_deactivate_xsk(struct mlx5e_channel *c);
+void mlx5e_deactivate_xsk_pre_sync(struct mlx5e_channel *c);
 
 #endif /* __MLX5_EN_XSK_SETUP_H__ */
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
index f90b5a34e097..fe4e5d0b2e9a 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
@@ -1458,9 +1458,14 @@ void mlx5e_activate_rq(struct mlx5e_rq *rq)
 	set_bit(MLX5E_RQ_STATE_ENABLED, &rq->state);
 }
 
-void mlx5e_deactivate_rq(struct mlx5e_rq *rq)
+void mlx5e_deactivate_rq_pre_sync(struct mlx5e_rq *rq)
 {
 	clear_bit(MLX5E_RQ_STATE_ENABLED, &rq->state);
+}
+
+void mlx5e_deactivate_rq(struct mlx5e_rq *rq)
+{
+	mlx5e_deactivate_rq_pre_sync(rq);
 	synchronize_net(); /* Sync with NAPI to prevent mlx5e_post_rx_wqes. */
 }
 
@@ -1954,13 +1959,15 @@ void mlx5e_tx_disable_queue(struct netdev_queue *txq)
 	__netif_tx_unlock_bh(txq);
 }
 
-void mlx5e_deactivate_txqsq(struct mlx5e_txqsq *sq)
+void mlx5e_deactivate_txqsq_pre_sync(struct mlx5e_txqsq *sq)
 {
-	struct mlx5_wq_cyc *wq = &sq->wq;
-
 	netif_queue_set_napi(sq->netdev, sq->txq_ix, NETDEV_QUEUE_TYPE_TX, NULL);
 	clear_bit(MLX5E_SQ_STATE_ENABLED, &sq->state);
-	synchronize_net(); /* Sync with NAPI to prevent netif_tx_wake_queue. */
+}
+
+void mlx5e_deactivate_txqsq_post_sync(struct mlx5e_txqsq *sq)
+{
+	struct mlx5_wq_cyc *wq = &sq->wq;
 
 	mlx5e_tx_disable_queue(sq->txq);
 
@@ -1978,6 +1985,13 @@ void mlx5e_deactivate_txqsq(struct mlx5e_txqsq *sq)
 	}
 }
 
+void mlx5e_deactivate_txqsq(struct mlx5e_txqsq *sq)
+{
+	mlx5e_deactivate_txqsq_pre_sync(sq);
+	synchronize_net(); /* Sync with NAPI to prevent netif_tx_wake_queue. */
+	mlx5e_deactivate_txqsq_post_sync(sq);
+}
+
 void mlx5e_close_txqsq(struct mlx5e_txqsq *sq)
 {
 	struct mlx5_core_dev *mdev = sq->mdev;
@@ -2089,9 +2103,14 @@ void mlx5e_activate_icosq(struct mlx5e_icosq *icosq)
 	set_bit(MLX5E_SQ_STATE_ENABLED, &icosq->state);
 }
 
-void mlx5e_deactivate_icosq(struct mlx5e_icosq *icosq)
+void mlx5e_deactivate_icosq_pre_sync(struct mlx5e_icosq *icosq)
 {
 	clear_bit(MLX5E_SQ_STATE_ENABLED, &icosq->state);
+}
+
+void mlx5e_deactivate_icosq(struct mlx5e_icosq *icosq)
+{
+	mlx5e_deactivate_icosq_pre_sync(icosq);
 	synchronize_net(); /* Sync with NAPI. */
 }
 
@@ -2141,18 +2160,28 @@ int mlx5e_open_xdpsq(struct mlx5e_channel *c, struct mlx5e_params *params,
 	return err;
 }
 
-void mlx5e_close_xdpsq(struct mlx5e_xdpsq *sq)
+static void mlx5e_destroy_xdpsq(struct mlx5e_xdpsq *sq)
 {
 	struct mlx5e_channel *c = sq->channel;
 
-	clear_bit(MLX5E_SQ_STATE_ENABLED, &sq->state);
-	synchronize_net(); /* Sync with NAPI. */
-
 	mlx5e_destroy_sq(c->mdev, sq->sqn);
 	mlx5e_free_xdpsq_descs(sq);
 	mlx5e_free_xdpsq(sq);
 }
 
+void mlx5e_close_xdpsq_post_sync(struct mlx5e_xdpsq *sq)
+{
+	clear_bit(MLX5E_SQ_STATE_ENABLED, &sq->state);
+	mlx5e_destroy_xdpsq(sq);
+}
+
+void mlx5e_close_xdpsq(struct mlx5e_xdpsq *sq)
+{
+	clear_bit(MLX5E_SQ_STATE_ENABLED, &sq->state);
+	synchronize_net(); /* Sync with NAPI. */
+	mlx5e_destroy_xdpsq(sq);
+}
+
 static struct mlx5e_xdpsq *mlx5e_open_xdpredirect_sq(struct mlx5e_channel *c,
 						     struct mlx5e_params *params,
 						     struct mlx5e_channel_param *cparam,
@@ -2184,6 +2213,13 @@ static struct mlx5e_xdpsq *mlx5e_open_xdpredirect_sq(struct mlx5e_channel *c,
 	return ERR_PTR(err);
 }
 
+static void mlx5e_close_xdpredirect_sq_post_sync(struct mlx5e_xdpsq *xdpsq)
+{
+	mlx5e_close_xdpsq_post_sync(xdpsq);
+	mlx5e_close_cq(&xdpsq->cq);
+	kvfree(xdpsq);
+}
+
 static void mlx5e_close_xdpredirect_sq(struct mlx5e_xdpsq *xdpsq)
 {
 	mlx5e_close_xdpsq(xdpsq);
@@ -2749,6 +2785,27 @@ static void mlx5e_close_queues(struct mlx5e_channel *c)
 	mlx5e_close_cq(&c->icosq.cq);
 }
 
+static void mlx5e_close_queues_post_sync(struct mlx5e_channel *c)
+{
+	if (c->xdp)
+		mlx5e_close_xdpsq_post_sync(&c->rq_xdpsq);
+	/* The same ICOSQ is used for UMRs for both RQ and XSKRQ. */
+	cancel_work_sync(&c->icosq.recover_work);
+	mlx5e_close_rq(&c->rq);
+	mlx5e_close_sqs(c);
+	mlx5e_close_icosq(&c->icosq);
+	mutex_destroy(&c->icosq_recovery_lock);
+	if (c->async_icosq)
+		mlx5e_close_async_icosq(c->async_icosq);
+	if (c->xdp)
+		mlx5e_close_cq(&c->rq_xdpsq.cq);
+	mlx5e_close_cq(&c->rq.cq);
+	if (c->xdpsq)
+		mlx5e_close_xdpredirect_sq_post_sync(c->xdpsq);
+	mlx5e_close_tx_cqs(c);
+	mlx5e_close_cq(&c->icosq.cq);
+}
+
 static u8 mlx5e_enumerate_lag_port(struct mlx5_core_dev *mdev, int ix)
 {
 	u16 port_aff_bias = mlx5_core_is_pf(mdev) ? 0 : MLX5_CAP_GEN(mdev, vhca_id);
@@ -2936,35 +2993,56 @@ static void mlx5e_activate_channel(struct mlx5e_channel *c)
 	netif_queue_set_napi(c->netdev, c->ix, NETDEV_QUEUE_TYPE_RX, &c->napi);
 }
 
-static void mlx5e_deactivate_channel(struct mlx5e_channel *c)
+static void mlx5e_deactivate_channel_pre_sync(struct mlx5e_channel *c)
 {
 	int tc;
 
 	netif_queue_set_napi(c->netdev, c->ix, NETDEV_QUEUE_TYPE_RX, NULL);
 
 	if (test_bit(MLX5E_CHANNEL_STATE_XSK, c->state))
-		mlx5e_deactivate_xsk(c);
+		mlx5e_deactivate_xsk_pre_sync(c);
 	else
-		mlx5e_deactivate_rq(&c->rq);
+		mlx5e_deactivate_rq_pre_sync(&c->rq);
 
 	if (c->async_icosq)
-		mlx5e_deactivate_icosq(c->async_icosq);
-	mlx5e_deactivate_icosq(&c->icosq);
+		mlx5e_deactivate_icosq_pre_sync(c->async_icosq);
+	mlx5e_deactivate_icosq_pre_sync(&c->icosq);
 	for (tc = 0; tc < c->num_tc; tc++)
-		mlx5e_deactivate_txqsq(&c->sq[tc]);
-	mlx5e_qos_deactivate_queues(c);
+		mlx5e_deactivate_txqsq_pre_sync(&c->sq[tc]);
+	mlx5e_qos_deactivate_queues_pre_sync(c);
+}
+
+static void mlx5e_deactivate_channel_post_sync(struct mlx5e_channel *c)
+{
+	int tc;
+
+	for (tc = 0; tc < c->num_tc; tc++)
+		mlx5e_deactivate_txqsq_post_sync(&c->sq[tc]);
+	mlx5e_qos_deactivate_queues_post_sync(c);
 
 	napi_disable_locked(&c->napi);
 }
 
-static void mlx5e_close_channel(struct mlx5e_channel *c)
+static void mlx5e_close_channel_pre_sync(struct mlx5e_channel *c)
 {
 	if (test_bit(MLX5E_CHANNEL_STATE_XSK, c->state))
-		mlx5e_close_xsk(c);
-	mlx5e_close_queues(c);
-	mlx5e_qos_close_queues(c);
-	netif_napi_del_locked(&c->napi);
+		mlx5e_close_xsk_pre_sync(c);
+}
+
+static void mlx5e_close_channel_post_sync(struct mlx5e_channel *c)
+{
+	if (c->xskrq.xsk_pool)
+		mlx5e_close_xsk_post_sync(c);
+	mlx5e_close_queues_post_sync(c);
+	mlx5e_qos_close_queues_post_sync(c);
+}
 
+static void mlx5e_close_channel(struct mlx5e_channel *c)
+{
+	mlx5e_close_channel_pre_sync(c);
+	synchronize_net();
+	mlx5e_close_channel_post_sync(c);
+	netif_napi_del_locked(&c->napi);
 	kvfree(c);
 }
 
@@ -3060,33 +3138,72 @@ static int mlx5e_wait_channels_min_rx_wqes(struct mlx5e_channels *chs)
 	return err ? -ETIMEDOUT : 0;
 }
 
-static void mlx5e_deactivate_channels(struct mlx5e_channels *chs)
+static void mlx5e_deactivate_channels_pre_sync(struct mlx5e_channels *chs)
 {
 	int i;
 
 	if (chs->ptp)
-		mlx5e_ptp_deactivate_channel(chs->ptp);
+		mlx5e_ptp_deactivate_channel_pre_sync(chs->ptp);
 
 	for (i = 0; i < chs->num; i++)
-		mlx5e_deactivate_channel(chs->c[i]);
+		mlx5e_deactivate_channel_pre_sync(chs->c[i]);
 }
 
-void mlx5e_close_channels(struct mlx5e_channels *chs)
+static void mlx5e_deactivate_channels_post_sync(struct mlx5e_channels *chs)
+{
+	int i;
+
+	if (chs->ptp)
+		mlx5e_ptp_deactivate_channel_post_sync(chs->ptp);
+
+	for (i = 0; i < chs->num; i++)
+		mlx5e_deactivate_channel_post_sync(chs->c[i]);
+}
+
+static void mlx5e_close_channels_pre_sync(struct mlx5e_channels *chs)
+{
+	int i;
+
+	for (i = 0; i < chs->num; i++)
+		mlx5e_close_channel_pre_sync(chs->c[i]);
+}
+
+static void mlx5e_close_channels_post_sync(struct mlx5e_channels *chs)
 {
 	int i;
 
-	ASSERT_RTNL();
 	if (chs->ptp) {
-		mlx5e_ptp_close(chs->ptp);
+		mlx5e_ptp_close_queues(chs->ptp);
+		__netif_napi_del_locked(&chs->ptp->napi);
+	}
+
+	for (i = 0; i < chs->num; i++) {
+		mlx5e_close_channel_post_sync(chs->c[i]);
+		__netif_napi_del_locked(&chs->c[i]->napi);
+	}
+
+	synchronize_net();
+
+	if (chs->ptp) {
+		kvfree(chs->ptp);
 		chs->ptp = NULL;
 	}
+
 	for (i = 0; i < chs->num; i++)
-		mlx5e_close_channel(chs->c[i]);
+		kvfree(chs->c[i]);
 
 	kfree(chs->c);
 	chs->num = 0;
 }
 
+void mlx5e_close_channels(struct mlx5e_channels *chs)
+{
+	ASSERT_RTNL();
+	mlx5e_close_channels_pre_sync(chs);
+	synchronize_net();
+	mlx5e_close_channels_post_sync(chs);
+}
+
 static int mlx5e_modify_tirs_packet_merge(struct mlx5e_priv *priv)
 {
 	struct mlx5e_rx_res *res = priv->rx_res;
@@ -3376,7 +3493,7 @@ static void mlx5e_cancel_tx_timeout_work(struct mlx5e_priv *priv)
 		cancel_work_sync(&priv->tx_timeout_work);
 }
 
-void mlx5e_deactivate_priv_channels(struct mlx5e_priv *priv)
+static void mlx5e_deactivate_priv_channels_pre_sync(struct mlx5e_priv *priv)
 {
 	if (priv->rx_res)
 		mlx5e_rx_res_channels_deactivate(priv->rx_res);
@@ -3394,8 +3511,21 @@ void mlx5e_deactivate_priv_channels(struct mlx5e_priv *priv)
 	 */
 	netif_tx_disable(priv->netdev);
 
-	mlx5e_xdp_tx_disable(priv);
-	mlx5e_deactivate_channels(&priv->channels);
+	mlx5e_xdp_tx_disable_pre_sync(priv);
+	mlx5e_deactivate_channels_pre_sync(&priv->channels);
+}
+
+static void mlx5e_deactivate_priv_channels_post_sync(struct mlx5e_priv *priv)
+{
+	mlx5e_deactivate_channels_post_sync(&priv->channels);
+}
+
+void mlx5e_deactivate_priv_channels(struct mlx5e_priv *priv)
+{
+	mlx5e_deactivate_priv_channels_pre_sync(priv);
+	/* One grace period for XDP, RQ, ICOSQ, TX, QoS, XSK and PTP. */
+	synchronize_net();
+	mlx5e_deactivate_priv_channels_post_sync(priv);
 }
 
 static int mlx5e_switch_priv_params(struct mlx5e_priv *priv,
@@ -3611,8 +3741,14 @@ int mlx5e_close_locked(struct net_device *netdev)
 	clear_bit(MLX5E_STATE_OPENED, &priv->state);
 
 	netif_carrier_off(priv->netdev);
-	mlx5e_deactivate_priv_channels(priv);
-	mlx5e_close_channels(&priv->channels);
+
+	mlx5e_deactivate_priv_channels_pre_sync(priv);
+	mlx5e_close_channels_pre_sync(&priv->channels);
+
+	synchronize_net();
+
+	mlx5e_deactivate_priv_channels_post_sync(priv);
+	mlx5e_close_channels_post_sync(&priv->channels);
 
 	return 0;
 }
-- 
2.44.0

Re: [PATCH net-next 1/2] net/mlx5e: Batch NAPI RCU grace periods on channel close

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 4 potential
issue(s) to consider.

Critical: 1 · High: 0 · Medium: 1 · Low: 2

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Critical] Use-after-free in mlx5e_close_locked() (the ndo_stop path),
  introduced by this patch.
- [Medium] The commit message does not match the close-phase code, and
  the close path's safety now rests on an undocumented, unenforced…
- [Low] mlx5e_close_locked() now calls mlx5e_close_channels_pre_sync()
  and mlx5e_close_channels_post_sync() directly instead of…
- [Low] The patch changes how mlx5e_close_channel_post_sync()
  (en_main.c:3032) decides whether to tear down a channel's XSK queues,…

--

net/mlx5e: Batch NAPI RCU grace periods on channel close

This splits mlx5e channel deactivate and close into pre_sync and post_sync
phases, so one synchronize_net() covers all channels instead of one per
queue. NAPI instances are unlinked with __netif_napi_del_locked(), and a
single grace period is taken before the channels are freed. Single-queue
paths are meant to keep their own synchronize_net() calls.

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> index f90b5a34e097..fe4e5d0b2e9a 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c

[ ... ]

> @@ -2141,18 +2160,28 @@ int mlx5e_open_xdpsq(struct mlx5e_channel *c, struct mlx5e_params *params,
>  	return err;
>  }
>  
> -void mlx5e_close_xdpsq(struct mlx5e_xdpsq *sq)
> +static void mlx5e_destroy_xdpsq(struct mlx5e_xdpsq *sq)
>  {
>  	struct mlx5e_channel *c = sq->channel;
>  
> -	clear_bit(MLX5E_SQ_STATE_ENABLED, &sq->state);
> -	synchronize_net(); /* Sync with NAPI. */
> -
>  	mlx5e_destroy_sq(c->mdev, sq->sqn);
>  	mlx5e_free_xdpsq_descs(sq);
>  	mlx5e_free_xdpsq(sq);
>  }
>  
> +void mlx5e_close_xdpsq_post_sync(struct mlx5e_xdpsq *sq)
> +{
> +	clear_bit(MLX5E_SQ_STATE_ENABLED, &sq->state);
> +	mlx5e_destroy_xdpsq(sq);
> +}
> +
> +void mlx5e_close_xdpsq(struct mlx5e_xdpsq *sq)
> +{
> +	clear_bit(MLX5E_SQ_STATE_ENABLED, &sq->state);
> +	synchronize_net(); /* Sync with NAPI. */
> +	mlx5e_destroy_xdpsq(sq);
> +}
> +

[Severity: Medium]
The commit message says:

  First clear the ENABLED state on all queues, then wait for a single
  grace period before continuing with the teardown.

and:

  Keep the existing per object synchronize_net() calls for single queue
  paths.

Does that match the close phase?

mlx5e_close_xdpsq_post_sync() clears MLX5E_SQ_STATE_ENABLED after the
shared grace period and then destroys the SQ right away. So there is no
pre_sync clear for rq_xdpsq, the XDP_REDIRECT xdpsq or xsksq, and nothing
waits after this clear_bit(). mlx5e_close_channel_pre_sync() only clears
MLX5E_CHANNEL_STATE_XSK.

The other close-phase waits follow the same pattern:

- mlx5e_qos_close_queues_post_sync() removes the synchronize_rcu()
  between unpublishing and freeing the array instead of batching it.

- The single-queue path mlx5e_close_xsk() goes from two synchronize_net()
  calls to one. That particular reduction looks safe on its own, because
  NAPI only touches xsksq under the xsk_open snapshot taken in the same
  RCU section.

So these waits look removed rather than batched, on the assumption that
NAPI is already disabled. That precondition only appears in a comment in
mlx5e_qos_close_queues_post_sync():

	/* Caller guarantees that NAPI is stopped or was never enabled. */

mlx5e_close_xdpsq_post_sync() is exported in en.h and also called from
xsk/setup.c, but nothing documents or enforces that requirement.

Could the commit message describe this? Could the precondition also be
documented on the exported post_sync helpers? As noted below for
mlx5e_close_locked(), NAPI being disabled may not be enough on its own.

[ ... ]

> @@ -2936,35 +2993,56 @@ static void mlx5e_activate_channel(struct mlx5e_channel *c)

[ ... ]

> -static void mlx5e_close_channel(struct mlx5e_channel *c)
> +static void mlx5e_close_channel_pre_sync(struct mlx5e_channel *c)
>  {
>  	if (test_bit(MLX5E_CHANNEL_STATE_XSK, c->state))
> -		mlx5e_close_xsk(c);
> -	mlx5e_close_queues(c);
> -	mlx5e_qos_close_queues(c);
> -	netif_napi_del_locked(&c->napi);
> +		mlx5e_close_xsk_pre_sync(c);
> +}
> +
> +static void mlx5e_close_channel_post_sync(struct mlx5e_channel *c)
> +{
> +	if (c->xskrq.xsk_pool)
> +		mlx5e_close_xsk_post_sync(c);

[Severity: Low]
This isn't a bug, but the test for tearing down the XSK queues changes
here from test_bit(MLX5E_CHANNEL_STATE_XSK, c->state) to
c->xskrq.xsk_pool. It is only correct because of the new error-path reset
in mlx5e_open_xsk() in xsk/setup.c:

err_close_rx_cq:
	mlx5e_close_cq(&c->xskrq.cq);

	c->xskrq.xsk_pool = NULL;
	c->xsksq.xsk_pool = NULL;

Without that reset, a failed mlx5e_xsk_enable_locked() on a live channel
would leave the pool pointer set. Close would then tear down xskrq and
xsksq a second time.

The two hunks depend on each other, but the commit message mentions
neither. Could this be noted in the commit message, so a partial backport
or a later edit to the error path doesn't bring back a double teardown?

> +	mlx5e_close_queues_post_sync(c);
> +	mlx5e_qos_close_queues_post_sync(c);
> +}

[ ... ]

> @@ -3060,33 +3138,72 @@ static int mlx5e_wait_channels_min_rx_wqes(struct mlx5e_channels *chs)

[ ... ]

> +static void mlx5e_close_channels_post_sync(struct mlx5e_channels *chs)
>  {
>  	int i;
>  
> -	ASSERT_RTNL();
>  	if (chs->ptp) {
> -		mlx5e_ptp_close(chs->ptp);
> +		mlx5e_ptp_close_queues(chs->ptp);
> +		__netif_napi_del_locked(&chs->ptp->napi);
> +	}
> +
> +	for (i = 0; i < chs->num; i++) {
> +		mlx5e_close_channel_post_sync(chs->c[i]);
> +		__netif_napi_del_locked(&chs->c[i]->napi);
> +	}
> +
> +	synchronize_net();
> +
> +	if (chs->ptp) {
> +		kvfree(chs->ptp);
>  		chs->ptp = NULL;
>  	}
> +
>  	for (i = 0; i < chs->num; i++)
> -		mlx5e_close_channel(chs->c[i]);
> +		kvfree(chs->c[i]);
>  
>  	kfree(chs->c);
>  	chs->num = 0;
>  }
>  
> +void mlx5e_close_channels(struct mlx5e_channels *chs)
> +{
> +	ASSERT_RTNL();
> +	mlx5e_close_channels_pre_sync(chs);
> +	synchronize_net();
> +	mlx5e_close_channels_post_sync(chs);
> +}

[Severity: Low]
mlx5e_close_locked() now calls mlx5e_close_channels_pre_sync() and
mlx5e_close_channels_post_sync() directly. That means the ASSERT_RTNL(),
which now lives only in this wrapper, no longer runs on the ndo_stop path.

This isn't a functional bug, since mlx5e_close() and mlx5e_rep_close()
both run under RTNL. Would it be worth keeping the assertion in
mlx5e_close_locked() or in the split helpers?

[ ... ]

> @@ -3611,8 +3741,14 @@ int mlx5e_close_locked(struct net_device *netdev)
>  	clear_bit(MLX5E_STATE_OPENED, &priv->state);
>  
>  	netif_carrier_off(priv->netdev);
> -	mlx5e_deactivate_priv_channels(priv);
> -	mlx5e_close_channels(&priv->channels);
> +
> +	mlx5e_deactivate_priv_channels_pre_sync(priv);
> +	mlx5e_close_channels_pre_sync(&priv->channels);
> +
> +	synchronize_net();
> +
> +	mlx5e_deactivate_priv_channels_post_sync(priv);
> +	mlx5e_close_channels_post_sync(&priv->channels);

[Severity: Critical]
In mlx5e_close_locked(), the only grace period now runs before
napi_disable_locked(). What waits for a NAPI poll that is still running
between napi_disable_locked() and the frees in
mlx5e_close_channels_post_sync()?

The sequence becomes:

mlx5e_close_locked()
    synchronize_net()
    mlx5e_deactivate_priv_channels_post_sync()
        mlx5e_deactivate_channel_post_sync()
            napi_disable_locked(&c->napi)
    mlx5e_close_channels_post_sync()
        mlx5e_close_channel_post_sync()
            mlx5e_close_queues_post_sync()
                frees sq->dim, rq->dim, c->async_icosq, c->xdpsq
            mlx5e_qos_close_queues_post_sync()
                kfree(sq), kvfree(qos_sqs), no grace period
        synchronize_net()
        kvfree(chs->c[i])

napi_disable_locked() only waits for NAPIF_STATE_SCHED to be released.
Documentation/networking/napi.rst notes that napi_disable() does not wait
for the poll method to exit.

mlx5e_napi_poll() keeps using queue memory after napi_complete_done(),
inside rcu_read_lock():

mlx5e_napi_poll() {
    ...
	qos_sqs = rcu_dereference(c->qos_sqs);
    ...
	if (unlikely(!napi_complete_done(napi, work_done)))
		goto out;
    ...
	for (i = 0; i < c->num_tc; i++) {
		mlx5e_handle_tx_dim(&c->sq[i]);
		mlx5e_cq_arm(&c->sq[i].cq);
    ...
			if (sq) {
				mlx5e_handle_tx_dim(sq);
				mlx5e_cq_arm(&sq->cq);
    ...
	mlx5e_handle_rx_dim(rq);
    ...
}

The NAPI CPU could be delayed after napi_complete_done(), for example a
preempted threaded NAPI on PREEMPT_RT, or vCPU preemption. In that case,
can this tail touch the QoS SQs, the qos_sqs array, the DIM structs,
async_icosq and xdpsq after they have been freed? net_dim() could also
schedule dim->work on freed memory after cancel_work_sync() has already
run.

Before this patch, mlx5e_qos_close_queues() did synchronize_rcu() between
rcu_replace_pointer() and the frees. The per-object synchronize_net()
calls also gave a grace period after each earlier channel's
napi_disable_locked().

The synchronize_net() in mlx5e_close_channels_post_sync() runs after the
queues are already freed, so it only covers the mlx5e_channel and
mlx5e_ptp structs.

The other paths call mlx5e_deactivate_priv_channels() and then
mlx5e_close_channels(), so they still get one grace period after
napi_disable_locked(). Is merging the two into a single wait safe for
mlx5e_close_locked()?

>  
>  	return 0;
>  }

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261011133212.807979-1-tariqt%40nvidia.com

[PATCH net-next 2/2] net/mlx5e: Batch ICOSQ trigger RCU synchronization

[Tariq Toukan]

From: Carolina Jubran <cjubran@nvidia.com>

During channel activation, mlx5e_activate_channels() triggers the ICOSQ
once for every channel in order to kick NAPI. mlx5e_trigger_napi_icosq()
sets MLX5E_SQ_STATE_LOCK_NEEDED and waits for an RCU grace period before
serializing the trigger with ICOSQ users in NAPI context.

Mark all channel ICOSQs as requiring locking first, wait for a single
grace period, and then trigger all of them.

Keep mlx5e_trigger_napi_icosq() with its existing synchronization
semantics for standalone callers.

248 channels, rcu_normal=1, ip link set eth2 up:
Before: 3.095s
After:  1.794s (~1.7x faster) 

Signed-off-by: Carolina Jubran <cjubran@nvidia.com>
Signed-off-by: Tariq Toukan <tariqt@nvidia.com>
---
 .../net/ethernet/mellanox/mlx5/core/en_main.c | 28 +++++++++++++++----
 1 file changed, 22 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
index fe4e5d0b2e9a..58e4874b664f 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
@@ -2837,14 +2837,11 @@ static int mlx5e_channel_stats_alloc(struct mlx5e_priv *priv, int ix, int cpu)
 	return 0;
 }
 
-void mlx5e_trigger_napi_icosq(struct mlx5e_channel *c)
+static void mlx5e_trigger_napi_icosq_post_sync(struct mlx5e_channel *c)
 {
 	struct mlx5e_icosq *sq = &c->icosq;
 	bool locked;
 
-	set_bit(MLX5E_SQ_STATE_LOCK_NEEDED, &sq->state);
-	synchronize_net();
-
 	locked = mlx5e_icosq_sync_lock(sq);
 	mlx5e_trigger_irq(sq);
 	mlx5e_icosq_sync_unlock(sq, locked);
@@ -2852,6 +2849,13 @@ void mlx5e_trigger_napi_icosq(struct mlx5e_channel *c)
 	clear_bit(MLX5E_SQ_STATE_LOCK_NEEDED, &sq->state);
 }
 
+void mlx5e_trigger_napi_icosq(struct mlx5e_channel *c)
+{
+	set_bit(MLX5E_SQ_STATE_LOCK_NEEDED, &c->icosq.state);
+	synchronize_net();
+	mlx5e_trigger_napi_icosq_post_sync(c);
+}
+
 void mlx5e_trigger_napi_async_icosq(struct mlx5e_channel *c)
 {
 	struct mlx5e_icosq *sq = c->async_icosq;
@@ -3099,6 +3103,19 @@ int mlx5e_open_channels(struct mlx5e_priv *priv,
 	return err;
 }
 
+static void mlx5e_trigger_napi_icosqs(struct mlx5e_channels *chs)
+{
+	int i;
+
+	for (i = 0; i < chs->num; i++)
+		set_bit(MLX5E_SQ_STATE_LOCK_NEEDED, &chs->c[i]->icosq.state);
+
+	synchronize_net();
+
+	for (i = 0; i < chs->num; i++)
+		mlx5e_trigger_napi_icosq_post_sync(chs->c[i]);
+}
+
 static void mlx5e_activate_channels(struct mlx5e_priv *priv, struct mlx5e_channels *chs)
 {
 	int i;
@@ -3109,8 +3126,7 @@ static void mlx5e_activate_channels(struct mlx5e_priv *priv, struct mlx5e_channe
 	if (priv->htb)
 		mlx5e_qos_activate_queues(priv);
 
-	for (i = 0; i < chs->num; i++)
-		mlx5e_trigger_napi_icosq(chs->c[i]);
+	mlx5e_trigger_napi_icosqs(chs);
 
 	if (chs->ptp)
 		mlx5e_ptp_activate_channel(chs->ptp);
-- 
2.44.0

Re: [PATCH net-next 0/2] net/mlx5e: Batch RCU grace periods on channel open and close

[Toke Høiland-Jørgensen]

Tariq Toukan <tariqt@nvidia.com> writes:

> Hi,
>
> This series by Carolina reduces the time mlx5e spends waiting for RCU
> grace periods when bringing an interface up or down with many channels.
>
> Today the channel open and close flows call synchronize_net() per
> object. On close, RQ, ICOSQ, TX SQ and NAPI teardown each require a
> grace period, so the number of waits scales with the number of channels.
> TX SQ teardown adds a wait per traffic class, and XDP and QoS can add
> more. On activation, mlx5e_activate_channels() triggers the ICOSQ of
> every channel, and each trigger waits for its own grace period. With a
> large channel count this makes link down and link up slow.
>
> Patch 1 batches the waits on channel close. Channel deactivate and close
> are split into pre_sync and post_sync phases: the ENABLED state is first
> cleared on all queues of all channels, a single grace period is awaited,
> and only then does the teardown continue. NAPI instances are likewise
> unlinked with __netif_napi_del_locked(), followed by one
> synchronize_net() before the channels are freed. The existing per object
> synchronize_net() calls are kept for single queue paths. The change
> touches the regular channels as well as the PTP, QoS and XSK queues.
>
> Patch 2 does the same for the ICOSQ trigger on activation: all channel
> ICOSQs are first marked as requiring locking, one grace period is
> awaited, and then all of them are triggered. mlx5e_trigger_napi_icosq()
> keeps its existing semantics for standalone callers.
>
> Results with 248 channels and rcu_normal=1:
> - ip link set eth2 down: 10.183s before, 0.715s after.
> - ip link set eth2 up: 3.095s before, 1.794s after.

Nice! Thanks for tackling this :)

Don't know the driver well enough to comment on the details, but in
principle, emphatically:

Acked-by: Toke Høiland-Jørgensen <toke@kernel.org>