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

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

[Wei Zhao]

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.

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

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

[Zhang, Qi Z]

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

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

[Zhao1, Wei]

Hi,

> -----Original Message-----
> From: Zhang, Qi Z
> Sent: Friday, June 22, 2018 9:47 PM
> To: Zhao1, Wei <wei.zhao1@intel.com>; dev@dpdk.org
> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; stable@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@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 :))

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.

> 
> 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

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

[Zhang, Qi Z]

> -----Original Message-----
> From: Zhao1, Wei
> Sent: Monday, June 25, 2018 9:58 AM
> To: Zhang, Qi Z <qi.z.zhang@intel.com>; dev@dpdk.org
> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; stable@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@intel.com>; dev@dpdk.org
> > Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; stable@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@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 :))
> 
> 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@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

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

[Zhao1, Wei]

Hi, qi

> -----Original Message-----
> From: Zhang, Qi Z
> Sent: Monday, June 25, 2018 10:49 AM
> To: Zhao1, Wei <wei.zhao1@intel.com>; dev@dpdk.org
> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; stable@dpdk.org
> Subject: RE: [PATCH] net/ixgbe: fix Tx check descriptor status APIs error
> 
> 
> 
> > -----Original Message-----
> > From: Zhao1, Wei
> > Sent: Monday, June 25, 2018 9:58 AM
> > To: Zhang, Qi Z <qi.z.zhang@intel.com>; dev@dpdk.org
> > Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; stable@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@intel.com>; dev@dpdk.org
> > > Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; stable@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@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
> > > > txq->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?

There are 3 tx packet function in ixgbe PMD:
ixgbe_xmit_pkts_vec AND ixgbe_xmit_pkts_simple set RS bit in index 31 63 and 9, they also do not support multiple segment packet tx.
ixgbe_xmit_pkts() will set RS bit as your description in index 30 62 and 94, it also set to 31 63 95 if we tx use 32 as burst number.
But ixgbe_xmit_cleanup function will check 31 63 and 95 DD bit to free descriptor, so this is a bug in ixgbe_xmit_pkts.
I will commit other patch to fix this problem.

 
> 
> >
> > >
> > > 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

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

[Zhao1, Wei]

Hi，qi

> -----Original Message-----
> From: Zhang, Qi Z
> Sent: Monday, June 25, 2018 10:49 AM
> To: Zhao1, Wei <wei.zhao1@intel.com>; dev@dpdk.org
> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; stable@dpdk.org
> Subject: RE: [PATCH] net/ixgbe: fix Tx check descriptor status APIs error
> 
> 
> 
> > -----Original Message-----
> > From: Zhao1, Wei
> > Sent: Monday, June 25, 2018 9:58 AM
> > To: Zhang, Qi Z <qi.z.zhang@intel.com>; dev@dpdk.org
> > Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; stable@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@intel.com>; dev@dpdk.org
> > > Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; stable@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@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
> > > > txq->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?
> 

Update to last mail.
There are another RS bit set code, which set RS bit on last descriptor of the threshold packet.
So, that is to say ixgbe_xmit_pkts() not only set 30 62 94, but also 31 63 95.
And it also set the last packet of the burst, so we do not need fix this function, it is not bug.
 
		/* Set RS bit only on threshold packets' last descriptor */
		if (txq->nb_tx_used >= txq->tx_rs_thresh) {
			PMD_TX_FREE_LOG(DEBUG,
					"Setting RS bit on TXD id="
					"%4u (port=%d queue=%d)",
					tx_last, txq->port_id, txq->queue_id);

			cmd_type_len |= IXGBE_TXD_CMD_RS;

			/* Update txq RS bit counters */
			txq->nb_tx_used = 0;
			txp = NULL;
		}


> >
> > >
> > > 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

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

[Zhang, Qi Z]

> -----Original Message-----
> From: Zhao1, Wei
> Sent: Monday, June 25, 2018 2:49 PM
> To: Zhang, Qi Z <qi.z.zhang@intel.com>; dev@dpdk.org
> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; stable@dpdk.org
> Subject: RE: [PATCH] net/ixgbe: fix Tx check descriptor status APIs error
> 
> Hi，qi
> 
> > -----Original Message-----
> > From: Zhang, Qi Z
> > Sent: Monday, June 25, 2018 10:49 AM
> > To: Zhao1, Wei <wei.zhao1@intel.com>; dev@dpdk.org
> > Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; stable@dpdk.org
> > Subject: RE: [PATCH] net/ixgbe: fix Tx check descriptor status APIs
> > error
> >
> >
> >
> > > -----Original Message-----
> > > From: Zhao1, Wei
> > > Sent: Monday, June 25, 2018 9:58 AM
> > > To: Zhang, Qi Z <qi.z.zhang@intel.com>; dev@dpdk.org
> > > Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; stable@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@intel.com>; dev@dpdk.org
> > > > Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; stable@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@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
> > > > > txq->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?
> >
> 
> Update to last mail.
> There are another RS bit set code, which set RS bit on last descriptor of the
> threshold packet.
> So, that is to say ixgbe_xmit_pkts() not only set 30 62 94, but also 31 63 95.
> And it also set the last packet of the burst, so we do not need fix this function,
> it is not bug.
> 
> 		/* Set RS bit only on threshold packets' last descriptor */
> 		if (txq->nb_tx_used >= txq->tx_rs_thresh) {
> 			PMD_TX_FREE_LOG(DEBUG,
> 					"Setting RS bit on TXD id="
> 					"%4u (port=%d queue=%d)",
> 					tx_last, txq->port_id, txq->queue_id);
> 
> 			cmd_type_len |= IXGBE_TXD_CMD_RS;
> 
> 			/* Update txq RS bit counters */
> 			txq->nb_tx_used = 0;
> 			txp = NULL;
> 		}

OK, but is this guarantee that slot 31, 63, 95 is always in the packet that cross tx_rx_thresh boundary?
Let's assume every packet has 5 seg, so in every 7 packet the last one will cross the boundary.
so it will be 
0-4, 5-9, 10-14, 15-19, 20-24, 25-29, 30-34,
35-39, 40-44, 45-49, 50-54, 55-59, 60-64, 65-69
Definitely, it is possible that last packet (and the previous before last) does not include 32*n-1
In ixgbe_xmit_cleanup, it use desc_to_clean_to = sw_ring[desc_to_clean_to].last_id + tx_rx_thresh to calculate next packet in boundary, that's no problem
But in ixgbe_dev_tx_descriptor_status, we assume the it is 31,63,95 pattern, that will be problem.



> 
> 
> > >
> > > >
> > > > 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

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

[Zhao1, Wei]

Hi, qi

> -----Original Message-----
> From: Zhang, Qi Z
> Sent: Monday, June 25, 2018 3:47 PM
> To: Zhao1, Wei <wei.zhao1@intel.com>; dev@dpdk.org
> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; stable@dpdk.org
> Subject: RE: [PATCH] net/ixgbe: fix Tx check descriptor status APIs error
> 
> 
> 
> > -----Original Message-----
> > From: Zhao1, Wei
> > Sent: Monday, June 25, 2018 2:49 PM
> > To: Zhang, Qi Z <qi.z.zhang@intel.com>; dev@dpdk.org
> > Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; stable@dpdk.org
> > Subject: RE: [PATCH] net/ixgbe: fix Tx check descriptor status APIs
> > error
> >
> > Hi，qi
> >
> > > -----Original Message-----
> > > From: Zhang, Qi Z
> > > Sent: Monday, June 25, 2018 10:49 AM
> > > To: Zhao1, Wei <wei.zhao1@intel.com>; dev@dpdk.org
> > > Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; stable@dpdk.org
> > > Subject: RE: [PATCH] net/ixgbe: fix Tx check descriptor status APIs
> > > error
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Zhao1, Wei
> > > > Sent: Monday, June 25, 2018 9:58 AM
> > > > To: Zhang, Qi Z <qi.z.zhang@intel.com>; dev@dpdk.org
> > > > Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; stable@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@intel.com>; dev@dpdk.org
> > > > > Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; stable@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@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
> > > > > > txq->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?
> > >
> >
> > Update to last mail.
> > There are another RS bit set code, which set RS bit on last descriptor
> > of the threshold packet.
> > So, that is to say ixgbe_xmit_pkts() not only set 30 62 94, but also 31 63 95.
> > And it also set the last packet of the burst, so we do not need fix
> > this function, it is not bug.
> >
> > 		/* Set RS bit only on threshold packets' last descriptor */
> > 		if (txq->nb_tx_used >= txq->tx_rs_thresh) {
> > 			PMD_TX_FREE_LOG(DEBUG,
> > 					"Setting RS bit on TXD id="
> > 					"%4u (port=%d queue=%d)",
> > 					tx_last, txq->port_id, txq->queue_id);
> >
> > 			cmd_type_len |= IXGBE_TXD_CMD_RS;
> >
> > 			/* Update txq RS bit counters */
> > 			txq->nb_tx_used = 0;
> > 			txp = NULL;
> > 		}
> 
> OK, but is this guarantee that slot 31, 63, 95 is always in the packet that cross
> tx_rx_thresh boundary?
> Let's assume every packet has 5 seg, so in every 7 packet the last one will
> cross the boundary.
> so it will be
> 0-4, 5-9, 10-14, 15-19, 20-24, 25-29, 30-34, 35-39, 40-44, 45-49, 50-54, 55-59,
> 60-64, 65-69 Definitely, it is possible that last packet (and the previous before
> last) does not include 32*n-1 In ixgbe_xmit_cleanup, it use desc_to_clean_to
> = sw_ring[desc_to_clean_to].last_id + tx_rx_thresh to calculate next packet
> in boundary, that's no problem But in ixgbe_dev_tx_descriptor_status, we
> assume the it is 31,63,95 pattern, that will be problem.

In my patch for ixgbe_dev_tx_descriptor_status()
We have code :

	desc = txq->sw_ring[desc].last_id;
 	status = &txq->tx_ring[desc].wb.status;

it will only check last_id DD, if in the several segment case, it will also to check the last DD.

> 
> 
> 
> >
> >
> > > >
> > > > >
> > > > > 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

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

[Zhang, Qi Z]

> -----Original Message-----
> From: Zhao1, Wei
> Sent: Monday, June 25, 2018 3:53 PM
> To: Zhang, Qi Z <qi.z.zhang@intel.com>; dev@dpdk.org
> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; stable@dpdk.org
> Subject: RE: [PATCH] net/ixgbe: fix Tx check descriptor status APIs error
> 
> Hi, qi
> 
> > -----Original Message-----
> > From: Zhang, Qi Z
> > Sent: Monday, June 25, 2018 3:47 PM
> > To: Zhao1, Wei <wei.zhao1@intel.com>; dev@dpdk.org
> > Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; stable@dpdk.org
> > Subject: RE: [PATCH] net/ixgbe: fix Tx check descriptor status APIs
> > error
> >
> >
> >
> > > -----Original Message-----
> > > From: Zhao1, Wei
> > > Sent: Monday, June 25, 2018 2:49 PM
> > > To: Zhang, Qi Z <qi.z.zhang@intel.com>; dev@dpdk.org
> > > Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; stable@dpdk.org
> > > Subject: RE: [PATCH] net/ixgbe: fix Tx check descriptor status APIs
> > > error
> > >
> > > Hi，qi
> > >
> > > > -----Original Message-----
> > > > From: Zhang, Qi Z
> > > > Sent: Monday, June 25, 2018 10:49 AM
> > > > To: Zhao1, Wei <wei.zhao1@intel.com>; dev@dpdk.org
> > > > Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; stable@dpdk.org
> > > > Subject: RE: [PATCH] net/ixgbe: fix Tx check descriptor status
> > > > APIs error
> > > >
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Zhao1, Wei
> > > > > Sent: Monday, June 25, 2018 9:58 AM
> > > > > To: Zhang, Qi Z <qi.z.zhang@intel.com>; dev@dpdk.org
> > > > > Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; stable@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@intel.com>; dev@dpdk.org
> > > > > > Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; stable@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@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
> > > > > > > txq->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
> > > > > > > txq->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?
> > > >
> > >
> > > Update to last mail.
> > > There are another RS bit set code, which set RS bit on last
> > > descriptor of the threshold packet.
> > > So, that is to say ixgbe_xmit_pkts() not only set 30 62 94, but also 31 63 95.
> > > And it also set the last packet of the burst, so we do not need fix
> > > this function, it is not bug.
> > >
> > > 		/* Set RS bit only on threshold packets' last descriptor */
> > > 		if (txq->nb_tx_used >= txq->tx_rs_thresh) {
> > > 			PMD_TX_FREE_LOG(DEBUG,
> > > 					"Setting RS bit on TXD id="
> > > 					"%4u (port=%d queue=%d)",
> > > 					tx_last, txq->port_id, txq->queue_id);
> > >
> > > 			cmd_type_len |= IXGBE_TXD_CMD_RS;
> > >
> > > 			/* Update txq RS bit counters */
> > > 			txq->nb_tx_used = 0;
> > > 			txp = NULL;
> > > 		}
> >
> > OK, but is this guarantee that slot 31, 63, 95 is always in the packet
> > that cross tx_rx_thresh boundary?
> > Let's assume every packet has 5 seg, so in every 7 packet the last one
> > will cross the boundary.
> > so it will be
> > 0-4, 5-9, 10-14, 15-19, 20-24, 25-29, 30-34, 35-39, 40-44, 45-49,
> > 50-54, 55-59, 60-64, 65-69 Definitely, it is possible that last packet
> > (and the previous before
> > last) does not include 32*n-1 In ixgbe_xmit_cleanup, it use
> > desc_to_clean_to = sw_ring[desc_to_clean_to].last_id + tx_rx_thresh to
> > calculate next packet in boundary, that's no problem But in
> > ixgbe_dev_tx_descriptor_status, we assume the it is 31,63,95 pattern, that
> will be problem.
> 
> In my patch for ixgbe_dev_tx_descriptor_status() We have code :
> 
> 	desc = txq->sw_ring[desc].last_id;
>  	status = &txq->tx_ring[desc].wb.status;
> 
> it will only check last_id DD, if in the several segment case, it will also to check
> the last DD.

Yes, but what I mean is we can't guarantee a packet cover 31, 63, 95 is the packet that cross tx_rs_thresh boundary,(whose last segment has RS).
> 
> >
> >
> >
> > >
> > >
> > > > >
> > > > > >
> > > > > > 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

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

[Zhao1, Wei]

> -----Original Message-----
> From: Zhang, Qi Z
> Sent: Monday, June 25, 2018 3:56 PM
> To: Zhao1, Wei <wei.zhao1@intel.com>; dev@dpdk.org
> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; stable@dpdk.org
> Subject: RE: [PATCH] net/ixgbe: fix Tx check descriptor status APIs error
> 
> 
> 
> > -----Original Message-----
> > From: Zhao1, Wei
> > Sent: Monday, June 25, 2018 3:53 PM
> > To: Zhang, Qi Z <qi.z.zhang@intel.com>; dev@dpdk.org
> > Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; stable@dpdk.org
> > Subject: RE: [PATCH] net/ixgbe: fix Tx check descriptor status APIs
> > error
> >
> > Hi, qi
> >
> > > -----Original Message-----
> > > From: Zhang, Qi Z
> > > Sent: Monday, June 25, 2018 3:47 PM
> > > To: Zhao1, Wei <wei.zhao1@intel.com>; dev@dpdk.org
> > > Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; stable@dpdk.org
> > > Subject: RE: [PATCH] net/ixgbe: fix Tx check descriptor status APIs
> > > error
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Zhao1, Wei
> > > > Sent: Monday, June 25, 2018 2:49 PM
> > > > To: Zhang, Qi Z <qi.z.zhang@intel.com>; dev@dpdk.org
> > > > Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; stable@dpdk.org
> > > > Subject: RE: [PATCH] net/ixgbe: fix Tx check descriptor status
> > > > APIs error
> > > >
> > > > Hi，qi
> > > >
> > > > > -----Original Message-----
> > > > > From: Zhang, Qi Z
> > > > > Sent: Monday, June 25, 2018 10:49 AM
> > > > > To: Zhao1, Wei <wei.zhao1@intel.com>; dev@dpdk.org
> > > > > Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; stable@dpdk.org
> > > > > Subject: RE: [PATCH] net/ixgbe: fix Tx check descriptor status
> > > > > APIs error
> > > > >
> > > > >
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Zhao1, Wei
> > > > > > Sent: Monday, June 25, 2018 9:58 AM
> > > > > > To: Zhang, Qi Z <qi.z.zhang@intel.com>; dev@dpdk.org
> > > > > > Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; stable@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@intel.com>; dev@dpdk.org
> > > > > > > Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; stable@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@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
> > > > > > > > txq->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
> > > > > > > > txq->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,
> > > > > > > > txq->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?
> > > > >
> > > >
> > > > Update to last mail.
> > > > There are another RS bit set code, which set RS bit on last
> > > > descriptor of the threshold packet.
> > > > So, that is to say ixgbe_xmit_pkts() not only set 30 62 94, but also 31 63
> 95.
> > > > And it also set the last packet of the burst, so we do not need
> > > > fix this function, it is not bug.
> > > >
> > > > 		/* Set RS bit only on threshold packets' last descriptor */
> > > > 		if (txq->nb_tx_used >= txq->tx_rs_thresh) {
> > > > 			PMD_TX_FREE_LOG(DEBUG,
> > > > 					"Setting RS bit on TXD id="
> > > > 					"%4u (port=%d queue=%d)",
> > > > 					tx_last, txq->port_id, txq->queue_id);
> > > >
> > > > 			cmd_type_len |= IXGBE_TXD_CMD_RS;
> > > >
> > > > 			/* Update txq RS bit counters */
> > > > 			txq->nb_tx_used = 0;
> > > > 			txp = NULL;
> > > > 		}
> > >
> > > OK, but is this guarantee that slot 31, 63, 95 is always in the
> > > packet that cross tx_rx_thresh boundary?
> > > Let's assume every packet has 5 seg, so in every 7 packet the last
> > > one will cross the boundary.
> > > so it will be
> > > 0-4, 5-9, 10-14, 15-19, 20-24, 25-29, 30-34, 35-39, 40-44, 45-49,
> > > 50-54, 55-59, 60-64, 65-69 Definitely, it is possible that last
> > > packet (and the previous before
> > > last) does not include 32*n-1 In ixgbe_xmit_cleanup, it use
> > > desc_to_clean_to = sw_ring[desc_to_clean_to].last_id + tx_rx_thresh
> > > to calculate next packet in boundary, that's no problem But in
> > > ixgbe_dev_tx_descriptor_status, we assume the it is 31,63,95
> > > pattern, that
> > will be problem.
> >
> > In my patch for ixgbe_dev_tx_descriptor_status() We have code :
> >
> > 	desc = txq->sw_ring[desc].last_id;
> >  	status = &txq->tx_ring[desc].wb.status;
> >
> > it will only check last_id DD, if in the several segment case, it will
> > also to check the last DD.
> 
> Yes, but what I mean is we can't guarantee a packet cover 31, 63, 95 is the
> packet that cross tx_rs_thresh boundary,(whose last segment has RS).

Ok， I  will commit v2 for that.

> >
> > >
> > >
> > >
> > > >
> > > >
> > > > > >
> > > > > > >
> > > > > > > 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

[dpdk-dev] [RFC] net/ixgbe: fix Tx descriptor status api

[Olivier Matz]

The Tx descriptor status api was not behaving as expected. This API is
used to inspect the content of the descriptors in the Tx ring to
determine the length of the Tx queue.

Since the software advances the tail pointer and the hardware advances
the head pointer, the Tx queue is located before txq->tx_tail in the
ring. Therefore, a call to rte_eth_tx_descriptor_status(..., offset=20)
should inspect the 20th descriptor before the tail, not after.

As before, we still need to take care about only checking descriptors
that have the RS bit.

Additionally, we can avoid an access to the ring if offset is greater or
equal to nb_tx_desc - nb_tx_free.

Fixes: a2919e13d95e ("net/ixgbe: implement descriptor status API")
Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
Signed-off-by: Didier Pallard <didier.pallard@6wind.com>
---

Hi Wei, Hi Qi,

We also recently found some issues in Tx descriptor status API for ixgbe,
i40, e1000, igb. I'm preparing a clean patchset for all of them.

Here is already the patch for ixgbe, please let me know what you think.

The API comment of rte_eth_tx_descriptor_status() is incorrect and should
be fixed too. The reference descriptor (when offset = 0) is not where the
next packet will be sent, but where the latest packet has been enqueued.

Regards,
Olivier



 drivers/net/ixgbe/ixgbe_rxtx.c | 45 +++++++++++++++++++++++++++++++-----------
 drivers/net/ixgbe/ixgbe_rxtx.h |  1 +
 2 files changed, 34 insertions(+), 12 deletions(-)

diff --git a/drivers/net/ixgbe/ixgbe_rxtx.c b/drivers/net/ixgbe/ixgbe_rxtx.c
index 3e13d26ae..384587cc6 100644
--- a/drivers/net/ixgbe/ixgbe_rxtx.c
+++ b/drivers/net/ixgbe/ixgbe_rxtx.c
@@ -2606,10 +2606,15 @@ ixgbe_dev_tx_queue_setup(struct rte_eth_dev *dev,
 	    hw->mac.type == ixgbe_mac_X540_vf ||
 	    hw->mac.type == ixgbe_mac_X550_vf ||
 	    hw->mac.type == ixgbe_mac_X550EM_x_vf ||
-	    hw->mac.type == ixgbe_mac_X550EM_a_vf)
+	    hw->mac.type == ixgbe_mac_X550EM_a_vf) {
 		txq->tdt_reg_addr = IXGBE_PCI_REG_ADDR(hw, IXGBE_VFTDT(queue_idx));
-	else
+		txq->tdh_reg_addr = IXGBE_PCI_REG_ADDR(hw,
+				IXGBE_VFTDH(queue_idx));
+	} else {
 		txq->tdt_reg_addr = IXGBE_PCI_REG_ADDR(hw, IXGBE_TDT(txq->reg_idx));
+		txq->tdh_reg_addr = IXGBE_PCI_REG_ADDR(hw,
+				IXGBE_TDH(txq->reg_idx));
+	}
 
 	txq->tx_ring_phys_addr = tz->iova;
 	txq->tx_ring = (union ixgbe_adv_tx_desc *) tz->addr;
@@ -3140,22 +3145,38 @@ ixgbe_dev_tx_descriptor_status(void *tx_queue, uint16_t offset)
 {
 	struct ixgbe_tx_queue *txq = tx_queue;
 	volatile uint32_t *status;
-	uint32_t desc;
+	int32_t desc, dd;
 
 	if (unlikely(offset >= txq->nb_tx_desc))
 		return -EINVAL;
+	if (offset >= txq->nb_tx_desc - txq->nb_tx_free)
+		return RTE_ETH_TX_DESC_DONE;
+
+	desc = txq->tx_tail - offset - 1;
+	if (desc < 0)
+		desc += txq->nb_tx_desc;
 
-	desc = txq->tx_tail + offset;
-	/* 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 -= txq->nb_tx_desc;
-		if (desc >= txq->nb_tx_desc)
-			desc -= txq->nb_tx_desc;
+	/* offset is too small, no other way than reading PCI reg */
+	if (unlikely(offset < txq->tx_rs_thresh)) {
+		int16_t tx_head, queue_size;
+		tx_head = ixgbe_read_addr(txq->tdh_reg_addr);
+		queue_size = txq->tx_tail - tx_head;
+		if (queue_size < 0)
+			queue_size += txq->nb_tx_desc;
+		return queue_size > offset ? RTE_ETH_TX_DESC_FULL :
+			RTE_ETH_TX_DESC_DONE;
 	}
 
-	status = &txq->tx_ring[desc].wb.status;
+	/* index of the dd bit to look at */
+	dd = (desc / txq->tx_rs_thresh + 1) * txq->tx_rs_thresh - 1;
+
+	/* In full featured mode, RS bit is only set in the last descriptor */
+	/* of a multisegments packet */
+	if (!((txq->offloads == 0) &&
+	      (txq->tx_rs_thresh >= RTE_PMD_IXGBE_TX_MAX_BURST)))
+		dd = txq->sw_ring[dd].last_id;
+
+	status = &txq->tx_ring[dd].wb.status;
 	if (*status & rte_cpu_to_le_32(IXGBE_ADVTXD_STAT_DD))
 		return RTE_ETH_TX_DESC_DONE;
 
diff --git a/drivers/net/ixgbe/ixgbe_rxtx.h b/drivers/net/ixgbe/ixgbe_rxtx.h
index 39378f754..384f6324d 100644
--- a/drivers/net/ixgbe/ixgbe_rxtx.h
+++ b/drivers/net/ixgbe/ixgbe_rxtx.h
@@ -201,6 +201,7 @@ struct ixgbe_tx_queue {
 		struct ixgbe_tx_entry_v *sw_ring_v; /**< address of SW ring for vector PMD */
 	};
 	volatile uint32_t   *tdt_reg_addr; /**< Address of TDT register. */
+	volatile uint32_t   *tdh_reg_addr; /**< Address of TDH register. */
 	uint16_t            nb_tx_desc;    /**< number of TX descriptors. */
 	uint16_t            tx_tail;       /**< current value of TDT reg. */
 	/**< Start freeing TX buffers if there are less free descriptors than
-- 
2.11.0

Re: [dpdk-dev] [RFC] net/ixgbe: fix Tx descriptor status api

[Olivier Matz]

Removing wrong mails from the To and Cc. Sorry.

On Mon, Jun 25, 2018 at 04:20:57PM +0200, Olivier Matz wrote:
> The Tx descriptor status api was not behaving as expected. This API is
> used to inspect the content of the descriptors in the Tx ring to
> determine the length of the Tx queue.
> 
> Since the software advances the tail pointer and the hardware advances
> the head pointer, the Tx queue is located before txq->tx_tail in the
> ring. Therefore, a call to rte_eth_tx_descriptor_status(..., offset=20)
> should inspect the 20th descriptor before the tail, not after.
> 
> As before, we still need to take care about only checking descriptors
> that have the RS bit.
> 
> Additionally, we can avoid an access to the ring if offset is greater or
> equal to nb_tx_desc - nb_tx_free.
> 
> Fixes: a2919e13d95e ("net/ixgbe: implement descriptor status API")
> Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
> Signed-off-by: Didier Pallard <didier.pallard@6wind.com>
> ---
> 
> Hi Wei, Hi Qi,
> 
> We also recently found some issues in Tx descriptor status API for ixgbe,
> i40, e1000, igb. I'm preparing a clean patchset for all of them.
> 
> Here is already the patch for ixgbe, please let me know what you think.
> 
> The API comment of rte_eth_tx_descriptor_status() is incorrect and should
> be fixed too. The reference descriptor (when offset = 0) is not where the
> next packet will be sent, but where the latest packet has been enqueued.
> 
> Regards,
> Olivier
> 
> 
> 
>  drivers/net/ixgbe/ixgbe_rxtx.c | 45 +++++++++++++++++++++++++++++++-----------
>  drivers/net/ixgbe/ixgbe_rxtx.h |  1 +
>  2 files changed, 34 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/net/ixgbe/ixgbe_rxtx.c b/drivers/net/ixgbe/ixgbe_rxtx.c
> index 3e13d26ae..384587cc6 100644
> --- a/drivers/net/ixgbe/ixgbe_rxtx.c
> +++ b/drivers/net/ixgbe/ixgbe_rxtx.c
> @@ -2606,10 +2606,15 @@ ixgbe_dev_tx_queue_setup(struct rte_eth_dev *dev,
>  	    hw->mac.type == ixgbe_mac_X540_vf ||
>  	    hw->mac.type == ixgbe_mac_X550_vf ||
>  	    hw->mac.type == ixgbe_mac_X550EM_x_vf ||
> -	    hw->mac.type == ixgbe_mac_X550EM_a_vf)
> +	    hw->mac.type == ixgbe_mac_X550EM_a_vf) {
>  		txq->tdt_reg_addr = IXGBE_PCI_REG_ADDR(hw, IXGBE_VFTDT(queue_idx));
> -	else
> +		txq->tdh_reg_addr = IXGBE_PCI_REG_ADDR(hw,
> +				IXGBE_VFTDH(queue_idx));
> +	} else {
>  		txq->tdt_reg_addr = IXGBE_PCI_REG_ADDR(hw, IXGBE_TDT(txq->reg_idx));
> +		txq->tdh_reg_addr = IXGBE_PCI_REG_ADDR(hw,
> +				IXGBE_TDH(txq->reg_idx));
> +	}
>  
>  	txq->tx_ring_phys_addr = tz->iova;
>  	txq->tx_ring = (union ixgbe_adv_tx_desc *) tz->addr;
> @@ -3140,22 +3145,38 @@ ixgbe_dev_tx_descriptor_status(void *tx_queue, uint16_t offset)
>  {
>  	struct ixgbe_tx_queue *txq = tx_queue;
>  	volatile uint32_t *status;
> -	uint32_t desc;
> +	int32_t desc, dd;
>  
>  	if (unlikely(offset >= txq->nb_tx_desc))
>  		return -EINVAL;
> +	if (offset >= txq->nb_tx_desc - txq->nb_tx_free)
> +		return RTE_ETH_TX_DESC_DONE;
> +
> +	desc = txq->tx_tail - offset - 1;
> +	if (desc < 0)
> +		desc += txq->nb_tx_desc;
>  
> -	desc = txq->tx_tail + offset;
> -	/* 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 -= txq->nb_tx_desc;
> -		if (desc >= txq->nb_tx_desc)
> -			desc -= txq->nb_tx_desc;
> +	/* offset is too small, no other way than reading PCI reg */
> +	if (unlikely(offset < txq->tx_rs_thresh)) {
> +		int16_t tx_head, queue_size;
> +		tx_head = ixgbe_read_addr(txq->tdh_reg_addr);
> +		queue_size = txq->tx_tail - tx_head;
> +		if (queue_size < 0)
> +			queue_size += txq->nb_tx_desc;
> +		return queue_size > offset ? RTE_ETH_TX_DESC_FULL :
> +			RTE_ETH_TX_DESC_DONE;
>  	}
>  
> -	status = &txq->tx_ring[desc].wb.status;
> +	/* index of the dd bit to look at */
> +	dd = (desc / txq->tx_rs_thresh + 1) * txq->tx_rs_thresh - 1;
> +
> +	/* In full featured mode, RS bit is only set in the last descriptor */
> +	/* of a multisegments packet */
> +	if (!((txq->offloads == 0) &&
> +	      (txq->tx_rs_thresh >= RTE_PMD_IXGBE_TX_MAX_BURST)))
> +		dd = txq->sw_ring[dd].last_id;
> +
> +	status = &txq->tx_ring[dd].wb.status;
>  	if (*status & rte_cpu_to_le_32(IXGBE_ADVTXD_STAT_DD))
>  		return RTE_ETH_TX_DESC_DONE;
>  
> diff --git a/drivers/net/ixgbe/ixgbe_rxtx.h b/drivers/net/ixgbe/ixgbe_rxtx.h
> index 39378f754..384f6324d 100644
> --- a/drivers/net/ixgbe/ixgbe_rxtx.h
> +++ b/drivers/net/ixgbe/ixgbe_rxtx.h
> @@ -201,6 +201,7 @@ struct ixgbe_tx_queue {
>  		struct ixgbe_tx_entry_v *sw_ring_v; /**< address of SW ring for vector PMD */
>  	};
>  	volatile uint32_t   *tdt_reg_addr; /**< Address of TDT register. */
> +	volatile uint32_t   *tdh_reg_addr; /**< Address of TDH register. */
>  	uint16_t            nb_tx_desc;    /**< number of TX descriptors. */
>  	uint16_t            tx_tail;       /**< current value of TDT reg. */
>  	/**< Start freeing TX buffers if there are less free descriptors than
> -- 
> 2.11.0
> 

Re: [dpdk-dev] [RFC] net/ixgbe: fix Tx descriptor status api

[Zhao1, Wei]

Hi,  Olivier Matz

 Will you commit fix patch for i40e and ixgbe and em?
And the code " dd = (desc / txq->tx_rs_thresh + 1) * txq->tx_rs_thresh - 1;"
Is only proper for tx function ixgbe_xmit_pkts_simple  and ixgbe_xmit_pkts_vec ().
But not proper for ixgbe_xmit_pkts (), the RS bit set rule is different from all these two. 


> -----Original Message-----
> From: Olivier Matz [mailto:olivier.matz@6wind.com]
> Sent: Monday, June 25, 2018 10:21 PM
> To: Zhao1@6wind.com; Zhao1, Wei <wei.zhao1@intel.com>;
> Zhang@6wind.com; Zhang, Qi Z <qi.z.zhang@intel.com>
> Cc: dev@dpdk.org; Lu@6wind.com; Lu, Wenzhuo <wenzhuo.lu@intel.com>
> Subject: [RFC] net/ixgbe: fix Tx descriptor status api
> 
> The Tx descriptor status api was not behaving as expected. This API is used to
> inspect the content of the descriptors in the Tx ring to determine the length
> of the Tx queue.
> 
> Since the software advances the tail pointer and the hardware advances the
> head pointer, the Tx queue is located before txq->tx_tail in the ring.
> Therefore, a call to rte_eth_tx_descriptor_status(..., offset=20) should
> inspect the 20th descriptor before the tail, not after.
> 
> As before, we still need to take care about only checking descriptors that
> have the RS bit.
> 
> Additionally, we can avoid an access to the ring if offset is greater or equal to
> nb_tx_desc - nb_tx_free.
> 
> Fixes: a2919e13d95e ("net/ixgbe: implement descriptor status API")
> Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
> Signed-off-by: Didier Pallard <didier.pallard@6wind.com>
> ---
> 
> Hi Wei, Hi Qi,
> 
> We also recently found some issues in Tx descriptor status API for ixgbe, i40,
> e1000, igb. I'm preparing a clean patchset for all of them.
> 
> Here is already the patch for ixgbe, please let me know what you think.
> 
> The API comment of rte_eth_tx_descriptor_status() is incorrect and should
> be fixed too. The reference descriptor (when offset = 0) is not where the
> next packet will be sent, but where the latest packet has been enqueued.
> 
> Regards,
> Olivier
> 
> 
> 
>  drivers/net/ixgbe/ixgbe_rxtx.c | 45
> +++++++++++++++++++++++++++++++-----------
>  drivers/net/ixgbe/ixgbe_rxtx.h |  1 +
>  2 files changed, 34 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/net/ixgbe/ixgbe_rxtx.c b/drivers/net/ixgbe/ixgbe_rxtx.c
> index 3e13d26ae..384587cc6 100644
> --- a/drivers/net/ixgbe/ixgbe_rxtx.c
> +++ b/drivers/net/ixgbe/ixgbe_rxtx.c
> @@ -2606,10 +2606,15 @@ ixgbe_dev_tx_queue_setup(struct rte_eth_dev
> *dev,
>  	    hw->mac.type == ixgbe_mac_X540_vf ||
>  	    hw->mac.type == ixgbe_mac_X550_vf ||
>  	    hw->mac.type == ixgbe_mac_X550EM_x_vf ||
> -	    hw->mac.type == ixgbe_mac_X550EM_a_vf)
> +	    hw->mac.type == ixgbe_mac_X550EM_a_vf) {
>  		txq->tdt_reg_addr = IXGBE_PCI_REG_ADDR(hw,
> IXGBE_VFTDT(queue_idx));
> -	else
> +		txq->tdh_reg_addr = IXGBE_PCI_REG_ADDR(hw,
> +				IXGBE_VFTDH(queue_idx));
> +	} else {
>  		txq->tdt_reg_addr = IXGBE_PCI_REG_ADDR(hw,
> IXGBE_TDT(txq->reg_idx));
> +		txq->tdh_reg_addr = IXGBE_PCI_REG_ADDR(hw,
> +				IXGBE_TDH(txq->reg_idx));
> +	}
> 
>  	txq->tx_ring_phys_addr = tz->iova;
>  	txq->tx_ring = (union ixgbe_adv_tx_desc *) tz->addr; @@ -3140,22
> +3145,38 @@ ixgbe_dev_tx_descriptor_status(void *tx_queue, uint16_t
> offset)  {
>  	struct ixgbe_tx_queue *txq = tx_queue;
>  	volatile uint32_t *status;
> -	uint32_t desc;
> +	int32_t desc, dd;
> 
>  	if (unlikely(offset >= txq->nb_tx_desc))
>  		return -EINVAL;
> +	if (offset >= txq->nb_tx_desc - txq->nb_tx_free)
> +		return RTE_ETH_TX_DESC_DONE;
> +
> +	desc = txq->tx_tail - offset - 1;
> +	if (desc < 0)
> +		desc += txq->nb_tx_desc;
> 
> -	desc = txq->tx_tail + offset;
> -	/* 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 -= txq->nb_tx_desc;
> -		if (desc >= txq->nb_tx_desc)
> -			desc -= txq->nb_tx_desc;
> +	/* offset is too small, no other way than reading PCI reg */
> +	if (unlikely(offset < txq->tx_rs_thresh)) {
> +		int16_t tx_head, queue_size;
> +		tx_head = ixgbe_read_addr(txq->tdh_reg_addr);
> +		queue_size = txq->tx_tail - tx_head;
> +		if (queue_size < 0)
> +			queue_size += txq->nb_tx_desc;
> +		return queue_size > offset ? RTE_ETH_TX_DESC_FULL :
> +			RTE_ETH_TX_DESC_DONE;
>  	}
> 
> -	status = &txq->tx_ring[desc].wb.status;
> +	/* index of the dd bit to look at */
> +	dd = (desc / txq->tx_rs_thresh + 1) * txq->tx_rs_thresh - 1;
> +
> +	/* In full featured mode, RS bit is only set in the last descriptor */
> +	/* of a multisegments packet */
> +	if (!((txq->offloads == 0) &&
> +	      (txq->tx_rs_thresh >= RTE_PMD_IXGBE_TX_MAX_BURST)))
> +		dd = txq->sw_ring[dd].last_id;
> +
> +	status = &txq->tx_ring[dd].wb.status;
>  	if (*status & rte_cpu_to_le_32(IXGBE_ADVTXD_STAT_DD))
>  		return RTE_ETH_TX_DESC_DONE;
> 
> diff --git a/drivers/net/ixgbe/ixgbe_rxtx.h b/drivers/net/ixgbe/ixgbe_rxtx.h
> index 39378f754..384f6324d 100644
> --- a/drivers/net/ixgbe/ixgbe_rxtx.h
> +++ b/drivers/net/ixgbe/ixgbe_rxtx.h
> @@ -201,6 +201,7 @@ struct ixgbe_tx_queue {
>  		struct ixgbe_tx_entry_v *sw_ring_v; /**< address of SW ring
> for vector PMD */
>  	};
>  	volatile uint32_t   *tdt_reg_addr; /**< Address of TDT register. */
> +	volatile uint32_t   *tdh_reg_addr; /**< Address of TDH register. */
>  	uint16_t            nb_tx_desc;    /**< number of TX descriptors. */
>  	uint16_t            tx_tail;       /**< current value of TDT reg. */
>  	/**< Start freeing TX buffers if there are less free descriptors than
> --
> 2.11.0

Re: [dpdk-dev] [RFC] net/ixgbe: fix Tx descriptor status api

[Olivier Matz]

Hi Wei,

On Tue, Jun 26, 2018 at 01:38:22AM +0000, Zhao1, Wei wrote:
> Hi,  Olivier Matz
> 
>  Will you commit fix patch for i40e and ixgbe and em?

If you think the patch are relevant, yes :)

Here is a pre-version (last 5 patches):
http://git.droids-corp.org/?p=dpdk.git;a=shortlog;h=refs/heads/tx-desc

It still need to fix checkpatch issues, few more tests, and
rebase on next-net.

> And the code " dd = (desc / txq->tx_rs_thresh + 1) * txq->tx_rs_thresh - 1;"
> Is only proper for tx function ixgbe_xmit_pkts_simple  and ixgbe_xmit_pkts_vec ().
> But not proper for ixgbe_xmit_pkts (), the RS bit set rule is different from all these two. 

Can you please give more detail please?

Note this code, maybe you are talking about this?

+       /* In full featured mode, RS bit is only set in the last descriptor */
+       /* of a multisegments packet */
+       if (!((txq->offloads == 0) &&
+             (txq->tx_rs_thresh >= RTE_PMD_IXGBE_TX_MAX_BURST)))
+               dd = txq->sw_ring[dd].last_id;

Maybe there is something better to test?

Just to ensure we are on the same line, here are some more infos.

===

- sw advances the tail pointer
- hw advances the head pointer
- the software populates the ring with full buffers to be sent by
  the hw
- head points to the in-progress descriptor.
- sw writes new descriptors at tail
- head == tail means that the transmit queue is empty
- when the hw has processed a descriptor, it sets the DD bit if
  the descriptor has the RS (report status) bit.
- the driver never reads the head (needs a pci transaction), instead
it monitors the DD bit of a descriptor that has the RS bit

txq->tx_tail: sw value for tail register
txq->tx_free_thresh: free buffers if count(free descs) < this value
txq->tx_rs_thresh: RS bit is set every rs_thresh descriptor
txq->tx_next_dd: next desc to scan for DD bit
txq->tx_next_rs: next desc to set RS bit
txq->last_desc_cleaned: last descriptor that have been cleaned
txq->nb_tx_free: number of free descriptors

Example:

|----------------------------------------------------------------|
|               D       R       R       R                        |
|        ............xxxxxxxxxxxxxxxxxxxxxxxxx                   |
|        <descs sent><- descs not sent yet  ->                   |
|        ............xxxxxxxxxxxxxxxxxxxxxxxxx                   |
|----------------------------------------------------------------|
        ^last_desc_cleaned=8                    ^next_rs=47
                ^next_dd=15                   ^sw_tail=45
                     ^hw_head=20

                     <----  nb_used  --------->

The hardware is currently processing the descriptor 20
'R' means the descriptor has the RS bit
'D' means the descriptor has the DD + RS bits
'x' are packets in txq (not sent)
'.' are packet already sent but not freed by sw

In this example, we have rs_thres=8. On next call to
ixgbe_tx_free_bufs(), some buffers will be freed.

===

Let's call ixgbe_dev_tx_descriptor_status(10):


- original version:

desc = 45 + 10 = 55
desc = ((55 + 8 - 1) / 8) * 8 = (62 / 8) * 8 = 56

wrong because it goes in the wrong direction, and because
56 does not have the RS bit

- after your patch:

desc = 45 + 10 = 55
desc = (((55 / 8) + 1) * 8) - 1 = (7 * 8) - 1 = 55

wrong because it goes in the wrong direction

- after my patch

desc = 45 - 10 - 1 = 34
desc = (((34 / 8) + 1) * 8) - 1 = (5 * 8) - 1 = 39

looks correct



Regards,
Olivier

Re: [dpdk-dev] [RFC] net/ixgbe: fix Tx descriptor status api

[Zhao1, Wei]

Hi, Olivier Matz

> -----Original Message-----
> From: Olivier Matz [mailto:olivier.matz@6wind.com]
> Sent: Tuesday, June 26, 2018 4:46 PM
> To: Zhao1, Wei <wei.zhao1@intel.com>
> Cc: Zhang, Qi Z <qi.z.zhang@intel.com>; dev@dpdk.org; Lu, Wenzhuo
> <wenzhuo.lu@intel.com>
> Subject: Re: [RFC] net/ixgbe: fix Tx descriptor status api
> 
> Hi Wei,
> 
> On Tue, Jun 26, 2018 at 01:38:22AM +0000, Zhao1, Wei wrote:
> > Hi,  Olivier Matz
> >
> >  Will you commit fix patch for i40e and ixgbe and em?
> 
> If you think the patch are relevant, yes :)
> 
> Here is a pre-version (last 5 patches):
> http://git.droids-corp.org/?p=dpdk.git;a=shortlog;h=refs/heads/tx-desc
> 
> It still need to fix checkpatch issues, few more tests, and rebase on next-net.
> 
> > And the code " dd = (desc / txq->tx_rs_thresh + 1) * txq->tx_rs_thresh -
> 1;"
> > Is only proper for tx function ixgbe_xmit_pkts_simple  and
> ixgbe_xmit_pkts_vec ().
> > But not proper for ixgbe_xmit_pkts (), the RS bit set rule is different from
> all these two.
> 
> Can you please give more detail please?
> 
> Note this code, maybe you are talking about this?


https://mails.dpdk.org/archives/dev/2018-June/105053.html
This reminder is from zhangqi to my patch, he seems reasonable.
He has find that in tx function  ixgbe_xmit_pkts (), it will deal with some packets with several segments.
In this case, txq->nb_tx_used = 0 will be set after set RS bit even if the last packet span several segements, but 
" dd = (desc / txq->tx_rs_thresh + 1) * txq->tx_rs_thresh -1" does not considering this problem.
Although we can change code to "txq->nb_tx_used = 0;  ------>  txq->nb_tx_used = txq->nb_tx_used - txq->tx_rs_thresh"
But that will cause problem in function ixgbe_xmit_cleanup().
So, I agree with zhangqi for this point.


> 
> +       /* In full featured mode, RS bit is only set in the last descriptor */
> +       /* of a multisegments packet */
> +       if (!((txq->offloads == 0) &&
> +             (txq->tx_rs_thresh >= RTE_PMD_IXGBE_TX_MAX_BURST)))
> +               dd = txq->sw_ring[dd].last_id;
> 
> Maybe there is something better to test?
> 
> Just to ensure we are on the same line, here are some more infos.
> 
> ===
> 
> - sw advances the tail pointer
> - hw advances the head pointer
> - the software populates the ring with full buffers to be sent by
>   the hw
> - head points to the in-progress descriptor.
> - sw writes new descriptors at tail
> - head == tail means that the transmit queue is empty
> - when the hw has processed a descriptor, it sets the DD bit if
>   the descriptor has the RS (report status) bit.
> - the driver never reads the head (needs a pci transaction), instead it
> monitors the DD bit of a descriptor that has the RS bit
> 
> txq->tx_tail: sw value for tail register
> txq->tx_free_thresh: free buffers if count(free descs) < this value
> txq->tx_rs_thresh: RS bit is set every rs_thresh descriptor
> txq->tx_next_dd: next desc to scan for DD bit
> txq->tx_next_rs: next desc to set RS bit
> txq->last_desc_cleaned: last descriptor that have been cleaned
> txq->nb_tx_free: number of free descriptors
> 
> Example:
> 
> |----------------------------------------------------------------|
> |               D       R       R       R                        |
> |        ............xxxxxxxxxxxxxxxxxxxxxxxxx                   |
> |        <descs sent><- descs not sent yet  ->                   |
> |        ............xxxxxxxxxxxxxxxxxxxxxxxxx                   |
> |----------------------------------------------------------------|
>         ^last_desc_cleaned=8                    ^next_rs=47
>                 ^next_dd=15                   ^sw_tail=45
>                      ^hw_head=20
> 
>                      <----  nb_used  --------->
> 
> The hardware is currently processing the descriptor 20 'R' means the
> descriptor has the RS bit 'D' means the descriptor has the DD + RS bits 'x' are
> packets in txq (not sent) '.' are packet already sent but not freed by sw
> 
> In this example, we have rs_thres=8. On next call to ixgbe_tx_free_bufs(),
> some buffers will be freed.
> 
> ===
> 
> Let's call ixgbe_dev_tx_descriptor_status(10):
> 
> 
> - original version:
> 
> desc = 45 + 10 = 55
> desc = ((55 + 8 - 1) / 8) * 8 = (62 / 8) * 8 = 56
> 
> wrong because it goes in the wrong direction, and because
> 56 does not have the RS bit
> 
> - after your patch:
> 
> desc = 45 + 10 = 55
> desc = (((55 / 8) + 1) * 8) - 1 = (7 * 8) - 1 = 55
> 
> wrong because it goes in the wrong direction
> 
> - after my patch
> 
> desc = 45 - 10 - 1 = 34
> desc = (((34 / 8) + 1) * 8) - 1 = (5 * 8) - 1 = 39
> 
> looks correct

It seems you have change the definition of "offset", it is before tail not after.
My patch is based on the old comment in RTE function. If so, your patch will be ok.
BTW, last_desc_cleaned should be 7, not 8.


> 
> 
> 
> Regards,
> Olivier

Re: [dpdk-dev] [RFC] net/ixgbe: fix Tx descriptor status api

[Zhang, Qi Z]

Hi Oliver

> -----Original Message-----
> From: Olivier Matz [mailto:olivier.matz@6wind.com]
> Sent: Tuesday, June 26, 2018 4:46 PM
> To: Zhao1, Wei <wei.zhao1@intel.com>
> Cc: Zhang, Qi Z <qi.z.zhang@intel.com>; dev@dpdk.org; Lu, Wenzhuo
> <wenzhuo.lu@intel.com>
> Subject: Re: [RFC] net/ixgbe: fix Tx descriptor status api
> 
> Hi Wei,
> 
> On Tue, Jun 26, 2018 at 01:38:22AM +0000, Zhao1, Wei wrote:
> > Hi,  Olivier Matz
> >
> >  Will you commit fix patch for i40e and ixgbe and em?
> 
> If you think the patch are relevant, yes :)
> 
> Here is a pre-version (last 5 patches):
> http://git.droids-corp.org/?p=dpdk.git;a=shortlog;h=refs/heads/tx-desc
> 
> It still need to fix checkpatch issues, few more tests, and rebase on next-net.
> 
> > And the code " dd = (desc / txq->tx_rs_thresh + 1) * txq->tx_rs_thresh - 1;"
> > Is only proper for tx function ixgbe_xmit_pkts_simple  and
> ixgbe_xmit_pkts_vec ().
> > But not proper for ixgbe_xmit_pkts (), the RS bit set rule is different from all
> these two.
> 
> Can you please give more detail please?

I think the problem in ixgbe_xmit_pkts is, we cannot guarantee tx_rs_thresh *n -1 will always get RS bit.
Because the next tx_rs_thresh cycle counted start from the last segment of the packet that cross the boundary, it could be any value. 

So probably we should return -ENOSUP if pkt_tx_burst = ixgbe_xmit_pkts,  or we need to change the method that write RS in ixgbe_xmit_pkts.

Btw, yes, we should look before the tail but not after, you fix is correct.

Thanks
Qi  

> 
> Note this code, maybe you are talking about this?
> 
> +       /* In full featured mode, RS bit is only set in the last descriptor */
> +       /* of a multisegments packet */
> +       if (!((txq->offloads == 0) &&
> +             (txq->tx_rs_thresh >= RTE_PMD_IXGBE_TX_MAX_BURST)))
> +               dd = txq->sw_ring[dd].last_id;
> 
> Maybe there is something better to test?
> 
> Just to ensure we are on the same line, here are some more infos.
> 
> ===
> 
> - sw advances the tail pointer
> - hw advances the head pointer
> - the software populates the ring with full buffers to be sent by
>   the hw
> - head points to the in-progress descriptor.
> - sw writes new descriptors at tail
> - head == tail means that the transmit queue is empty
> - when the hw has processed a descriptor, it sets the DD bit if
>   the descriptor has the RS (report status) bit.
> - the driver never reads the head (needs a pci transaction), instead it monitors
> the DD bit of a descriptor that has the RS bit
> 
> txq->tx_tail: sw value for tail register
> txq->tx_free_thresh: free buffers if count(free descs) < this value
> txq->tx_rs_thresh: RS bit is set every rs_thresh descriptor
> txq->tx_next_dd: next desc to scan for DD bit
> txq->tx_next_rs: next desc to set RS bit
> txq->last_desc_cleaned: last descriptor that have been cleaned
> txq->nb_tx_free: number of free descriptors
> 
> Example:
> 
> |----------------------------------------------------------------|
> |               D       R       R       R
> |
> |        ............xxxxxxxxxxxxxxxxxxxxxxxxx                   |
> |        <descs sent><- descs not sent yet  ->                   |
> |        ............xxxxxxxxxxxxxxxxxxxxxxxxx                   |
> |----------------------------------------------------------------|
>         ^last_desc_cleaned=8                    ^next_rs=47
>                 ^next_dd=15                   ^sw_tail=45
>                      ^hw_head=20
> 
>                      <----  nb_used  --------->
> 
> The hardware is currently processing the descriptor 20 'R' means the
> descriptor has the RS bit 'D' means the descriptor has the DD + RS bits 'x' are
> packets in txq (not sent) '.' are packet already sent but not freed by sw
> 
> In this example, we have rs_thres=8. On next call to ixgbe_tx_free_bufs(),
> some buffers will be freed.
> 
> ===
> 
> Let's call ixgbe_dev_tx_descriptor_status(10):
> 
> 
> - original version:
> 
> desc = 45 + 10 = 55
> desc = ((55 + 8 - 1) / 8) * 8 = (62 / 8) * 8 = 56
> 
> wrong because it goes in the wrong direction, and because
> 56 does not have the RS bit
> 
> - after your patch:
> 
> desc = 45 + 10 = 55
> desc = (((55 / 8) + 1) * 8) - 1 = (7 * 8) - 1 = 55
> 
> wrong because it goes in the wrong direction
> 
> - after my patch
> 
> desc = 45 - 10 - 1 = 34
> desc = (((34 / 8) + 1) * 8) - 1 = (5 * 8) - 1 = 39
> 
> looks correct
> 
> 
> 
> Regards,
> Olivier

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

[Wei Zhao]

This is an 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.

Fixes: a2919e13d95e ("net/ixgbe: implement descriptor status API")

Signed-off-by: Wei Zhao <wei.zhao1@intel.com>

---

v2:
-add not support case for this feature
---
 drivers/net/ixgbe/ixgbe_rxtx.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ixgbe/ixgbe_rxtx.c b/drivers/net/ixgbe/ixgbe_rxtx.c
index 3e13d26..087657c 100644
--- a/drivers/net/ixgbe/ixgbe_rxtx.c
+++ b/drivers/net/ixgbe/ixgbe_rxtx.c
@@ -3145,16 +3145,20 @@ ixgbe_dev_tx_descriptor_status(void *tx_queue, uint16_t offset)
 	if (unlikely(offset >= txq->nb_tx_desc))
 		return -EINVAL;
 
+	if (rte_eth_devices[txq->port_id].tx_pkt_burst ==
+		ixgbe_xmit_pkts)
+		return -ENOTSUP;
+
 	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