Re: [RFC 0/9] get Rx and Tx used descriptors

[Bruce Richardson <bruce.richardson@intel.com>]

On Tue, Jan 17, 2017 at 09:24:10AM +0100, Olivier Matz wrote:
> Hi,
> 
> Thanks Bruce for the comments.
> 
> On Fri, 13 Jan 2017 17:32:38 +0000, "Richardson, Bruce"
> <bruce.richardson@intel.com> wrote:
> > > -----Original Message-----
> > > From: Olivier Matz [mailto:olivier.matz@6wind.com]
> > > Sent: Friday, January 13, 2017 4:44 PM
> > > To: dev@dpdk.org
> > > Cc: thomas.monjalon@6wind.com; Ananyev, Konstantin
> > > <konstantin.ananyev@intel.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>;
> > > Zhang, Helin <helin.zhang@intel.com>; Richardson, Bruce
> > > <bruce.richardson@intel.com>
> > > Subject: Re: [dpdk-dev] [RFC 0/9] get Rx and Tx used descriptors
> > > 
> > > Hi,
> > > 
> > > On Thu, 24 Nov 2016 10:54:12 +0100, Olivier Matz
> > > <olivier.matz@6wind.com> wrote:  
> > > > This RFC patchset introduces a new ethdev API function
> > > > rte_eth_tx_queue_count() which is the tx counterpart of
> > > > rte_eth_rx_queue_count(). It implements this API on some Intel
> > > > drivers for reference, and it also optimizes the implementation of
> > > > rte_eth_rx_queue_count().
> > > >  
> > > 
> > > I'm planning to send a new version of this patchset, fixing the
> > > issues seen by Ferruh, plus a bug fix in the e1000 implementation.
> > > 
> > > Does anyone have any comment about the new API or about questions
> > > raised in the cover letter? Especially about the real meaning of
> > > "used descriptor": should it include the descriptors hold by the
> > > driver?  
> > For TX, I think we just need used/unused, since for TX any driver
> > will reuse a slot that has been completed by the NIC, and doesn't
> > hold the mbufs back for buffering at all.
> 
> Agree
> 
> > For RX, strictly speaking, we should have three categories, rather
> > than trying to work it into 2. I don't see why we can't report a slot
> > as used/unused/unavailable.
> 
> With the rte_eth_rx_queue_count() API, we don't have this opportunity
> since it just returns an int.
> 
> Something I found a bit strange when doing this patchset is that the
> user does not have the full control of the number of hold buffers. With
> default parameters, the effective size of a ring of 128 is 64.
> 
> So it is, we could introduce an API to retrieve the status:
> used/unused/unavailable.
> 
> > > Any comment about the method (binary search to find the used
> > > descriptors)?  
> > 
> > I think binary search should work ok, though linear search may work
> > better for smaller ranges as we can prefetch ahead since we know what
> > we will check next. Linear can also go backward only if we want
> > accuracy (going forward risks having race conditions between read and
> > NIC write). Overall, though I think binary search should work well
> > enough.
> > 
> > > 
> > > I'm also wondering about adding rte_eth_tx_descriptor_done() in the
> > > API at the same time.
> > >   
> > 
> > Let me switch the question around - do we need the queue_count APIs at
> > all, and is it not more efficient to just supply the
> > descriptor_done() APIs? If an app wants to know the use of the ring,
> > and take some action based on it, that app is going to have one or
> > more thresholds for taking the action, right? In that case, rather
> > than scanning descriptors to find the absolute number of free/used
> > descriptors, it would be more efficient for the app to just check the
> > descriptor on the threshold - and take action based just on that
> > value. 
> 
> Yes, I reached the same conclusion (...after posting the RFC patchset
> unfortunatly).
> 
> > Any app that really does need the absolute value of the ring
> > capacity can presumably do its own binary search or linear search to
> > determine the value itself. However, I think just doing a done
> > function should encourage people to use the more efficient solution
> > of just checking the minimum number of descriptors needed.
> 
> 
> The question is: now that the work is done, is there any application
> that would require this absolute values? For instance, monitoring.
> 
> If not, I have no problem to the patchset, I just need to validate my
> application with a descriptor_done() API. In this case we can also
> deprecate rx_queue_count() and tx_queue_count().

I wouldn't have a problem with deprecating these functions.

> 
> The rte_eth_rx_descriptor_done() function could be updated into:
> 
> /**
>  * Check the status of a RX descriptor in the queue.
>  *
>  * @param port_id
>  *  The port identifier of the Ethernet device.
>  * @param queue_id
>  *  The queue id on the specific port.
>  * @param offset
>  *  The offset of the descriptor ID from tail (0 is the next packet to
>  *  be received by the driver).
>  *  - (2) Descriptor is unavailable (hold by driver, not yet returned to hw)
>  *  - (1) Descriptor is done (filled by hw, but not processed by the driver,
>  *        i.e. in the receive queue)
>  *  - (0) Descriptor is available for the hardware to receive a packet.
>  *  - (-ENODEV) if *port_id* invalid.
>  *  - (-ENOTSUP) if the device does not support this function
>  */
>  static inline int rte_eth_rx_descriptor_done(uint8_t port_id,
>  	uint16_t queue_id, uint16_t offset)
> 
> 
> A similar rte_eth_tx_descriptor_done() would be introduced:
> 
> /**
>  * Check the status of a TX descriptor in the queue.
>  *
>  * @param port_id
>  *  The port identifier of the Ethernet device.
>  * @param queue_id
>  *  The queue id on the specific port.
>  * @param offset
>  *  The offset of the descriptor ID from tail (0 is the place where the next
>  *  packet will be send).
>  *  - (1) Descriptor is beeing processed by the hw, i.e. in the transmit queue
>  *  - (0) Descriptor is available for the driver to send a packet.
>  *  - (-ENODEV) if *port_id* invalid.
>  *  - (-ENOTSUP) if the device does not support this function
>  */
>  static inline int rte_eth_tx_descriptor_done(uint8_t port_id,
>  	uint16_t queue_id, uint16_t offset)
> 
> 
> 
> An alternative would be to rename these functions in descriptor_status()
> instead of descriptor_done().

Seems good naming to me.

/Bruce