Re: [RFC net-next 4/7] net/mlx5e: Drain old TX SQs before activating new channels

[Rishikesh Jethwani]

On Mon, Nov 9, 2026 at 3:49 AM Tariq Toukan <tariqt@xxxxxxxxxx> wrote:
>
> On devices with tis_tir_td_order, commit 3b88a535a8e1 ("net/mlx5e:
> Defer channels closure to reduce interface down time") closes the old
> channels only after the new ones are activated. Their SQs are
> deactivated before that, which rings a final doorbell, but nothing
> waits for the HW to execute their pending WQEs: the completions of a
> deactivated SQ are not polled, and the SQ is destroyed only when its
> channel is closed. Meanwhile, the new SQ of the same TX queue starts
> transmitting. A flow's WQEs still pending on the old SQ can then
> execute after its newer WQEs on the new SQ, as WQE fences order only
> within an SQ.
>
> Besides reordering packets, this breaks kTLS TX offload. The WQEs of a
> connection update its TIS crypto and record tracking state as they
> execute. When old WQEs execute after new ones, e.g. because a PFC
> pause held the old SQ back, the TIS state gets out of sync with the
> TCP stream, and the connection's records go out corrupted.
>
> Before activating the new channels, drain the TX SQs of the old ones:
> reap their completions until all their WQEs have completed. If they
> do not drain in time, e.g. under a long pause, warn, and close the old
> channels before activating the new ones, as before that commit. Move
> the deferred closure into mlx5e_switch_priv_channels(), next to that
> decision.
>
> Pointed out by Rishikesh Jethwani while discussing kTLS TX TIS reuse
> for TLS 1.3 rekey on netdev.
>
> Reproduced on ConnectX-6 Dx, with 248 channels, by changing the
> channel count between 247 and 248 every 2 seconds while 32 kTLS TX
> offloaded connections stream pattern data to a software kTLS receiver
> that checks every byte. On this setup, the new channels are activated
> about 230 ms after the old ones are deactivated, so the old SQs must
> still hold WQEs by then: with every TX queue rate limited to 10 Mb/s,
> 9 of 41 connections failed decryption in 60 seconds, and 11 of 43 in
> 120 seconds. With this patch, no record failed in five such runs, 7
> minutes in total; the drain timed out and fell back to closing the old
> channels first 7 times. At 1 Gb/s per queue, the old SQs always
> drained in time.
>
> Fixes: 3b88a535a8e1 ("net/mlx5e: Defer channels closure to reduce interface down time")
> Reported-by: Rishikesh Jethwani <rjethwani@xxxxxxxxxxxxxxxx>
> Closes: https://lore.kernel.org/netdev/CAKaoeS1hj9XuM7ojLBCjGA=EXc2M-b+eRjC2Sk=8L_8XYeJ18w@xxxxxxxxxxxxxx/
> Signed-off-by: Tariq Toukan <tariqt@xxxxxxxxxx>
> ---
>  .../net/ethernet/mellanox/mlx5/core/en/txrx.h |  1 +
>  .../net/ethernet/mellanox/mlx5/core/en_main.c | 88 ++++++++++++++++++-
>  .../net/ethernet/mellanox/mlx5/core/en_tx.c   | 41 +++++++--
>  3 files changed, 118 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/txrx.h b/drivers/net/ethernet/mellanox/mlx5/core/en/txrx.h
> index 24e91279243e..b5bd1b6e8601 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en/txrx.h
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en/txrx.h
> @@ -112,6 +112,7 @@ struct mlx5e_xmit_data_frags {
>
>  netdev_tx_t mlx5e_xmit(struct sk_buff *skb, struct net_device *dev);
>  bool mlx5e_poll_tx_cq(struct mlx5e_cq *cq, int napi_budget);
> +bool mlx5e_txqsq_drain(struct mlx5e_txqsq *sq);
>  void mlx5e_free_txqsq_descs(struct mlx5e_txqsq *sq);
>
>  static inline bool
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> index c3c2c5720c6f..a721e30bcaa8 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> @@ -3421,6 +3421,82 @@ static int mlx5e_switch_priv_params(struct mlx5e_priv *priv,
>         return 0;
>  }
>
> +/* Time for the TX SQs of deactivated channels to complete their WQEs. */
> +#define MLX5E_SQ_DRAIN_TIMEOUT 100 /* msecs */
> +
> +/* Returns 1 if the SQ still has pending WQEs, and records the first such SQ. */
> +static int mlx5e_drain_txqsq(struct mlx5e_txqsq *sq,
> +                            struct mlx5e_txqsq **pending)
> +{
> +       if (mlx5e_txqsq_drain(sq))
> +               return 0;
> +
> +       if (!*pending)
> +               *pending = sq;
> +
> +       return 1;
> +}
> +
> +/* Returns the number of the channel's TX SQs that still have pending WQEs. */
> +static int mlx5e_drain_channel_txqsqs(struct mlx5e_channel *c,
> +                                     struct mlx5e_txqsq **pending)
> +{
> +       struct mlx5e_txqsq __rcu **qos_sqs;
> +       int num_pending = 0;
> +       int i;
> +
> +       for (i = 0; i < c->num_tc; i++)
> +               num_pending += mlx5e_drain_txqsq(&c->sq[i], pending);
> +
> +       qos_sqs = mlx5e_state_dereference(c->priv, c->qos_sqs);
> +       if (!qos_sqs)
> +               return num_pending;
> +
> +       for (i = 0; i < c->qos_sqs_size; i++) {
> +               struct mlx5e_txqsq *sq;
> +
> +               sq = mlx5e_state_dereference(c->priv, qos_sqs[i]);
> +               if (sq)
> +                       num_pending += mlx5e_drain_txqsq(sq, pending);
> +       }
> +
> +       return num_pending;
> +}
> +
> +/* Wait for the TX SQs of deactivated channels to complete all their WQEs,
> + * so that none of them executes concurrently with the new SQs.
> + * The PTP SQs are not waited for, they carry PTP event packets only.
> + */

The regular/QoS drain and the timeout ordering look right to me. The
PTP SQs can carry TLS data, though. Their indices are in
real_num_tx_queues and txq2sq, and egress skbedit can select them,
bypassing mlx5e_select_queue(). mlx5e_xmit() runs the kTLS handling on
any SQ, and does not reject a non-timestamped skb on a PTP SQ.

If a PTP SQ holds TLS WQEs during a channel replacement, the drain can
succeed while its TX queue index is reused, by the replacement PTP SQ
or by a new regular SQ after a channel count change. With one TC,
growing from 247 to 248 channels changes index 247 from PTP to the new
regular channel's SQ. A fixed skbedit rule still selects 247, and patch
7's owner-index comparison permits new TLS work there while the old
PTP SQ is still pending and not yet destroyed.

I'd prefer to include the PTP SQs in the drain/close ordering.
Rejecting TLS data on them before any connection-state change or TLS
WQE posting would also be safe, but with a fixed skbedit rule every
retransmit goes back to the same queue and is dropped again, stalling
the connection. Excluding their indices only in queue selection does
not cover skbedit.

> +static bool mlx5e_drain_channels_txqsqs(struct mlx5e_channels *chs)
> +{
> +       unsigned long exp_time;
> +
> +       exp_time = jiffies + msecs_to_jiffies(MLX5E_SQ_DRAIN_TIMEOUT);
> +       for (;;) {
> +               struct mlx5e_txqsq *pending = NULL;
> +               int num_pending = 0;
> +               int i;
> +
> +               for (i = 0; i < chs->num; i++)
> +                       num_pending += mlx5e_drain_channel_txqsqs(chs->c[i],
> +                                                                 &pending);
> +
> +               if (!num_pending)
> +                       return true;
> +
> +               if (time_after(jiffies, exp_time)) {
> +                       netdev_warn(pending->netdev,
> +                                   "Old TX SQs not drained in %d msecs (%d pending, e.g. SQ 0x%x txq %d cc 0x%x pc 0x%x), closing the old channels first\n",
> +                                   MLX5E_SQ_DRAIN_TIMEOUT, num_pending,
> +                                   pending->sqn, pending->txq_ix,
> +                                   pending->cc, pending->pc);
> +                       return false;
> +               }
> +
> +               usleep_range(10, 20);
> +       }
> +}
> +
>  static int mlx5e_switch_priv_channels(struct mlx5e_priv *priv,
>                                       struct mlx5e_channels *old_chs,
>                                       struct mlx5e_channels *new_chs,
> @@ -3428,6 +3504,7 @@ static int mlx5e_switch_priv_channels(struct mlx5e_priv *priv,
>                                       void *context)
>  {
>         struct net_device *netdev = priv->netdev;
> +       bool defer_close = false;
>         int carrier_ok;
>         int err = 0;
>
> @@ -3450,7 +3527,10 @@ static int mlx5e_switch_priv_channels(struct mlx5e_priv *priv,
>         }
>
>         mlx5e_set_xdp_feature(priv);
> -       if (!MLX5_CAP_GEN(priv->mdev, tis_tir_td_order))
> +
> +       defer_close = MLX5_CAP_GEN(priv->mdev, tis_tir_td_order) &&
> +                     mlx5e_drain_channels_txqsqs(old_chs);
> +       if (!defer_close)
>                 mlx5e_close_channels(old_chs);
>         priv->profile->update_rx(priv);
>
> @@ -3462,6 +3542,9 @@ static int mlx5e_switch_priv_channels(struct mlx5e_priv *priv,
>         if (carrier_ok)
>                 netif_carrier_on(netdev);
>
> +       if (defer_close)
> +               mlx5e_close_channels(old_chs);
> +
>         return err;
>  }
>
> @@ -3499,9 +3582,6 @@ int mlx5e_safe_switch_params(struct mlx5e_priv *priv,
>         if (err)
>                 goto err_close;
>
> -       if (MLX5_CAP_GEN(priv->mdev, tis_tir_td_order))
> -               mlx5e_close_channels(old_chs);
> -
>         kfree(new_chs);
>         kfree(old_chs);
>         return 0;
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tx.c b/drivers/net/ethernet/mellanox/mlx5/core/en_tx.c
> index bee6f0bf1eff..eb20e3d14d46 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_tx.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tx.c
> @@ -780,10 +780,11 @@ void mlx5e_txqsq_wake(struct mlx5e_txqsq *sq)
>         }
>  }
>
> -bool mlx5e_poll_tx_cq(struct mlx5e_cq *cq, int napi_budget)
> +/* Returns the number of polled CQEs. */
> +static int mlx5e_poll_tx_cqes(struct mlx5e_txqsq *sq, int napi_budget)
>  {
> +       struct mlx5e_cq *cq = &sq->cq;
>         struct mlx5e_sq_stats *stats;
> -       struct mlx5e_txqsq *sq;
>         struct mlx5_cqe64 *cqe;
>         u32 dma_fifo_cc;
>         u32 nbytes;
> @@ -791,14 +792,9 @@ bool mlx5e_poll_tx_cq(struct mlx5e_cq *cq, int napi_budget)
>         u16 sqcc;
>         int i;
>
> -       sq = container_of(cq, struct mlx5e_txqsq, cq);
> -
> -       if (unlikely(!test_bit(MLX5E_SQ_STATE_ENABLED, &sq->state)))
> -               return false;
> -
>         cqe = mlx5_cqwq_get_cqe(&cq->wq);
>         if (!cqe)
> -               return false;
> +               return 0;
>
>         stats = sq->stats;
>
> @@ -891,11 +887,40 @@ bool mlx5e_poll_tx_cq(struct mlx5e_cq *cq, int napi_budget)
>
>         netdev_tx_completed_queue(sq->txq, npkts, nbytes);
>
> +       return i;
> +}
> +
> +bool mlx5e_poll_tx_cq(struct mlx5e_cq *cq, int napi_budget)
> +{
> +       struct mlx5e_txqsq *sq = container_of(cq, struct mlx5e_txqsq, cq);
> +       int i;
> +
> +       if (unlikely(!test_bit(MLX5E_SQ_STATE_ENABLED, &sq->state)))
> +               return false;
> +
> +       i = mlx5e_poll_tx_cqes(sq, napi_budget);
> +       if (!i)
> +               return false;
> +
>         mlx5e_txqsq_wake(sq);
>
>         return (i == MLX5E_TX_CQ_POLL_BUDGET);
>  }
>
> +/* Reap the completions of a deactivated SQ, whose NAPI is disabled, without
> + * waking its TX queue. Returns true once all its WQEs have completed.
> + */
> +bool mlx5e_txqsq_drain(struct mlx5e_txqsq *sq)
> +{
> +       int polled;
> +
> +       do {
> +               polled = mlx5e_poll_tx_cqes(sq, 0);
> +       } while (polled == MLX5E_TX_CQ_POLL_BUDGET);
> +
> +       return sq->cc == sq->pc;
> +}
> +
>  static void mlx5e_tx_wi_kfree_fifo_skbs(struct mlx5e_txqsq *sq, struct mlx5e_tx_wqe_info *wi)
>  {
>         int i;
> --
> 2.44.0
>