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

[Tariq Toukan <tariqt@nvidia.com>]

On 04/11/2026 7:01, Rishikesh Jethwani wrote:
 > Thanks, this answers the cross-SQ ordering question. Short version:
 > I agree with (1), and (3a) fits the series; I prefer it. For (2), I
 > agree with the goal, but wmem-gated reselection alone does not keep a
 > connection's work on one SQ in mlx5e; the holes are listed below,
 > with two ways to close them, and I'd like your preference.

Hi Rishikesh,

Thanks, very thorough again. Short version from my side: I'd go with
(i), closing the holes, rather than (ii), and with (3a) along the lines
of your contract. As the queue moves corrupt records today, regardless
of rekey, I'd send (2) and the mlx5e side of (i) to net as fixes; I
could reproduce the corruption on ConnectX-6 Dx. Details inline.
Feedback is welcome, especially where it touches your series.

 > Two hardware questions here, and a third on initial setup under (1):
 >
 >  - Does a completed DESTROY_SQ, or finding an SQ in ERR, guarantee
 >    that none of its WQEs can still access a TIS or DEK? And can
 >    DESTROY_SQ fail while the function is healthy?
[...]
 >  - DEK release in mlx5_crypto_dek_destroy() has two paths. Without
[...]
 >    After all WQEs referencing a DEK have retired, is either path,
 >    including reuse of a pooled id by another connection, safe while
 >    an idle TIS still names that DEK, before replacement static
 >    parameters are posted? Or is today's release only safe because
 >    of the destruct-time ordering?

Good questions. I'll take them to our HW/FW folks, and get back to you
once I have answers I can back up. Meanwhile, what I propose below
relies only on what the close path already relies on today: a CQE
retires all earlier WQEs of its SQ, and once an SQ is destroyed, none
of its WQEs executes.

 >> 1. Pre-existing, independent of rekey: the stranded WQEs above.
[...]
 > Agreed. Three spots for it: on SEND DMA failure, mlx5e_sq_xmit_wqe() and
 > mlx5e_sq_xmit_mpwqe() already post a NOP through mlx5e_tx_flush() but
 > free the skb first, so the skb should be held instead; that NOP asks for
 > a completion because mlx5e_notify_hw() sets MLX5_WQE_CTRL_CQ_UPDATE, not
 > mlx5e_post_nop(). mlx5e_ktls_handle_tx_skb()'s err_out needs the same
 > hold-NOP and a doorbell, since it can leave parameter/DUMP WQEs posted
 > without one. The MPWQE case is reachable too: when TX MPWQE is enabled,
 > mlx5e_sq_xmit_mpwqe() takes any linear, untagged, non-GSO skb with
 > inline mode NONE, which includes a pass-through skb that posted pending
 > parameters and took SKIP_NO_DATA, since it leaves with no TISN.

Right, the patch covers the same three spots: mlx5e_tx_flush() takes
the skb and attaches it to its NOP, which requests a completion through
mlx5e_notify_hw(), and both SEND error paths hand it the skbs of kTLS
sockets, MPWQE included.

 > One correction to "SQ error/reset is covered": error-CQE recovery never
 > runs mlx5e_free_txqsq_descs(). mlx5e_tx_reporter_err_cqe_recover()
 > waits in mlx5e_wait_for_sq_flush() for cc to reach pc, then zeroes both
 > counters, so on success every WQE has been retired by an error CQE and
 > the held skb is released normally.
[...]

Correct, thanks.

 > A related recovery gap exists today without rekey: if a connection's
 > initial static parameters are flushed without executing, recovery does
 > not re-arm ctx_post_pending. A retransmit of the same record can also
 > skip static parameters, so software does not guarantee that the new
 > context gets installed. What does the hardware do with a SEND naming a
 > TIS whose static parameters never executed, recycled or freshly created?
 > The rekey contract below handles the corresponding replacement case;
 > initial setup may need the same treatment.

Agreed, and it is wider than the static params: any lost WQE leaves the
TIS state off from what the driver tracks. WQEs are lost when an skb is
dropped after its kTLS WQEs were posted (or after expected_seq advanced
past it), when an SQ error flushes them, and when an SQ is closed with
WQEs pending. The driver holds the skb in each of these, so I'd mark
its connection right there, and have the next skb of a marked
connection resync fully: static params regardless of rec_seq, progress
params, and a DUMP of the record prefix. That covers initial setup, and
the replacement case of your contract, with no rekey-specific code. I'd
send it to net together with (1).

 > When do you expect to post (1)? I'll base the series on it and can
 > test on ConnectX-6 Dx before you post.

After internal review and testing; I can share the patches as RFCs in
the meantime. (1) goes to net together with the resync fix above and
the fixes for (2) below: the stack part, the mlx5e queue pin, the
channel replacement overlap, and the pass-through skbs. I tested them
on ConnectX-6 Dx, and testing on your side is very welcome, especially
with your series on top.

 >> 2. Pre-existing, and a dependency of rekey: TX queue stability. I'd
[...]
 > Gating ooo_okay, the dst-reset queue clear and txq_reselection_ms
 > on wmem covers every path through sk_tx_queue_mapping. In mlx5e the
 > SQ can still change while that mapping stays put, or bypass it:
 >
 >  - Channel replacement with tis_tir_td_order destroys the old
 >    channels only after the new ones are active (3b88a535a8e1).
 >    mlx5e_deactivate_txqsq() posts a final NOP, but nothing waits for
 >    its CQE, and mlx5e_poll_tx_cq() returns early once ENABLED is
 >    cleared. The deactivated SQ can still hold old-key WQEs (e.g.
 >    under PFC pause) while the same txq index already names the new
 >    SQ, so (3a)'s "same SQ as all old-key WQEs" holds only once the
 >    old SQ is destroyed, which is the DESTROY_SQ question above.

Right, and that overlap is a bug of 3b88a535a8e1 on its own. I'd fix it
by draining the old SQs before activating the new channels: their NAPI
is already disabled, so the switch reaps their completions directly,
and the old channels are still closed after the activation, which keeps
the downtime saving. If they don't drain in time (e.g. under a long
pause), it warns and falls back to closing them first. No kTLS-specific
count is needed.

 >  - With num_tcs > 1, mlx5e_select_queue() adds a priority offset
 >    after netdev_pick_tx() (DSCP under MLX5_QPTS_TRUST_DSCP, else VLAN
 >    prio; HTB by skb->priority), so IP_TOS/IPV6_TCLASS or SO_PRIORITY
 >    changes move the SQ.

mlx5e_select_queue() can keep the final queue of such sockets: cache it
in sk_tx_queue_mapping, and reuse it as long as ooo_okay is clear. A
priority change then takes effect once the connection has nothing in
flight, which for a busy bulk flow can take a while; that's the price
of keeping the order.

 >  - Egress skbedit queue_mapping bypasses ndo_select_queue(), and
 >    SKBEDIT_F_TXQ_SKBHASH can move a flow after a txhash rethink.

I'd document it as not supported with this kind of offload, and have
the driver count it (see below).

 >  - A queue-count change: netdev_pick_tx() ignores a cached index
 >    >= real_num_tx_queues whatever wmem says.

In mlx5e that comes only with a channel replacement, which the drain
covers, or with an HTB leaf deletion, which closes the leaf SQ before
its queue is removed.

 > Separately, "a drained wmem means that no work referencing the
 > connection state is pending on any SQ" has a pre-existing hole:
 > netem with delay, jitter or rate calls skb_orphan_partial(), which
 > keeps skb->sk but swaps tcp_wfree for sock_efree, releasing the wmem
 > charge while the skb is parked.
[...]
 > Both kinds of skb above can be handled in the driver alone: mlx5e can
 > skip the context WQEs for an skb with skb->destructor != tcp_wfree (the
 > test tcp_gso_segment() already uses), which covers the pre-install
 > plaintext, whose orphan decision netem_enqueue() made before the socket
 > was offloaded, and the parked rekey ciphertext alike.
[...]

Agreed that it belongs in the driver. Rather than keying on the
destructor, I wrote it so that nothing is posted for an skb that
doesn't use the context: the pending params are posted only once the
skb is known to need them (in order, or resynced, where they fold into
the full resync), and stay pending on SKIP_NO_DATA. A pass-through skb
then touches nothing, wherever it was parked. Does that cover both of
your cases?

 > So (2) as proposed is a good stability hint, but not proof. I see
 > two ways to get correctness:
 >
 >  i. Close the holes individually, without a new per-packet lock: the
[...]
 > ii. A driver-side owner check where WQEs are posted: priv_tx
[...]
 > I lean towards (ii) for robustness, with (2) as the hint that keeps
 > it off the slow path, but I'd rather hear your view on the hot-path
 > cost first.

I'd prefer (i). With the drain, it no longer needs to close channels
first or to count connections, and the rest is small and local. (ii)
adds a lock and a SW encryption fallback to the mlx5e xmit path, for a
case that shouldn't happen once the holes are closed; a fallback that
hardly ever runs is also hard to keep correct.

And as these moves corrupt records today, without rekey, I'd send (2)
and the mlx5e queue pin to net as fixes. On ConnectX-6 Dx with many
channels, kTLS TX records already fail decryption at the receiver with
the default txq_reselection_ms, and forcing moves breaks most
connections; with the net fixes, the same runs are clean.

As for a future path silently breaking the invariant, the driver can
catch that cheaply: it remembers the SQ that a connection last posted
on, and when the next skb arrives on another SQ while ooo_okay is clear
and the old SQ is still live, it bumps a counter and warns once. That
is a pointer compare per skb, and it turns a silent corruption into a
visible one.

 > (3a) fits, and I prefer it. sendmsg keeps sending SW ciphertext during
 > rekey, which holds wmem, so (3b) can wait indefinitely unless the
 > sender stalls. With this series, mlx5e is the only in-tree driver
 > accepting TLS 1.3 crypto_info, so the core can require the new op for
 > TX rekey and leave other devices in SW.
 >
 > Proposed contract:
 >
 >   int (*tls_dev_rekey)(struct net_device *netdev, struct sock *sk,
 >                        enum tls_offload_ctx_dir direction,
 >                        struct tls_crypto_info *crypto_info,
 >                        u32 start_offload_tcp_sn);

Looks good to me.

 >  - The old DEK is freed from process context once a successful
 >    CQE covers the fenced replacement, subject to your answer to the
 >    second question above.

Agreed, and it's simpler than I first thought: the driver doesn't need
to track completions per connection. When it posts the fenced
replacement, it can hand the old DEK to that WQE's descriptor, the way
DUMP WQEs hold their page refs today, and release it once the SQ
retires that WQE. Completions being in order, that covers every old-key
WQE posted before it on the SQ. A flush or an SQ close retires it too;
the TIS may then still name the old DEK until the resync installs the
new one, which is your second question again, and the same as an idle
TIS in the pool today. The release runs from NAPI, so the destroy
itself would be deferred to a work item.

 >  - Retirement of the SQ only proves the old work stopped, not that
 >    the replacement took effect. If the SQ errors or closes before
 >    a successful CQE covers the replacement, the driver forces a
 >    resync on the next offloaded skb regardless of expected_seq:
[...]

That's the resync fix above, which isn't specific to rekey.

One question on the core side: once the HW validator is restored, can
a decrypted old-key skb still be queued, e.g. a spurious retransmit
parked in a qdisc, and reach the driver? It would end before the new
marker, so mlx5e would take SKIP_NO_DATA with a decrypted skb, which
today hits WARN_ON_ONCE() and drops it.

 > If this direction works for you, I'll write mlx5e_ktls_rekey_tx()
 > and whichever (2) option you prefer on top of your fix, and test
 > them on the same hardware.

Sounds good. (2) is covered by the net fixes, so please go ahead with
the op and mlx5e_ktls_rekey_tx() on top of them, and I'll review. The
RFCs mentioned above should let you start before the fixes land.

Thanks,
Tariq