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

[Zoltan Kiss <zoltan.kiss@linaro.org>]

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.

> 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.
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);
>>>