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

[Tariq Toukan <tariqt@nvidia.com>]

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@everpuredata.com>
Signed-off-by: Tariq Toukan <tariqt@nvidia.com>
---
 .../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;
+	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);
+}
+
 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