Re: [PATCH 0/2] net/mana: fix transmit burst and add hardware TSO support

[Stephen Hemminger <stephen@networkplumber.org>]

On Mon, 12 Oct 2026 07:36:24 +0000
Tathagat Priyadarshi <tathagat.dpdk@gmail.com> wrote:

> This patch series addresses transmit burst error handling and introduces
> hardware TCP Segmentation Offload (TSO) support for the Microsoft Azure
> MANA PMD.
> 
> Patch 1 fixes the transmit burst loop in mana_tx_burst() to terminate
> with break instead of continue upon packet errors. Under the DPDK
> rte_eth_tx_burst() API contract, callers expect a contiguous prefix of
> transmitted packets. Skipping a packet caused unsent packets to be treated
> as unsent and freed by caller, followed by a second free upon hardware CQE
> arrival.
> 
> Patch 2 adds support for hardware TCP Segmentation Offload (TSO),
> formatting SGL entries with packet headers in SGE0, handling GDMA work
> request flags, advertising RTE_ETH_TX_OFFLOAD_TCP_TSO, and updating
> the mana driver feature table.
> 
> Tathagat Priyadarshi (2):
>   net/mana: fix transmit burst error handling
>   net/mana: add hardware TSO support


Several problems with this patch series.

Review: [PATCH 1/2] net/mana: fix transmit burst error handling
        [PATCH 2/2] net/mana: add hardware TSO support

Series summary
--------------
Patch 1 correctly identifies the bug (continue breaks the prefix
contract and leads to double free) but the fix trades it for a
head-of-line block. Patch 2 carries a stable-worthy bounds fix that
should be in patch 1, and adds more break-on-bad-packet paths.


Patch 1/2
---------

Error: break on a per-packet validation failure stalls the queue.

A packet that exceeds max_send_sge, or whose MR lookup fails, will
fail again on every retry. Applications that loop until all packets
are sent (testpmd with retry, most forwarding apps) spin forever on
it. Short return from tx_burst is for ring full / backpressure only;
a bad packet must be dropped, counted, and consumed so the prefix
contract still holds.

Keep pkt_sent for the doorbell, and return a separate count of
consumed packets:

	uint16_t nb_done = 0;
	...
	if (unlikely(m_pkt->nb_segs > priv->max_send_sge)) {
		DP_LOG(ERR, "send packet segments %d exceeding max",
		       m_pkt->nb_segs);
		goto drop;
	}
	...
	if (seg_idx != m_pkt->nb_segs)
		goto drop;
	...
	if (!ret) {
		...
		pkt_sent++;
		nb_done++;
		...
	} else {
		break;		/* ring full: real backpressure */
	}
	continue;
drop:
	txq->stats.errors++;
	rte_pktmbuf_free(m_pkt);
	nb_done++;
	}
	...
	return nb_done;

Note the RTE_ARCH_32 short doorbell break happens after nb_done is
incremented, so the count stays correct there.

Info: CQE suppression is decided before the next packet is known to
be valid. If the packet after a suppressed WQE fails, no completion
is generated until a later burst posts an unsuppressed WQE; with no
further traffic those mbufs are held indefinitely. This existed
before, but break makes it reachable from any mid-burst failure
rather than only the last packet. With drop-and-continue it is back
to the last-packet case only.


Patch 2/2
---------

Error: same as 1/2. The new "TSO header cannot be fragmented",
"Invalid IP version for TSO" and "failed to build TSO SGL" paths
all break and leave the packet at the head of the burst. They are
permanent per-packet failures; use the drop path.

Warning: the MAX_SGL_ENTRIES_FOR_TRANSMIT check fixes an existing
bug and does not belong in a feature patch. sgl.gdma_sgl[] is sized
MAX_SGL_ENTRIES_FOR_TRANSMIT (30) but the old non-TSO loop writes up
to nb_segs <= max_send_sge entries, and max_send_sge comes from the
device with nothing clamping it to 30. Move that check to patch 1
(or its own fix with Fixes/Cc stable), and clamp what is advertised:

	dev_info->tx_desc_lim.nb_seg_max =
		RTE_MIN(priv->max_send_sge, MAX_SGL_ENTRIES_FOR_TRANSMIT);
	dev_info->tx_desc_lim.nb_mtu_seg_max =
		RTE_MIN(priv->max_send_sge, MAX_SGL_ENTRIES_FOR_TRANSMIT);

Also, a TSO packet with payload in the first segment needs one
extra SGE. An application that honours nb_seg_max will still get
drops; either advertise nb_seg_max - 1 when TSO is enabled or
document the limit in mana.rst.

Warning: RTE_MBUF_F_TX_TCP_SEG implies RTE_MBUF_F_TX_TCP_CKSUM
(rte_mbuf_core.h). When TCP_SEG is set but the packet falls back to
the non-TSO path (tso_segsz == 0 or no payload), the TCP checksum
is only offloaded if the application also set TCP_CKSUM. Make the
fallback honour the implied flag:

	} else if ((m_pkt->ol_flags & RTE_MBUF_F_TX_L4_MASK) ==
			RTE_MBUF_F_TX_TCP_CKSUM ||
		   (m_pkt->ol_flags & RTE_MBUF_F_TX_TCP_SEG)) {

Warning: no release note. New driver features go in
doc/guides/rel_notes/release_26_11.rst under New Features.

Warning: commit message is a bullet list restating the diff (which
macro is defined where, which field is zeroed). Say what is added
and why, and how it was tested: which Azure SKU / MANA firmware,
IPv4 and IPv6, single and multi-segment mbufs, e.g. testpmd
"csum set tcp hw" + "tso set <mss>" with iperf through the VM.

Info: the num_sge >= MAX_SGL_ENTRIES_FOR_TRANSMIT check inside the
TSO segment loop cannot trigger; expected_sge was already checked
against the same limit and nb_segs bounds the walk. Drop it.

Info: GDMA_WR_OOB_IN_SGL and GDMA_WR_PAD_BY_SGE0 are hardware work
request flags. Put them next to struct gdma_work_request (the flags
member comment already refers to them) rather than between the
offload capability macros.