Re: [dpdk-dev] [PATCH] ixgbe: fix checking for tx_free_thresh

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

> -----Original Message-----
> From: Zoltan Kiss [mailto:zoltan.kiss@linaro.org]
> Sent: Tuesday, June 09, 2015 4:08 PM
> To: Ananyev, Konstantin; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] ixgbe: fix checking for tx_free_thresh
> 
> 
> 
> On 09/06/15 12:18, Ananyev, Konstantin wrote:
> >
> >
> >> -----Original Message-----
> >> From: Zoltan Kiss [mailto:zoltan.kiss@linaro.org]
> >> Sent: Wednesday, June 03, 2015 6:47 PM
> >> To: Ananyev, Konstantin; dev@dpdk.org
> >> Subject: Re: [dpdk-dev] [PATCH] ixgbe: fix checking for tx_free_thresh
> >>
> >>
> >>
> >> On 02/06/15 18:35, Ananyev, Konstantin wrote:
> >>>
> >>>
> >>>> -----Original Message-----
> >>>> From: Zoltan Kiss [mailto:zoltan.kiss@linaro.org]
> >>>> Sent: Tuesday, June 02, 2015 4:08 PM
> >>>> To: Ananyev, Konstantin; dev@dpdk.org
> >>>> Subject: Re: [dpdk-dev] [PATCH] ixgbe: fix checking for tx_free_thresh
> >>>>
> >>>>
> >>>>
> >>>> On 02/06/15 14:31, Ananyev, Konstantin wrote:
> >>>>> Hi Zoltan,
> >>>>>
> >>>>>> -----Original Message-----
> >>>>>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Zoltan Kiss
> >>>>>> Sent: Monday, June 01, 2015 5:16 PM
> >>>>>> To: dev@dpdk.org
> >>>>>> Subject: Re: [dpdk-dev] [PATCH] ixgbe: fix checking for tx_free_thresh
> >>>>>>
> >>>>>> Hi,
> >>>>>>
> >>>>>> Anyone would like to review this patch? Venky sent a NAK, but I've
> >>>>>> explained to him why it is a bug.
> >>>>>
> >>>>>
> >>>>> Well, I think Venky is right here.
> >>>> I think the comments above rte_eth_tx_burst() definition are quite clear
> >>>> about what tx_free_thresh means, e1000 and i40e use it that way, but not
> >>>> ixgbe.
> >>>>
> >>>>> Indeed that fix, will cause more often unsuccessful checks for DD bits and might cause a
> >>>>> slowdown for TX fast-path.
> >>>> Not if the applications set tx_free_thresh according to the definition
> >>>> of this value. But we can change the default value from 32 to something
> >>>> higher, e.g I'm using nb_desc/2, and it works out well.
> >>>
> >>> Sure we can, as I said below, we can unify it one way or another.
> >>> One way would be to make fast-path TX to free TXDs when number of occupied TXDs raises above tx_free_thresh
> >>> (what rte_ethdev.h comments say and what full-featured TX is doing).
> >>> Though in that case we have to change default value for tx_free_thresh, and all existing apps that
> >>> using tx_free_thresh==32 and fast-path TX will probably experience a slowdown.
> >>
> >> They are in trouble already, because i40e and e1000 uses it as defined.
> >
> > In fact, i40e has exactly the same problem as ixgbe:
> > fast-path and full-featured TX  code treat  tx_free_thresh in a different way.
> > igb just ignores input tx_free_thresh, while em has only full featured path.
> >
> > What I am saying, existing app that uses TX fast-path and sets tx_free_thresh=32
> > (as we did in our examples in previous versions) will experience a slowdown,
> > if we'll make all TX functions to behave like full-featured ones
> > (txq->nb_tx_desc - txq->nb_tx_free > txq->tx_free_thresh).
> >
> >  From other side, if app uses TX full-featured TX and sets tx_free_thresh=32,
> > then it  already has a possible slowdown, because of too often TXDs checking.
> > So, if we'll change tx_free_thresh semantics to wht fast-path uses,
> > It shouldn't see any slowdown, in fact it might see some improvement.
> >
> >> But I guess most apps are going with 0, which sets the drivers default.
> >> Others have to change the value to nb_txd - curr_value to have the same
> >> behaviour
> >>
> >>> Another way would be to make all TX functions to treat tx_conf->tx_free_thresh as fast-path TX functions do
> >>> (free TXDs when number of free TXDs drops below  tx_free_thresh) and update  rte_ethdev.h comments.
> >> And i40e and e1000e code as well. I don't see what difference it makes
> >> which way of definition you use, what I care is that it should be used
> >> consistently.
> >
> > Yes, both ways are possible, the concern is - how to minimise the impact for existing apps.
> > That's why I am leaning to the fast-path way.
> 
> Make sense to favour the fast-path way, I'll look into that and try to
> come up with a patch
> 
> >
> >>>
> >>> Though, I am not sure that it really worth all these changes.
> >>>   From one side, whatever tx_free_thresh would be,
> >>> the app should still assume that the worst case might happen,
> >>> and up to nb_tx_desc mbufs can be consumed by the queue.
> >>>   From other side, I think the default value should work well for most cases.
> >>> So I am still for graceful deprecation of that config parameter, see below.
> >>>
> >>>>
> >>>>> Anyway, with current PMD implementation, you can't guarantee that at any moment
> >>>>> TX queue wouldn't use more than tx_free_thresh mbufs.
> >>>>
> >>>>
> >>>>> There could be situations (low speed, or link is down for some short period, etc), when
> >>>>> much more than tx_free_thresh TXDs are in use and none of them could be freed by HW right now.
> >>>>> So your app better be prepared, that up to (nb_tx_desc * num_of_TX_queues) could be in use
> >>>>> by TX path at any given moment.
> >>>>>
> >>>>> Though yes,  there is an inconsistency how different ixgbe TX functions treat tx_conf->tx_free_thresh parameter.
> >>>>> That probably creates wrong expectations and confusion.
> >>>> Yes, ixgbe_xmit_pkts() use it the way it's defined, this two function
> >>>> doesn't.
> >>>>
> >>>>> We might try to unify it's usage one way or another, but I personally don't see much point in it.
> >>>>> After all, tx_free_tresh seems like a driver internal choice (based on the nb_tx_desc and other parameters).
> >>>>> So I think a better way would be:
> >>>>> 1. Deprecate tx_conf->tx_free_thresh (and remove it in later releases) and make
> >>>>> each driver to use what it thinks would be the best value.
> >>>> But how does the driver knows what's the best for the applications
> >>>> traffic pattern? I think it's better to leave the possibility for the
> >>>> app to fine tune it.
> >>>
> >>> My understanding is that for most cases the default value should do pretty well.
> >>> That default value, shouldn't be too small, so we avoid unnecessary & unsuccessful checks,
> >>> and probably shouldn't be too big, to prevent unnecessary mbufs consumption
> >>> (something between nb_tx_desc / 2 and 3 * nb_tx_desc / 4 probably).
> >> I agree
> >>
> >>>
> >>> But might be you have a good example, when such tuning is needed?
> >>> For what traffic patterns you would set tx_free_thresh to some different values,
> >>> and how will it impact performance?
> >> I don't have an actual example, but I think it's worth to keep this
> >> tuning option if we already have it. Most people probably wouldn't use
> >> it, but I can imagine that the very enthusiastic wants to try out
> >> different settings to find the best.
> >> E.g. I was testing odp_l2fwd when I came across the problem, and I found
> >> it useful to have this option. With its traffic pattern (receive a batch
> >> of packets then send them out on an another interface) it can happen
> >> that with different clock speeds you can find different optimums.
> >
> > Actually, after thinking about it a bit more -
> > I think it would more depend on how many RX/TX queues particular  lcore has to manage.
> > So, as you said, yes we probably better leave it, at least for now.
> >
> >>
> >>>
> >>> Again, if there would be tx_free_pkts(), why someone would also need a tx_conf->tx_free_thresh?
> >> I think about tx_free_pkts as a rainy day option, when you want ALL TX
> >> completed packets to be released, because you are out of buffers.
> >
> > What do you mean as 'rainy day' here?
> > App getting short of mbufs?
> > As I said before, it could be absolutely valid situation when
> > up to nb_tx_desc mbufs per TX queue can be in use.
> > So the app better be prepared for such situation anyway.
> > Either make sure there are enough mbufs in the pool,
> > or somehow gracefully degrade the service.
> > Again, if nb_tx_desc is big (let say for ixgbe it could be up to 4K),
> > freeing all TXDs at once could take a long time.
> > So I think it should be possible to specify maximum number of mbufs to free per call.
> >
> > My thought people will use it to release mbufs when there is an idle period.
> > Something like:
> >
> > n = rx_burst(...);
> > if (n == 0)  { ...; tx_free_mbufs(...); ... }
> > else {...}
> 
> Yes, this is similar to what I'm doing in odp-dpdk:
> 
> https://git.linaro.org/lng/odp-dpdk.git/commitdiff/1a8df254e18bb50dbd835729bc3d01fcb87ebc6b
> 
> The only problem is when the tx_free_threshold is not reached, the
> interfaces doesn't even start releasing the mbufs. And when you can't
> receive anything, it's likely you won't send out anything as well, which
> would normally trigger the releasing of the completed buffers.

Yep, that's why I think it is good to add this function:
If you are going to TX something, then you'll probably hit tx_free_thresh
and PMD will release unused mbufs 'automatically'.
If you have nothing to TX (idle period), you can try to release unused mbufs manually:
by calling tx_free_mbufs().

Konstantin

> 
> >
> > Konstantin
> >
> >> While
> >> tx_free_thresh is the fast path way of TX completion, when you have the
> >> room to wait for more packets to be gathered.
> >>
> >>>
> >>> Konstantin
> >>>
> >>>> In the meantime we can improve the default selection as well, as I
> >>>> suggested above.
> >>>>
> >>>>> 2. As you suggested in another mail, introduce an new function:
> >>>>> uint16_t rte_eth_tx_free_pkts(port_id, queue_id, nb_to_free).
> >>>>> That would give upper layer a better control of memory usage, and might be called by the upper layer at idle time,
> >>>>> so further tx_burst, don't need to spend time on freeing TXDs/packets.
> >>>> I agree.
> >>>>
> >>>>>
> >>>>> Konstantin
> >>>>>
> >>>>>
> >>>>>>
> >>>>>> Regards,
> >>>>>>
> >>>>>> Zoltan
> >>>>>>
> >>>>>> On 27/05/15 21:12, Zoltan Kiss wrote:
> >>>>>>> This check doesn't do what's required by rte_eth_tx_burst:
> >>>>>>> "When the number of previously sent packets reached the "minimum transmit
> >>>>>>> packets to free" threshold"
> >>>>>>>
> >>>>>>> This can cause problems when txq->tx_free_thresh + [number of elements in the
> >>>>>>> pool] < txq->nb_tx_desc.
> >>>>>>>
> >>>>>>> Signed-off-by: Zoltan Kiss <zoltan.kiss@linaro.org>
> >>>>>>> ---
> >>>>>>>      drivers/net/ixgbe/ixgbe_rxtx.c     | 4 ++--
> >>>>>>>      drivers/net/ixgbe/ixgbe_rxtx_vec.c | 2 +-
> >>>>>>>      2 files changed, 3 insertions(+), 3 deletions(-)
> >>>>>>>
> >>>>>>> diff --git a/drivers/net/ixgbe/ixgbe_rxtx.c b/drivers/net/ixgbe/ixgbe_rxtx.c
> >>>>>>> index 4f9ab22..b70ed8c 100644
> >>>>>>> --- a/drivers/net/ixgbe/ixgbe_rxtx.c
> >>>>>>> +++ b/drivers/net/ixgbe/ixgbe_rxtx.c
> >>>>>>> @@ -250,10 +250,10 @@ tx_xmit_pkts(void *tx_queue, struct rte_mbuf **tx_pkts,
> >>>>>>>
> >>>>>>>      	/*
> >>>>>>>      	 * Begin scanning the H/W ring for done descriptors when the
> >>>>>>> -	 * number of available descriptors drops below tx_free_thresh.  For
> >>>>>>> +	 * number of in flight descriptors reaches tx_free_thresh. For
> >>>>>>>      	 * each done descriptor, free the associated buffer.
> >>>>>>>      	 */
> >>>>>>> -	if (txq->nb_tx_free < txq->tx_free_thresh)
> >>>>>>> +	if ((txq->nb_tx_desc - txq->nb_tx_free) > txq->tx_free_thresh)
> >>>>>>>      		ixgbe_tx_free_bufs(txq);
> >>>>>>>
> >>>>>>>      	/* Only use descriptors that are available */
> >>>>>>> diff --git a/drivers/net/ixgbe/ixgbe_rxtx_vec.c b/drivers/net/ixgbe/ixgbe_rxtx_vec.c
> >>>>>>> index abd10f6..f91c698 100644
> >>>>>>> --- a/drivers/net/ixgbe/ixgbe_rxtx_vec.c
> >>>>>>> +++ b/drivers/net/ixgbe/ixgbe_rxtx_vec.c
> >>>>>>> @@ -598,7 +598,7 @@ ixgbe_xmit_pkts_vec(void *tx_queue, struct rte_mbuf **tx_pkts,
> >>>>>>>      	if (unlikely(nb_pkts > RTE_IXGBE_VPMD_TX_BURST))
> >>>>>>>      		nb_pkts = RTE_IXGBE_VPMD_TX_BURST;
> >>>>>>>
> >>>>>>> -	if (txq->nb_tx_free < txq->tx_free_thresh)
> >>>>>>> +	if ((txq->nb_tx_desc - txq->nb_tx_free) > txq->tx_free_thresh)
> >>>>>>>      		ixgbe_tx_free_bufs(txq);
> >>>>>>>
> >>>>>>>      	nb_commit = nb_pkts = (uint16_t)RTE_MIN(txq->nb_tx_free, nb_pkts);
> >>>>>>>