Re: [PATCH v7 1/3] ethdev: new API to free consumed buffers in Tx ring

[Olivier Matz <olivier.matz@6wind.com>]

On Fri, 24 Mar 2017 09:18:54 -0400, Billy McFall <bmcfall@redhat.com> wrote:
> On Fri, Mar 24, 2017 at 8:46 AM, Olivier Matz <olivier.matz@6wind.com>
> wrote:

[...]

> > > I read through this patch. This API doesn't indicate if the packet was  
> > > transmitted or dropped (I think that is what you were asking). This API
> > > could be used by the application to determine if the mbuf has been
> > > freed, as opposed to polling the rte_mbuf_refcnt_read() for a change
> > > in value. Did I miss your point?  
> >
> > Maybe my question was not clear :)
> > Let me try to reword it.
> >
> > For a traffic generator use-case, a dummy algorithm may be:
> >
> > 1/ send packets in a loop until a condition is met (ex: packet count
> > reached)
> > 2/ call rte_eth_tx_done_cleanup()
> > 3/ read stats for report
> >
> > I think there is something missing between 1/ and 2/, to ensure that
> > all packets that were in the tx queue are processed (either transmitted
> > or dropped). If that's not the case, both steps 2/ and 3/ will not
> > behave as expected:
> > - all mbufs won't be returned to the pool
> > - statistics may be wrong
> >
> > Maybe a simple wait() could do the job.
> > Using a combination of rte_eth_tx_done_cleanup() +
> > rte_eth_tx_descriptor_status()
> > is probably also a solution.
> >
> > Do you confirm rte_eth_tx_done_cleanup() does not check that?
> >
> Confirm.  rte_eth_tx_done_cleanup() does not check that. In the flooding  
> case,
> the applications is expected to poll rte_eth_tx_done_cleanup() until some
> condition
> is met, like ref_count of given packet is decremented. So on the packetGen
> case, the
> application would need to wait some time and/or call
> rte_eth_tx_descriptor_status()
> as you suggested.
> 
> My original patch returned RTE_DONE (no more packets pending),
> RTE_PROCESSING (freed what I could but there are still packets in the queue)
> or -ERRNO for error. Then packets freed count was returned via a pointer in
> the param list.
> That would have solved what you are asking, but that was shot down as being
> overkill.
> 
> Should I add another sentence to the packet generator bullet indicating
> that it is the
> application's job to make sure no more packets are pending? Like:
> 
>   In this case, it can call the ``rte_eth_tx_done_cleanup()`` API for each
> destination interface it has been using
>   to request it to release of all its used mbufs.
> + It is the application's responsibility to ensure all packets have been
> processed by the destination interface.
> + Use rte_eth_tx_descriptor_status() to obtain the status of the transmit
> queue,

Thanks for the clarification.
Not sure the sentence is required, since rte_eth_tx_descriptor_status()
is not included yet.

Regards,
Olivier