Re: [RFC net-next 7/7] net/mlx5e: kTLS, Keep a TX connection on one SQ at a time

[Rishikesh Jethwani]

On Mon, Nov 9, 2026 at 3:50 AM Tariq Toukan <tariqt@xxxxxxxxxx> wrote:
>
> A kTLS TX connection must not post WQEs to another SQ while WQEs that it
> posted to its SQ may be pending: both SQs would update its TIS state
> with no ordering, corrupting the records on the wire. The stack and the
> driver keep the TX queue of such a connection stable
> (TLSDEV_F_STICKY_TXQ), but some paths bypass or defeat that: tc skbedit
> queue_mapping skips queue selection, sk_tx_queue_mapping is shared with
> the other devices on the route, and ooo_okay can be stale after an
> upper device's qdisc reordered the socket's skbs.
>
> Enforce it where the WQEs are posted. Each connection records the TX
> queue that it posts on. An skb on another TX queue moves the connection
> only if no other skb of its socket is in flight. An skb is freed only
> once the WQEs posted for it completed, or were destroyed with their SQ,
> so the connection then has no WQE pending on any SQ. An SQ that
> replaces another on the same TX queue was drained or destroyed first.
> Otherwise the skb is dropped before anything is posted, and counted in
> a new tx[i]_tls_drop_txq_move counter. TCP retransmits it.
>
> The move is made under the old TX queue's lock, taken with a trylock,
> under which the connection's skbs are posted there, so it cannot race
> with them. A connection that stays on its TX queue only compares the
> queue index.
>
> A move that the TX queue selection allows, when the socket has nothing
> else in flight, is made at selection. Otherwise the socket's next skbs,
> queued behind the first one on the new TX queue, would hold
> sk_wmem_alloc when it reaches the driver, and it would be dropped,
> although nothing of the connection is pending on the old SQ.
>
> Tested on ConnectX-6 Dx, with 128 channels and 32 kTLS TX offloaded
> connections streaming pattern data for 60 seconds to a software kTLS
> receiver that checks every byte, while an egress tc skbedit
> queue_mapping filter flips them between two TX queues every 20 ms.
> Without this patch, 23 of 55 connections failed decryption. With it,
> none did, and 7515 skbs were dropped over 2035 flips. Without the flips,
> no skb was dropped.
>
> Suggested-by: Rishikesh Jethwani <rjethwani@xxxxxxxxxxxxxxxx>
> Signed-off-by: Tariq Toukan <tariqt@xxxxxxxxxx>
> ---
>  .../ethernet/mellanox/mlx5/counters.rst       |  7 ++
>  .../net/ethernet/mellanox/mlx5/core/en/selq.c | 16 +++--
>  .../mellanox/mlx5/core/en_accel/ktls_tx.c     | 71 +++++++++++++++++++
>  .../mellanox/mlx5/core/en_accel/ktls_txrx.h   |  6 ++
>  .../ethernet/mellanox/mlx5/core/en_stats.c    |  4 ++
>  .../ethernet/mellanox/mlx5/core/en_stats.h    |  2 +
>  6 files changed, 100 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/networking/device_drivers/ethernet/mellanox/mlx5/counters.rst b/Documentation/networking/device_drivers/ethernet/mellanox/mlx5/counters.rst
> index cc498895f92e..0a08ffddc834 100644
> --- a/Documentation/networking/device_drivers/ethernet/mellanox/mlx5/counters.rst
> +++ b/Documentation/networking/device_drivers/ethernet/mellanox/mlx5/counters.rst
> @@ -711,6 +711,13 @@ the software port.
>         data.
>       - Acceleration
>
> +   * - `tx[i]_tls_drop_txq_move`
> +     - Number of TLS packets dropped on ring i because their connection still
> +       had packets in flight on another ring. Posting them on ring i would
> +       break the ordering of the connection's TLS state updates in the
> +       device. TCP retransmits the dropped data.
> +     - Error
> +
>     * - `ptp_cq[i]_abort`
>       - Number of times a CQE has to be skipped in precision time protocol due to
>         a skew between the port timestamp and CQE timestamp being greater than
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/selq.c b/drivers/net/ethernet/mellanox/mlx5/core/en/selq.c
> index dfaee2c3624e..0bac217bbfec 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en/selq.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en/selq.c
> @@ -9,6 +9,7 @@
>  #include "en.h"
>  #include "en/ptp.h"
>  #include "en/htb.h"
> +#include "en_accel/ktls_txrx.h"
>
>  struct mlx5e_selq_params {
>         unsigned int num_regular_queues;
> @@ -288,9 +289,6 @@ static int mlx5e_select_sticky_queue(struct net_device *dev,
>  {
>         int txq_ix;
>
> -       if (mlx5e_sk_txq_may_move(skb))
> -               return -1;
> -
>         txq_ix = sk_tx_queue_get(skb->sk);
>         if (txq_ix < 0 || txq_ix >= dev->real_num_tx_queues)
>                 return -1;
> @@ -309,6 +307,7 @@ u16 mlx5e_select_queue(struct net_device *dev, struct sk_buff *skb,
>         struct mlx5e_priv *priv = netdev_priv(dev);
>         struct mlx5e_selq_params *selq;
>         struct sock *sk = skb->sk;
> +       bool may_move;
>         int txq_ix;
>
>         selq = rcu_dereference_bh(priv->selq.active);
> @@ -323,14 +322,19 @@ u16 mlx5e_select_queue(struct net_device *dev, struct sk_buff *skb,
>         if (likely(!mlx5e_sk_sticky_txq(sk)))
>                 return __mlx5e_select_queue(dev, skb, selq);
>
> -       txq_ix = mlx5e_select_sticky_queue(dev, skb, selq);
> -       if (txq_ix >= 0)
> -               return txq_ix;
> +       may_move = mlx5e_sk_txq_may_move(skb);
> +       if (!may_move) {
> +               txq_ix = mlx5e_select_sticky_queue(dev, skb, selq);
> +               if (txq_ix >= 0)
> +                       return txq_ix;
> +       }
>
>         txq_ix = __mlx5e_select_queue(dev, skb, selq);
>         /* Cache the final queue, as netdev_pick_tx() does with its own pick. */
>         if (rcu_access_pointer(sk->sk_dst_cache))
>                 sk_tx_queue_set(sk, txq_ix);
> +       if (may_move)
> +               mlx5e_ktls_tx_select_txq(dev, skb, txq_ix);
>
>         return txq_ix;
>  }
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ktls_tx.c b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ktls_tx.c
> index 0bd4d961e704..9603ef4b3eb3 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ktls_tx.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ktls_tx.c
> @@ -92,12 +92,15 @@ enum {
>         MLX5E_KTLS_TX_RESYNC, /* WQEs lost, see mlx5e_ktls_tx_handle_lost() */
>  };
>
> +#define MLX5E_KTLS_TX_NO_TXQ (-1)
> +
>  struct mlx5e_ktls_offload_context_tx {
>         /* fast path */
>         u32 expected_seq;
>         u32 tisn;
>         bool ctx_post_pending;
>         unsigned long flags; /* MLX5E_KTLS_TX_* */
> +       int txq_ix; /* that it posts on, see mlx5e_ktls_tx_own_txq() */
>         /* control / resync */
>         struct list_head list_node; /* member of the pool */
>         union mlx5e_crypto_info crypto_info;
> @@ -505,6 +508,7 @@ int mlx5e_ktls_add_tx(struct net_device *netdev, struct sock *sk,
>
>         priv_tx->ctx_post_pending = true;
>         priv_tx->flags = 0;
> +       priv_tx->txq_ix = MLX5E_KTLS_TX_NO_TXQ;
>         atomic64_inc(&priv_tx->sw_stats->tx_tls_ctx);
>
>         return 0;
> @@ -846,6 +850,59 @@ void mlx5e_ktls_tx_handle_lost(struct mlx5e_txqsq *sq, struct sk_buff *skb)
>         rcu_read_unlock();
>  }
>
> +/* A connection must not post WQEs to another SQ while WQEs that it posted to
> + * its SQ may be pending, see TLSDEV_F_STICKY_TXQ. Let it move to the TX queue
> + * of @sq only if no other skb of its socket is in flight: an skb is freed only
> + * once the WQEs posted for it are done. The move is made under the lock of the
> + * old TX queue, under which the connection posts there.
> + */
> +static bool mlx5e_ktls_tx_own_txq(struct net_device *netdev,
> +                                 struct mlx5e_ktls_offload_context_tx *priv_tx,
> +                                 struct mlx5e_txqsq *sq, struct sk_buff *skb)
> +{
> +       int old = READ_ONCE(priv_tx->txq_ix);
> +       struct netdev_queue *old_txq;
> +       bool own;
> +
> +       if (old == MLX5E_KTLS_TX_NO_TXQ)
> +               return cmpxchg(&priv_tx->txq_ix, old, sq->txq_ix) == old;
> +
> +       old_txq = netdev_get_tx_queue(netdev, old);
> +       if (!__netif_tx_trylock(old_txq))
> +               return false;
> +
> +       own = READ_ONCE(priv_tx->txq_ix) == old &&
> +             sk_wmem_alloc_get(skb->sk) <= skb->truesize;

This serializes the ownership check and update with posting on the old
queue. But selection bypasses it:

> +       if (own)
> +               WRITE_ONCE(priv_tx->txq_ix, sq->txq_ix);
> +       __netif_tx_unlock(old_txq);
> +
> +       return own;
> +}
> +
> +/* The TX queue selection lets the socket of @skb move to @txq_ix only while
> + * no other skb of the socket is in flight, see mlx5e_select_queue(), so the
> + * connection has no WQE pending on any SQ: hand it the new TX queue now. Once
> + * @skb is queued there, the socket's next skbs queue up behind it and hold
> + * sk_wmem_alloc, and mlx5e_ktls_tx_own_txq() would refuse the move.
> + */
> +void mlx5e_ktls_tx_select_txq(struct net_device *netdev, struct sk_buff *skb,
> +                             u16 txq_ix)
> +{
> +       struct mlx5e_ktls_offload_context_tx *priv_tx;
> +       struct tls_context *tls_ctx;
> +
> +       if (!tls_is_skb_tx_device_offloaded(skb))
> +               return;
> +
> +       tls_ctx = tls_get_ctx(skb->sk);
> +       if (rcu_dereference_bh(tls_ctx->netdev) != netdev)
> +               return;
> +
> +       priv_tx = mlx5e_get_ktls_tx_priv_ctx(tls_ctx);
> +       WRITE_ONCE(priv_tx->txq_ix, txq_ix);
> +}
> +

This write relies on an earlier may_move check in mlx5e_select_queue(),
without locking the old queue or rechecking wmem.

With independent bond child qdiscs and skbedit selecting different upper
TX queues for A and B:

  1. A is dequeued asynchronously and reaches the lower mlx5e selector.
     Only A holds wmem, so may_move=true.
  2. TCP emits B through another child qdisc. B sees both charges,
     reuses cached lower SQ0 and posts; its WQEs remain pending.
  3. A resumes, picks SQ1 and overwrites the owner. The xmit guard sees
     a matching index, so A posts on SQ1 while B's WQEs still reference
     the same TIS from SQ0.

A can be dequeued via a qdisc watchdog outside the socket lock. Lower
selection precedes the lower TX lock. Both packets use the socket's
offload slave. The same race exists when the owner is unclaimed and B
claims it first.

B must select and post between A's may_move check and owner write in
one mlx5e_select_queue() call. Hard IRQs, vCPU descheduling and
PREEMPT_RT preemption of BH-disabled sections can extend that short
window while B runs on another CPU.

Selection needs the same initial-claim CAS and locked handoff: recheck
the owner and current wmem under the old queue's lock, and leave
ownership unchanged on failure. Locking only the write leaves the wmem
check stale. Preserve zero self-charge for non-decrypted, possibly
partially orphaned skbs.

A failed transfer can fall back to the owner's queue, updating the
cache, if the index is in [0, real_num_tx_queues), still maps to a live
SQ and is not a PTP index. Keep the xmit ownership guard.

>  bool mlx5e_ktls_handle_tx_skb(struct net_device *netdev, struct mlx5e_txqsq *sq,
>                               struct sk_buff *skb,
>                               struct mlx5e_accel_tx_tls_state *state)
> @@ -881,6 +938,10 @@ bool mlx5e_ktls_handle_tx_skb(struct net_device *netdev, struct mlx5e_txqsq *sq,
>
>         priv_tx = mlx5e_get_ktls_tx_priv_ctx(tls_ctx);
>
> +       if (unlikely(READ_ONCE(priv_tx->txq_ix) != sq->txq_ix) &&
> +           !mlx5e_ktls_tx_own_txq(netdev, priv_tx, sq, skb))
> +               goto err_move;
> +
>         /* WQEs naming the TIS, pending context params included, are
>          * posted only for an skb that uses it.
>          */
> @@ -929,6 +990,16 @@ bool mlx5e_ktls_handle_tx_skb(struct net_device *netdev, struct mlx5e_txqsq *sq,
>
>         return true;
>
> +err_move:
> +       /* Nothing was posted for the skb, TCP retransmits it. */
> +       stats->tls_drop_txq_move++;
> +       netdev_warn_once(netdev,
> +                        "kTLS TX connection moved to TX queue %d with packets in flight, dropping\n",
> +                        sq->txq_ix);
> +       dev_kfree_skb_any(skb);
> +       mlx5e_tx_flush(sq, NULL);
> +       return false;
> +
>  err_lost:
>         /* Resync params may have moved the TIS state to the dropped skb. */
>         set_bit(MLX5E_KTLS_TX_RESYNC, &priv_tx->flags);
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ktls_txrx.h b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ktls_txrx.h
> index a6b15dcd32b8..acfd1b4025aa 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ktls_txrx.h
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ktls_txrx.h
> @@ -30,6 +30,8 @@ void mlx5e_ktls_tx_handle_resync_dump_comp(struct mlx5e_txqsq *sq,
>                                            struct mlx5e_tx_wqe_info *wi,
>                                            u32 *dma_fifo_cc);
>  void mlx5e_ktls_tx_handle_lost(struct mlx5e_txqsq *sq, struct sk_buff *skb);
> +void mlx5e_ktls_tx_select_txq(struct net_device *netdev, struct sk_buff *skb,
> +                             u16 txq_ix);
>
>  /* The WQEs posted for the skb may have not been executed. */
>  static inline void
> @@ -84,6 +86,10 @@ mlx5e_ktls_tx_handle_lost(struct mlx5e_txqsq *sq, struct sk_buff *skb) {}
>  static inline void
>  mlx5e_ktls_tx_try_handle_lost(struct mlx5e_txqsq *sq, struct sk_buff *skb) {}
>
> +static inline void
> +mlx5e_ktls_tx_select_txq(struct net_device *netdev, struct sk_buff *skb,
> +                        u16 txq_ix) {}
> +
>  static inline bool
>  mlx5e_ktls_rx_handle_resync_list(struct mlx5e_icosq *sq, int budget)
>  {
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_stats.c b/drivers/net/ethernet/mellanox/mlx5/core/en_stats.c
> index cd94bb44f6ab..d23790403714 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_stats.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_stats.c
> @@ -132,6 +132,7 @@ static const struct counter_desc sw_stats_desc[] = {
>         { MLX5E_DECLARE_STAT(struct mlx5e_sw_stats, tx_tls_skip_no_sync_data) },
>         { MLX5E_DECLARE_STAT(struct mlx5e_sw_stats, tx_tls_drop_no_sync_data) },
>         { MLX5E_DECLARE_STAT(struct mlx5e_sw_stats, tx_tls_drop_bypass_req) },
> +       { MLX5E_DECLARE_STAT(struct mlx5e_sw_stats, tx_tls_drop_txq_move) },
>  #endif
>
>         { MLX5E_DECLARE_STAT(struct mlx5e_sw_stats, rx_lro_packets) },
> @@ -441,6 +442,7 @@ static void mlx5e_stats_grp_sw_update_stats_sq(struct mlx5e_sw_stats *s,
>         s->tx_tls_skip_no_sync_data += sq_stats->tls_skip_no_sync_data;
>         s->tx_tls_drop_no_sync_data += sq_stats->tls_drop_no_sync_data;
>         s->tx_tls_drop_bypass_req   += sq_stats->tls_drop_bypass_req;
> +       s->tx_tls_drop_txq_move += sq_stats->tls_drop_txq_move;
>  #endif
>         s->tx_cqes                  += sq_stats->cqes;
>  }
> @@ -2303,6 +2305,7 @@ static const struct counter_desc sq_stats_desc[] = {
>         { MLX5E_DECLARE_TX_STAT(struct mlx5e_sq_stats, tls_skip_no_sync_data) },
>         { MLX5E_DECLARE_TX_STAT(struct mlx5e_sq_stats, tls_drop_no_sync_data) },
>         { MLX5E_DECLARE_TX_STAT(struct mlx5e_sq_stats, tls_drop_bypass_req) },
> +       { MLX5E_DECLARE_TX_STAT(struct mlx5e_sq_stats, tls_drop_txq_move) },
>  #endif
>         { MLX5E_DECLARE_TX_STAT(struct mlx5e_sq_stats, csum_none) },
>         { MLX5E_DECLARE_TX_STAT(struct mlx5e_sq_stats, stopped) },
> @@ -2456,6 +2459,7 @@ static const struct counter_desc qos_sq_stats_desc[] = {
>         { MLX5E_DECLARE_QOS_TX_STAT(struct mlx5e_sq_stats, tls_skip_no_sync_data) },
>         { MLX5E_DECLARE_QOS_TX_STAT(struct mlx5e_sq_stats, tls_drop_no_sync_data) },
>         { MLX5E_DECLARE_QOS_TX_STAT(struct mlx5e_sq_stats, tls_drop_bypass_req) },
> +       { MLX5E_DECLARE_QOS_TX_STAT(struct mlx5e_sq_stats, tls_drop_txq_move) },
>  #endif
>         { MLX5E_DECLARE_QOS_TX_STAT(struct mlx5e_sq_stats, csum_none) },
>         { MLX5E_DECLARE_QOS_TX_STAT(struct mlx5e_sq_stats, stopped) },
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_stats.h b/drivers/net/ethernet/mellanox/mlx5/core/en_stats.h
> index 09f155acb461..8f5fb0478f00 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_stats.h
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_stats.h
> @@ -237,6 +237,7 @@ struct mlx5e_sw_stats {
>         u64 tx_tls_skip_no_sync_data;
>         u64 tx_tls_drop_no_sync_data;
>         u64 tx_tls_drop_bypass_req;
> +       u64 tx_tls_drop_txq_move;
>
>         u64 rx_tls_decrypted_packets;
>         u64 rx_tls_decrypted_bytes;
> @@ -434,6 +435,7 @@ struct mlx5e_sq_stats {
>         u64 tls_skip_no_sync_data;
>         u64 tls_drop_no_sync_data;
>         u64 tls_drop_bypass_req;
> +       u64 tls_drop_txq_move;
>  #endif
>         /* less likely accessed in data path */
>         u64 csum_none;
> --
> 2.44.0
>