Re: [dpdk-dev] [PATCH v6 3/4] net/ixgbe: cleanup Tx buffers

["Ananyev, Konstantin" <konstantin.ananyev@intel.com>]

Hi Chenxu,

> > > > > > > + * tx_tail is the last sent packet on the sw_ring. Goto the
> > > > > > > + end
> > > > > > > + * of that packet (the last segment in the packet chain) and
> > > > > > > + * then the next segment will be the start of the oldest
> > > > > > > + segment
> > > > > > > + * in the sw_ring.
> > > > > >
> > > > > > Not sure I understand the sentence above.
> > > > > > tx_tail is the value of TDT HW register (most recently armed by SW TD).
> > > > > > last_id  is the index of last descriptor for multi-seg packet.
> > > > > > next_id is just the index of next descriptor in HW TD ring.
> > > > > > How do you conclude that it will be the ' oldest segment in the sw_ring'?
> > > > > >
> > > > >
> > > > > The tx_tail is the last sent packet on the sw_ring. While the
> > > > > xmit_cleanup or Tx_free_bufs will be call when the nb_tx_free <
> > > > tx_free_thresh .
> > > > > So the sw_ring[tx_tail].next_id must be the begin of mbufs which
> > > > > are not used or  Already freed . then begin the loop until the
> > > > > mbuf is used and
> > > > begin to free them.
> > > > >
> > > > >
> > > > >
> > > > > > Another question why do you need to write your own functions?
> > > > > > Why can't you reuse existing ixgbe_xmit_cleanup() for
> > > > > > full(offload) path and
> > > > > > ixgbe_tx_free_bufs() for simple path?
> > > > > > Yes,  ixgbe_xmit_cleanup() doesn't free mbufs, but at least it
> > > > > > could be used to determine finished TX descriptors.
> > > > > > Based on that you can you can free appropriate sw_ring[] entries.
> > > > > >
> > > > >
> > > > > The reason why I don't reuse existing function is that they all
> > > > > free several mbufs While the free_cnt of the API
> > > > > rte_eth_tx_done_cleanup() is the
> > > > number of packets.
> > > > > It also need to be done that check which mbuffs are from the same packet.
> > > >
> > > > At first, I don't see anything bad if tx_done_cleanup() will free
> > > > only some segments from the packet. As long as it is safe - there is no
> > problem with that.
> > > > I think rte_eth_tx_done_cleanup() operates on mbuf, not packet quantities.
> > > > But in our case I think it doesn't matter, as ixgbe_xmit_cleanup()
> > > > mark TXDs as free only when HW is done with all TXDs for that packet.
> > > > As long as there is a way to reuse existing code and avoid
> > > > duplication (without introducing any degradation) - we should use it.
> > > > And I think there is a very good opportunity here to reuse existing
> > > > ixgbe_xmit_cleanup() for tx_done_cleanup() implementation.
> > > > Moreover because your code doesn’t follow
> > > > ixgbe_xmit_pkts()/ixgbe_xmit_cleanup()
> > > > logic and infrastructure, it introduces unnecessary scans over TXD
> > > > ring, and in some cases doesn't work as expected:
> > > >
> > > > +while (1) {
> > > > +tx_last = sw_ring[tx_id].last_id;
> > > > +
> > > > +if (sw_ring[tx_last].mbuf) {
> > > > +if (txr[tx_last].wb.status &
> > > > +IXGBE_TXD_STAT_DD) {
> > > > ...
> > > > +} else {
> > > > +/*
> > > > + * mbuf still in use, nothing left to
> > > > + * free.
> > > > + */
> > > > +break;
> > > >
> > > > It is not correct to expect that IXGBE_TXD_STAT_DD will be set on
> > > > last TXD for
> > > > *every* packet.
> > > > We set IXGBE_TXD_CMD_RS bit only on threshold packet last descriptor.
> > > > Plus ixgbe_xmit_cleanup() can cleanup TXD wb.status.
> > > >
> > > > So I strongly recommend to reuse ixgbe_xmit_cleanup() here.
> > > > It would be much less error prone and will help to avoid code duplication.
> > > >
> > > > Konstantin
> > > >
> > >
> > > At first.
> > > The function ixgbe_xmit_cleanup(struct ixgbe_tx_queue *txq) will cleanup
> > TXD wb.status.
> > > the number of status cleanuped is always txq->tx_rs_thresh.
> >
> > Yes, and what's wrong with it?
> >
> > >
> > > The  API rte_eth_tx_done_cleanup() in rte_eth_dev.h show that @param
> > > free_cnt
> > >  *   Maximum number of packets to free. Use 0 to indicate all possible packets
> > >  *   should be freed. Note that a packet may be using multiple mbufs.
> >
> > I don't think it is a good approach, better would be to report number of freed
> > mbufs, but ok, as it is a public API, we probably need to keep it as it is.
> >
> > > a number must be set while ixgbe_xmit_cleanup and ixgbe_tx_free_bufs only
> > have one parameter txq.
> >
> > Yes, ixgbe_xmit_cleanup() cleans up at least txq->tx_rs_thresh TXDs.
> > So if user requested more packets to be freed we can call ixgbe_xmit_cleanup()
> > in a loop.
> >
> 
> That is a great idea and I discuss with my workmate about it today. there is also some
> Question that we don’t confirm.
> Actually it can call ixgbe_xmit_cleanup() in a loop if user requested more packets,
> How to deal with the MOD. For example:
> 	The default tx_rs_thresh is 32.if the count of mbufs we need free is 50.
> 	We can call ixgbe_xmit_cleanup() one time to free 32 mbufs.
> 	Then how about other 18 mbufs.
> 	1.If we do nothing, it looks not good.
> 	2.if we call ixgbe_xmit_cleanup() successfully, we free 14 mbufs more.
> 	3.if we call ixgbe_xmit_cleanup() fail, the status of No.32 mbufs is not DD
> 	  While No .18 is DD. So we do not free 18 mbufs what we can and should free.
> 
> We have try some plans about it, likes add parameter for ixgbe_xmit_cleanup(), change
>  Tx_rs_thresh or copy the code or ixgbe_xmit_cleanup() as a new function with more
> Parameter. But all of them seem not perfect.
> 
> So  can you give some comment about it? It seems not easy as we think by reuse function.

My thought about it:
for situations when cnt % rs_thresh != 0
we'll still call ixgbe_xmit_cleanup() cnt / rs_thresh + 1 times.
But then, we can free just cnt % rs_thresh mbufs, while keeping
rest of them intact. 
  
Let say at some moment we have txq->nb_tx_free==0,
and user called tx_done_cleanup(txq, ..., cnt==50)
So we call ixgbe_xmit_cleanup(txq) first time.
Suppose it frees 32 TXDs, then we walk through corresponding
sw_ring[] entries and let say free 32 packets (one mbuf per packet).
Then we call ixgbe_xmit_cleanup(txq)  second time.  
Suppose it will free another 32 TXDs, we again walk thour sw_ring[],
but free only first 18 mbufs and return.
Suppose we call tx_done_cleanup(txq, cnt=50) immediately again.
Now txq->nb_tx_free==64, so we can start to scan sw_entries[]
from tx_tail straightway. We'll skip first 50 entries as they are 
already empty, then free remaining 14 mbufs, then will
call ixgbe_xmit_cleanup(txq) again, and if it would be successful,
will scan and free another 32 sw_ring[] entries.
Then again will call ixgbe_xmit_cleanup(txq), but will free
only first 8 available sw_ring[].mbuf.   
Probably a bit easier with the code:

tx_done_cleanup(..., uint32_t cnt)
{
       swr = txq->sw_ring;
       txr     = txq->tx_ring;
       id   = txq->tx_tail;
  
      if (txq->nb_tx_free == 0)
         ixgbe_xmit_cleanup(txq);

      free =  txq->nb_tx_free;       
      for (n = 0; n < cnt && free != 0; ) {

          for (j = 0; j != free && n < cnt; j++) {
             swe = &swr[id + j];
             if (swe->mbuf != NULL) {
                   rte_pktmbuf_free_seg(swe->mbuf);
                   swe->mbuf = NULL;
             
                  /* last segment in the packet, increment packet count */
                  n += (swe->last_id == id + j);
             }
          } 

          if (n < cnt) { 
               ixgbe_xmit_cleanup(txq);
               free =   txq->nb_tx_free - free;
          }
     }
     return n;
}

For the situation when there are less then rx_thresh free TXDs
((txq->tx_ring[desc_to_clean_to].wb.status & IXGBE_TXD_STAT_DD) == 0)
we do nothing - in that case we consider there are no more mbufs to free.

> 
> 
> > > And what should do is not only free buffers and status but also check
> > > which bufs are from  One packet and count the packet freed.
> >
> > ixgbe_xmit_cleanup() itself doesn't free mbufs itself.
> > It only cleans up TXDs.
> > So in tx_done_cleanup() after calling ixgbe_xmit_cleanup() you'll still need to go
> > through sw_ring[] entries that correspond to free TXDs and call mbuf_seg_free().
> > You can count number of full packets here.
> >
> > > So I think it can't be implemented that reuse function xmit_cleanup without
> > change it.
> > > And create a new function with the code of xmit_cleanup will cause many
> > duplication.
> >
> > I don't think it would.
> > I think all we need here is something like that (note it is schematic one, doesn't
> > take into accounf that TXD ring is circular):
> >
> > tx_done_cleanup(..., uint32_t cnt)
> > {
> >       /* we have txq->nb_tx_free TXDs starting from txq->tx_tail.
> >            Scan them first and free as many mbufs as we can.
> >            If we need more mbufs to free call  ixgbe_xmit_cleanup()
> >            to free more TXDs. */
> >
> >        swr = txq->sw_ring;
> >        txr     = txq->tx_ring;
> >        id   = txq->tx_tail;
> >        free =  txq->nb_tx_free;
> >
> >        for (n = 0; n < cnt && free != 0; ) {
> >
> >           for (j = 0; j != free && n < cnt; j++) {
> >              swe = &swr[id + j];
> >              if (swe->mbuf != NULL) {
> >                    rte_pktmbuf_free_seg(swe->mbuf);
> >                    swe->mbuf = NULL;
> >              }
> >              n += (swe->last_id == id + j)
> >           }
> >
> >           if (n < cnt) {
> >                ixgbe_xmit_cleanup(txq);
> >                free =   txq->nb_tx_free - free;
> >           }
> >      }
> >      return n;
> > }
> >
> > >
> > > Above all , it seem not a perfect idea to reuse ixgbe_xmit_cleanup().
> >
> > Totally disagree, see above.
> >
> > >
> > > Second.
> > > The function in patch is copy from code in igb_rxtx.c. it already
> > > updated in 2017, The commit id is
> > 8d907d2b79f7a54c809f1c44970ff455fa2865e1.
> >
> > I realized that.
> > But I think it as a problem, not a positive thing.
> > While they do have some similarities, igb abd ixgbe are PMDs for different
> > devices, and their TX code differs quite a lot. Let say igb doesn't use
> > tx_rs_threshold, but instead set RS bit for each last TXD.
> > So, just blindly copying tx_done_cleanup() from igb to ixgbe doesn't look like a
> > good idea to me.
> >
> > > I trust the logic of code is right.
> > > Actually it don't complete for ixgbe, i40e and ice, while it don't
> > > change the value of last_desc_cleaned and tx_next_dd. And it's
> > > beginning prefer last_desc_cleaned or  tx_next_dd(for offload or simple) to
> > tx_tail.
> > >
> > > So, I suggest to use the old function and fix the issue.
> > >
> > > > >
> > > > >
> > > > > > >This is the first packet that will be
> > > > > > > + * attempted to be freed.
> > > > > > > + */
> > > > > > > +
> > > > > > > +/* Get last segment in most recently added packet. */ tx_last
> > > > > > > += sw_ring[txq->tx_tail].last_id;
> > > > > > > +
> > > > > > > +/* Get the next segment, which is the oldest segment in ring.
> > > > > > > +*/ tx_first = sw_ring[tx_last].next_id;
> > > > > > > +
> > > > > > > +/* Set the current index to the first. */ tx_id = tx_first;
> > > > > > > +
> > > > > > > +/*
> > > > > > > + * Loop through each packet. For each packet, verify that an
> > > > > > > + * mbuf exists and that the last segment is free. If so, free
> > > > > > > + * it and move on.
> > > > > > > + */
> > > > > > > +while (1) {
> > > > > > > +tx_last = sw_ring[tx_id].last_id;
> > > > > > > +
> > > > > > > +if (sw_ring[tx_last].mbuf) {
> > > > > > > +if (!(txr[tx_last].wb.status &
> > > > > > > +IXGBE_TXD_STAT_DD))
> > > > > > > +break;
> > > > > > > +
> > > > > > > +/* Get the start of the next packet. */ tx_next =
> > > > > > > +sw_ring[tx_last].next_id;
> > > > > > > +
> > > > > > > +/*
> > > > > > > + * Loop through all segments in a
> > > > > > > + * packet.
> > > > > > > + */
> > > > > > > +do {
> > > > > > > +rte_pktmbuf_free_seg(sw_ring[tx_id].mbuf);
> > > > > > > +sw_ring[tx_id].mbuf = NULL;
> > > > > > > +sw_ring[tx_id].last_id = tx_id;
> > > > > > > +
> > > > > > > +/* Move to next segment. */
> > > > > > > +tx_id = sw_ring[tx_id].next_id;
> > > > > > > +
> > > > > > > +} while (tx_id != tx_next);
> > > > > > > +
> > > > > > > +/*
> > > > > > > + * Increment the number of packets
> > > > > > > + * freed.
> > > > > > > + */
> > > > > > > +count++;
> > > > > > > +
> > > > > > > +if (unlikely(count == (int)free_cnt)) break; } else {
> > > > > > > +/*
> > > > > > > + * There are multiple reasons to be here:
> > > > > > > + * 1) All the packets on the ring have been
> > > > > > > + *    freed - tx_id is equal to tx_first
> > > > > > > + *    and some packets have been freed.
> > > > > > > + *    - Done, exit
> > > > > > > + * 2) Interfaces has not sent a rings worth of
> > > > > > > + *    packets yet, so the segment after tail is
> > > > > > > + *    still empty. Or a previous call to this
> > > > > > > + *    function freed some of the segments but
> > > > > > > + *    not all so there is a hole in the list.
> > > > > > > + *    Hopefully this is a rare case.
> > > > > > > + *    - Walk the list and find the next mbuf. If
> > > > > > > + *      there isn't one, then done.
> > > > > > > + */
> > > > > > > +if (likely(tx_id == tx_first && count != 0)) break;
> > > > > > > +
> > > > > > > +/*
> > > > > > > + * Walk the list and find the next mbuf, if any.
> > > > > > > + */
> > > > > > > +do {
> > > > > > > +/* Move to next segment. */
> > > > > > > +tx_id = sw_ring[tx_id].next_id;
> > > > > > > +
> > > > > > > +if (sw_ring[tx_id].mbuf)
> > > > > > > +break;
> > > > > > > +
> > > > > > > +} while (tx_id != tx_first);
> > > > > > > +
> > > > > > > +/*
> > > > > > > + * Determine why previous loop bailed. If there
> > > > > > > + * is not an mbuf, done.
> > > > > > > + */
> > > > > > > +if (sw_ring[tx_id].mbuf == NULL) break; } }
> > > > > > > +
> > > > > > > +return count;
> > > > > > > +}
> > > > > > > +
> > > > > > >  static void __attribute__((cold))
> > > > > > > ixgbe_tx_free_swring(struct ixgbe_tx_queue *txq)  { diff --git
> > > > > > > a/drivers/net/ixgbe/ixgbe_rxtx.h
> > > > > > > b/drivers/net/ixgbe/ixgbe_rxtx.h index 505d344b9..2c3770af6
> > > > > > > 100644
> > > > > > > --- a/drivers/net/ixgbe/ixgbe_rxtx.h
> > > > > > > +++ b/drivers/net/ixgbe/ixgbe_rxtx.h
> > > > > > > @@ -285,6 +285,8 @@ int
> > > > > > > ixgbe_rx_vec_dev_conf_condition_check(struct
> > > > > > > rte_eth_dev *dev);  int ixgbe_rxq_vec_setup(struct
> > > > > > > ixgbe_rx_queue *rxq);  void
> > > > > > > ixgbe_rx_queue_release_mbufs_vec(struct
> > > > > > > ixgbe_rx_queue *rxq);
> > > > > > >
> > > > > > > +int ixgbe_tx_done_cleanup(void *txq, uint32_t free_cnt);
> > > > > > > +
> > > > > > >  extern const uint32_t ptype_table[IXGBE_PACKET_TYPE_MAX];
> > > > > > >  extern const uint32_t
> > > > > > > ptype_table_tn[IXGBE_PACKET_TYPE_TN_MAX];
> > > > > > >
> > > > > > > --
> > > > > > > 2.17.1
> > > > > >
> > > > >
> > > >
> > >
> >
>