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

Zhang, Qi Z qi.z.zhang at intel.com
Mon Jun 25 04:48:35 CEST 2018



> -----Original Message-----
> From: Zhao1, Wei
> Sent: Monday, June 25, 2018 9:58 AM
> To: Zhang, Qi Z <qi.z.zhang at intel.com>; dev at dpdk.org
> Cc: Lu, Wenzhuo <wenzhuo.lu at intel.com>; stable at dpdk.org
> Subject: RE: [PATCH] net/ixgbe: fix Tx check descriptor status APIs error
> 
> Hi,
> 
> > -----Original Message-----
> > From: Zhang, Qi Z
> > Sent: Friday, June 22, 2018 9:47 PM
> > To: Zhao1, Wei <wei.zhao1 at intel.com>; dev at dpdk.org
> > Cc: Lu, Wenzhuo <wenzhuo.lu at intel.com>; stable at dpdk.org
> > Subject: RE: [PATCH] net/ixgbe: fix Tx check descriptor status APIs
> > error
> >
> > Hi Wei:
> >
> > > -----Original Message-----
> > > From: Zhao1, Wei
> > > Sent: Friday, June 22, 2018 4:39 PM
> > > To: dev at dpdk.org
> > > Cc: Lu, Wenzhuo <wenzhuo.lu at intel.com>; Zhang, Qi Z
> > > <qi.z.zhang at intel.com>; stable at dpdk.org; Zhao1, Wei
> > > <wei.zhao1 at 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 :))
> 
> In this patch, code "desc = txq->sw_ring[desc].last_id;" will get the last index
> for several segments packet, that  solve the case when packet contain more
> than one segment.

Yes, but my understanding is we "set RS on the previous packet" but not the packet cross tx_rs_thresh boundary 
So even without multi-seg , it will be 30, 62, 94, but not 31, 63, 95, probably the reason we didn't see the issue, is because if we test
it with 32 burst, the latest packet still will be set RS, so it will be 30,31, 62,63, 94, 95,
but if we tested with 64 burst, I assume it will be 30, 62, 63, 94 ... right?

> 
> >
> > Regards
> > Qi
> > >
> > > Fixes: a2919e13d95e ("net/ixgbe: implement descriptor status API")
> > >
> > > Signed-off-by: Wei Zhao <wei.zhao1 at 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



More information about the dev mailing list