Hi Oliver > -----Original Message----- > From: Olivier Matz [mailto:olivier.m...@6wind.com] > Sent: Tuesday, June 26, 2018 4:46 PM > To: Zhao1, Wei <wei.zh...@intel.com> > Cc: Zhang, Qi Z <qi.z.zh...@intel.com>; dev@dpdk.org; Lu, Wenzhuo > <wenzhuo...@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
