Re: [dpdk-dev] [PATCH] net/ixgbe: fix Tx check descriptor status APIs error

["Zhang, Qi Z" <qi.z.zhang@intel.com>]

Hi Wei:

> -----Original Message-----
> From: Zhao1, Wei
> Sent: Friday, June 22, 2018 4:39 PM
> To: dev@dpdk.org
> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Zhang, Qi Z
> <qi.z.zhang@intel.com>; stable@dpdk.org; Zhao1, Wei <wei.zhao1@intel.com>
> Subject: [PATCH] net/ixgbe: fix Tx check descriptor status APIs error
> 
> This is a issue involve RS bit set rule in ixgbe.
> Let us take function ixgbe_xmit_pkts_vec () as an example, in this function RS
> bit will be set for descriptor with index
> txq->tx_next_rs, and also descriptor free function
> ixgbe_tx_free_bufs() also check RS bit for descriptor with index
> txq->tx_next_rs, This is perfect ok. Let us take an example,
> if app set tx_rs_thresh = 32 and nb_desc = 512, then ixgbe PMD code will init
> txq->tx_next_rs = 31 in function ixgbe_reset_tx_queue when tx queue setup.
> And also txq->tx_next_rs will be update as 63, 95 and so on. But, in the
> function ixgbe_dev_tx_descriptor_status(), the RS bit to check is " desc = ((desc
> + txq->tx_rs_thresh - 1) /
> txq->tx_rs_thresh) * txq-tx_rs_thresh", which is 32 ,64, 96 and so on.
> So, they are all wrong! In tx function of ixgbe_xmit_pkts_simple, the RS bit rule
> is also the same, it also set index 31 ,64, 95.
> we need to correct it.

One question:
does only the descriptor with RS bit will have DD status, or NIC will always update all descriptor's DD status but this happens when the next descriptor with RS bit has been sent?
If it is the first case, I think you fix still have problem, because multi-seg mbuf or tso offload will break the 31, 63, 95 pattern
See:
						nb_used = (uint16_t)(tx_pkt->nb_segs + new_ctx);

						if (txp != NULL &&
                                nb_used + txq->nb_tx_used >= txq->tx_rs_thresh)
                        /* set RS on the previous packet in the burst */
                        txp->read.cmd_type_len |=
                                rte_cpu_to_le_32(IXGBE_TXD_CMD_RS);

so the possible solution is store each RS position in a list at tx, and find the next RS from the list in ixgbe_dev_tx_descriptor_status  

If it is the second case, it will be simple we don't need to check forward with tx_rs_thresh, just check the exact position ( I hope it is this case :))

Regards
Qi
> 
> Fixes: a2919e13d95e ("net/ixgbe: implement descriptor status API")
> 
> Signed-off-by: Wei Zhao <wei.zhao1@intel.com>
> ---
>  drivers/net/ixgbe/ixgbe_rxtx.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/ixgbe/ixgbe_rxtx.c b/drivers/net/ixgbe/ixgbe_rxtx.c
> index 3e13d26..f185219 100644
> --- a/drivers/net/ixgbe/ixgbe_rxtx.c
> +++ b/drivers/net/ixgbe/ixgbe_rxtx.c
> @@ -3146,15 +3146,15 @@ ixgbe_dev_tx_descriptor_status(void *tx_queue,
> uint16_t offset)
>  		return -EINVAL;
> 
>  	desc = txq->tx_tail + offset;
> +	if (desc >= txq->nb_tx_desc)
> +		desc -= txq->nb_tx_desc;
>  	/* go to next desc that has the RS bit */
> -	desc = ((desc + txq->tx_rs_thresh - 1) / txq->tx_rs_thresh) *
> -		txq->tx_rs_thresh;
> -	if (desc >= txq->nb_tx_desc) {
> +	desc = (desc  / txq->tx_rs_thresh + 1) *
> +			txq->tx_rs_thresh - 1;
> +	if (desc >= txq->nb_tx_desc)
>  		desc -= txq->nb_tx_desc;
> -		if (desc >= txq->nb_tx_desc)
> -			desc -= txq->nb_tx_desc;
> -	}
> 
> +	desc = txq->sw_ring[desc].last_id;
>  	status = &txq->tx_ring[desc].wb.status;
>  	if (*status & rte_cpu_to_le_32(IXGBE_ADVTXD_STAT_DD))
>  		return RTE_ETH_TX_DESC_DONE;
> --
> 2.7.5