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.richard...@intel.com> wrote: > > > -----Original Message----- > > > From: Olivier Matz [mailto:olivier.m...@6wind.com] > > > Sent: Friday, January 13, 2017 4:44 PM > > > To: dev@dpdk.org > > > Cc: thomas.monja...@6wind.com; Ananyev, Konstantin > > > <konstantin.anan...@intel.com>; Lu, Wenzhuo <wenzhuo...@intel.com>; > > > Zhang, Helin <helin.zh...@intel.com>; Richardson, Bruce > > > <bruce.richard...@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.m...@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
