Re: Question: net/mlx5e kTLS TX TIS reuse vs in-flight WQEs

[Tariq Toukan <tariqt@nvidia.com>]

On 06/11/2026 21:58, Rishikesh Jethwani wrote:
 > Thanks. Agreed on (3a), (i) and the net fixes, with one condition on
 > (i) below.

Hi Rishikesh,

Thanks. I took your points into the patches, details inline. I'm
re-running the ConnectX-6 Dx tests with the changes, and will share the
RFCs once they pass.

 > Thanks. Until then, the two hardware questions remain open, including
 > whether DESTROY_SQ can fail; the destroy wrapper discards the status.

DESTROY_SQ doesn't fail while the function is healthy. If it does,
something went badly wrong in the firmware.

 > One correction to my last mail: recovery clears RECOVERING even on a
 > flush timeout, so the queue can wake once stop_room frees.

Thanks. WQEs posted to that SQ then complete with error CQEs, which mark
their connections for a full resync, as in the first error.

 > Agreed. What rekey needs from that resync fix: a lost initialization
 > or replacement always triggers a full resync on the next skb that
 > needs the TLS context, and a connection marked again while the driver
 > is consuming an earlier mark stays marked (test-and-clear, not
 > clear-after).

Both hold now. The mark stays until an skb that uses the context
consumes it, and it is taken with a test-and-clear before the resync is
posted, so a loss reported meanwhile marks the connection again.

 > An explicit bypass right after the
 > datalen check in mlx5e_ktls_handle_tx_skb() closes that without a
 > sequence comparison:
 >
 >         if (!skb_is_decrypted(skb))
 >                 return true;

Agreed, it's in the pass-through patch, with credit.

 > I'm happy to use (i), but warning/counting alone does not cover the
 > forced-queue case: a detected conflict has to prevent the conflicting
 > hardware access, since old TIS accesses must have retired before
 > another SQ can modify the context. Dropping before any TLS WQE or
 > context-state change would preserve safety without SW crypto.

Agreed, the counter is now a drop. 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
nothing of the connection is then pending on any SQ. Otherwise the skb
is dropped before anything is posted, and counted, and TCP retransmits
it.

The move is made under the old TX queue's lock, taken with a trylock,
under which the connection posts there. That serializes the check and
the publication with that path, and leaves the connection's datapath
with a compare of the queue index.

This covers your points 1 and 2 where the WQEs are posted, wherever the
queue came from. For 2, the mlx5e pin now also re-checks wmem at
selection as you suggest, so a stale ooo_okay doesn't lead to drops in
the first place. Does that match what you had in mind?

 > 3. The drain has to cover the QoS/HTB SQs too, and reap deactivated
 >    SQs although mlx5e_poll_tx_cq() returns early once ENABLED is
 >    clear; a missing or replaced txq2sq entry isn't evidence of
 >    retirement. On a drain timeout, the fallback must close the old
 >    SQs first (which keeps the guarantee, modulo DESTROY_SQ), never
 >    activate with them alive.

That's how the drain works: it covers the QoS SQs, reaps the deactivated
SQs directly, and on a timeout closes the old channels before activating
the new ones. The drop above doesn't rely on txq2sq either: an SQ that
is deactivated alone, e.g. on an HTB leaf delete, keeps its skbs until
it is destroyed, so their socket keeps its wmem until then.

 > On the price you mention for priority changes, agreed. Please
 > document in Documentation/networking/tls-offload.rst that pinning
 > delays priority/DSCP/HTB-driven TX queue changes until migration is
 > safe, potentially indefinitely under sustained traffic.

Done.

 > Nice, that's simpler. On the core side, the op's contract: the
 > callback prepares the new key and pending parameters but posts no TLS
 > WQEs for this connection.

[...]

 > Callbacks are serialized per connection, but another may arrive
 > before the previous replacement is posted. On success, the driver
 > must clean up any superseded, never-posted key.

Agreed on the contract.

 > Record lookup also uses 32-bit TCP sequence numbers
 > today without rekey, so a sufficiently delayed clone can alias a live
 > record there too. The floor catches old-key clones within its
 > comparison window, but does not remove that ambiguity. I'll look at
 > closing this in the core separately.

Thanks. On the driver side, the bypass above keeps plaintext skbs out of
the record lookup, so it's down to the decrypted clones.

 > Please Cc me on the RFCs, and let me know which fixes the rekey
 > patches should depend on.

Will do. The rekey patches should go on top of these net fixes:

   net/mlx5e: kTLS, Hold a dropped skb until its posted WQEs complete
   net/mlx5e: kTLS, Resync TX fully after its WQEs were lost
   net/mlx5e: kTLS, Post TX context params only for skbs that use them
   net/mlx5e: Drain old TX SQs before activating new channels
   net: tls: keep the TX queue of a device-offloaded socket stable
   net/mlx5e: kTLS, Keep the TX queue of an offloaded socket stable

The drop goes to net-next separately, as "net/mlx5e: kTLS, Keep a TX
connection on one SQ at a time"; the rekey patches don't depend on it.

Thanks,
Tariq