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

["Zhao1, Wei" <wei.zhao1@intel.com>]

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