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

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

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.
Indeed that fix, will cause more often unsuccessful checks for DD bits and might cause a
slowdown for TX fast-path.  
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.
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.
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.

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