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