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"
[off-list ref] wrote:

quoted

quoted

-----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
[off-list ref]; Lu, Wenzhuo [off-list ref];
Zhang, Helin [off-list ref]; Richardson, Bruce
[off-list ref]
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
[off-list ref] wrote:  

quoted

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

quoted

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.

quoted

quoted

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.

quoted

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

quoted

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