Re: [PATCH] net/intel/common: track all RS buckets used by a Tx packet

[Bruce Richardson <bruce.richardson@intel.com>]

On Fri, Nov 06, 2026 at 10:25:14AM +0100, Jerome Tollet wrote:
> A TSO packet can use more descriptors than tx_rs_thresh. The scalar Tx
> path records the completion descriptor only for the bucket containing
> the packet's first descriptor. If the packet fills the next bucket,
> that bucket has no completion entry. Cleanup then stops reclaiming
> descriptors and the Tx ring stalls.
> 
> Record the packet's last descriptor for every bucket it crosses, and
> wrap the bucket index at the end of the ring.
> 
> On E810 PF (ice) and VF (iavf) with 2K buffers and tx_rs_thresh=32,
> 65K GSO NVMe/TCP traffic reproduced the stall. With this change,
> 4K and 128K CRC tests passed with TSO enabled on both.
> 
> A standalone DPDK iavf reproducer, with no VPP, sends a 31-descriptor
> TSO packet followed by a 33-descriptor TSO packet. Both pass
> rte_eth_tx_prepare(). The original PMD leaves rs_last_id[1] pointing
> to descriptor 0 and stalls after 991 further packets. With this fix,
> it records descriptor 63 and transmits all 2048 further packets.
> 
> Fixes: 8f4396b9f63c ("net/intel: use separate array for desc status tracking")
> Cc: stable@dpdk.org
> Signed-off-by: Jerome Tollet <jtollet@cisco.com>
> ---
>  .mailmap                             |  1 +
>  drivers/net/intel/common/tx_scalar.h | 23 +++++++++++++----------
>  2 files changed, 14 insertions(+), 10 deletions(-)
> 

Thanks, fix looks correct to me, and AI agrees. However, I have a few
stylistic comments inline below. If you're ok with it, I can make the
couple of suggested changes on apply. (Otherwise, please send a v2)

/Bruce

> diff --git a/.mailmap b/.mailmap
> index 754dd9e760..ff75ec12a2 100644
> --- a/.mailmap
> +++ b/.mailmap
> @@ -752,6 +752,7 @@ Jerin Jacob <jerinj@marvell.com> <jerin.jacob@caviumnetworks.com>
>  Jerin Jacob <jerinj@marvell.com> <jerinjacobk@gmail.com>
>  Jeroen de Borst <jeroendb@google.com>
>  Jerome Jutteau <jerome.jutteau@outscale.com>
> +Jerome Tollet <jtollet@cisco.com>
>  Jerry Hao OS <jerryhao@os.amperecomputing.com>
>  Jerry Lilijun <jerry.lilijun@huawei.com>
>  Jerry Zhang <jerry.zhang@intel.com>
> diff --git a/drivers/net/intel/common/tx_scalar.h b/drivers/net/intel/common/tx_scalar.h
> index 7368d3abc9..c328f8d56b 100644
> --- a/drivers/net/intel/common/tx_scalar.h
> +++ b/drivers/net/intel/common/tx_scalar.h
> @@ -614,22 +614,25 @@ ci_xmit_pkts(struct ci_tx_queue *txq,
>  end_pkt:
>  		txq->nb_tx_free = (uint16_t)(txq->nb_tx_free - nb_used);
>  
> -		/* Check if packet crosses into a new RS threshold bucket.
> -		 * The RS bit is set on the last descriptor when we move from one bucket to another.
> -		 * For example, with tx_rs_thresh=32 and a 5-descriptor packet using slots 30-34:
> -		 *   - pkt_rs_idx = 30 >> 5 = 0 (started in bucket 0)
> -		 *   - tx_last = 34, so 35 >> 5 = 1 (next packet is in bucket 1)
> -		 *   - Since 0 != 1, set RS bit on descriptor 34, and record rs_last_id[0] = 34
> +		/* A packet may consume more than one RS threshold bucket. The
> +		 * last descriptor reports completion for every bucket crossed by
> +		 * that packet, including buckets with no packet start.
>  		 */

I'm not happy with deleting the old comment here, I think it was useful to
have. Therefore I'd suggest keeping it and moving the new comment down to
be just before the loop which actually does the rs_last_id assignment. That
keeps things more comprehensible for future readers, I feel.

> -		uint16_t next_rs_idx = ((tx_last + 1) >> txq->log2_rs_thresh);
> +		const uint16_t nb_rs_buckets = txq->nb_tx_desc >> txq->log2_rs_thresh;
> +		uint16_t next_rs_idx = (tx_last + 1) >> txq->log2_rs_thresh;
> +		uint16_t rs_idx = pkt_rs_idx;

rs_idx is the loop counter only used there, so I'd suggest moving it to be
defined inside the if to reduce its scope. (To minimise the diff I'd also
tend to keep the braces on the nex_rs_idx assignment, when reviewing I was
scanning it trying to see how the calculation had changed.)

>  
> +		if (next_rs_idx == nb_rs_buckets)
> +			next_rs_idx = 0;
>  		if (next_rs_idx != pkt_rs_idx) {
> -			/* Packet crossed into a new bucket - set RS bit on last descriptor */

Again, no need to remove the comment.

>  			txd->cmd_type_offset_bsz |=
>  					rte_cpu_to_le_64(CI_TX_DESC_CMD_RS << CI_TXD_QW1_CMD_S);
>  
> -			/* Record the last descriptor ID for the bucket we're leaving */
> -			txq->rs_last_id[pkt_rs_idx] = tx_last;
> +			do {
> +				txq->rs_last_id[rs_idx] = tx_last;
> +				if (++rs_idx == nb_rs_buckets)
> +					rs_idx = 0;
> +			} while (rs_idx != next_rs_idx);
>  		}
>  
>  		if (ts_fns != NULL)
> -- 
> 2.56.0
>