Re: [dpdk-dev] [PATCH 1/6] ethdev: add descriptor status API

[Andrew Rybchenko <arybchenko@solarflare.com>]

Hi Olivier,

On 03/02/2017 04:57 PM, Olivier Matz wrote:
> Hi Andrew,
>
> Thank you for the review. Comments inline.
>
> On Wed, 1 Mar 2017 21:22:14 +0300, Andrew Rybchenko <arybchenko@solarflare.com> wrote:
>> On 03/01/2017 08:19 PM, Olivier Matz wrote:
>>> Introduce a new API to get the status of a descriptor.
>>>
>>> For Rx, it is almost similar to rx_descriptor_done API, except it
>>> differentiates "used" descriptors (which are hold by the driver and not
>>> returned to the hardware).
>>>
>>> For Tx, it is a new API.
>>>
>>> The descriptor_done() API, and probably the rx_queue_count() API could
>>> be replaced by this new API as soon as it is implemented on all PMDs.
>>>
>>> Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
>>> ---
>>>    lib/librte_ether/rte_ethdev.h | 86 +++++++++++++++++++++++++++++++++++++++++++
>>>    1 file changed, 86 insertions(+)
>>>
>>> diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
>>> index 97f3e2d..9ac9c61 100644
>>> --- a/lib/librte_ether/rte_ethdev.h
>>> +++ b/lib/librte_ether/rte_ethdev.h
>>> @@ -1179,6 +1179,14 @@ typedef uint32_t (*eth_rx_queue_count_t)(struct rte_eth_dev *dev,
>>>    typedef int (*eth_rx_descriptor_done_t)(void *rxq, uint16_t offset);
>>>    /**< @internal Check DD bit of specific RX descriptor */
>>>    
>>> +typedef int (*eth_rx_descriptor_status_t)(struct rte_eth_dev *dev,
>>> +	uint16_t rx_queue_id, uint16_t offset);
>>> +/**< @internal Check the status of a Rx descriptor */
>>> +
>>> +typedef int (*eth_tx_descriptor_status_t)(struct rte_eth_dev *dev,
>>> +	uint16_t tx_queue_id, uint16_t offset);
>>> +/**< @internal Check the status of a Tx descriptor */
>>> +
>>>    typedef int (*eth_fw_version_get_t)(struct rte_eth_dev *dev,
>>>    				     char *fw_version, size_t fw_size);
>>>    /**< @internal Get firmware information of an Ethernet device. */
>>> @@ -1483,6 +1491,10 @@ struct eth_dev_ops {
>>>    	eth_queue_release_t        rx_queue_release; /**< Release RX queue. */
>>>    	eth_rx_queue_count_t       rx_queue_count;/**< Get Rx queue count. */
>>>    	eth_rx_descriptor_done_t   rx_descriptor_done; /**< Check rxd DD bit. */
>>> +	eth_rx_descriptor_status_t rx_descriptor_status;
>>> +	/**< Check the status of a Rx descriptor. */
>>> +	eth_tx_descriptor_status_t tx_descriptor_status;
>>> +	/**< Check the status of a Tx descriptor. */
>>>    	eth_rx_enable_intr_t       rx_queue_intr_enable;  /**< Enable Rx queue interrupt. */
>>>    	eth_rx_disable_intr_t      rx_queue_intr_disable; /**< Disable Rx queue interrupt. */
>>>    	eth_tx_queue_setup_t       tx_queue_setup;/**< Set up device TX queue. */
>>> @@ -2768,6 +2780,80 @@ rte_eth_rx_descriptor_done(uint8_t port_id, uint16_t queue_id, uint16_t offset)
>>>    		dev->data->rx_queues[queue_id], offset);
>>>    }
>>>    
>>> +#define RTE_ETH_RX_DESC_AVAIL 0 /**< Desc available for hw. */
>>> +#define RTE_ETH_RX_DESC_DONE  1 /**< Desc done, filled by hw. */
>>> +#define RTE_ETH_RX_DESC_USED  2 /**< Desc used by driver. */
>>> +
>>> +/**
>>> + * Check the status of a Rx descriptor in the queue
>> I think it would be useful to highlight caller context.
>> Should it be the same CPU which receives packets from the queue?
> Yes, you are right it would be useful. I suggest the following sentences:
>
>    This function should be called on a dataplane core like the
>    Rx function. They should not be called concurrently on the same
>    queue.

The first sentence looks fine. "They" (functions?, dataplane cores?) is 
unclear for me in the second. May be the first one is simply sufficient.

>>> + *
>>> + * @param port_id
>>> + *  The port identifier of the Ethernet device.
>>> + * @param queue_id
>>> + *  The Rx queue identifier on this port.
>>> + * @param offset
>>> + *  The offset of the descriptor starting from tail (0 is the next
>>> + *  packet to be received by the driver).
>>> + * @return
>>> + *  - (RTE_ETH_DESC_AVAIL): Descriptor is available for the hardware to
>>> + *    receive a packet.
>>> + *  - (RTE_ETH_DESC_DONE): Descriptor is done, it is filled by hw, but
>>> + *    not yet processed by the driver (i.e. in the receive queue).
>>> + *  - (RTE_ETH_DESC_USED): Descriptor is unavailable (hold by driver,
>>> + *    not yet returned to hw).
>> It looks like it is the most suitable for descriptors which are reserved
>> and never used.
> Can you give some more details about what is a reserved but never
> used descriptor? (same question for Tx)

Our HW has a requirement to keep few descriptors always unused (i.e. 
some gap between tail and head). It is just a few descriptors, but 
invalid descriptor status may misguide application. E.g. if Rx queue 
size is 512 and offset 510, it will always be unused (since it is 
reserved). It is not an indication that core is too slow and can't keep 
the pace.

>>> + *  - (-ENODEV) if *port_id* invalid.
>>> + *  - (-EINVAL) bad descriptor offset.
>>> + *  - (-ENOTSUP) if the device does not support this function.
>> What should be returned if queue_id is invalid?
> I'd say -ENODEV too. On the other hand, adding these checks is
> maybe not a good idea as we are in dataplane.
>
> The previous API rx_descriptor_done() API was taking the queue
> pointer as parameter, like Rx/Tx functions. It's probably a better
> idea.

I think so too since Rx burst callback (used nearby as above descriptor 
says) gets queue pointer.

>> What should be returned if the queue is stopped?
> For the same performance reasons, I think we should just highlight
> in the API that this dataplane function should not be called on a
> stopped queue.

OK.

>>> + */
>>> +static inline int
>>> +rte_eth_rx_descriptor_status(uint8_t port_id, uint16_t queue_id,
>>> +	uint16_t offset)
>>> +{
>>> +	struct rte_eth_dev *dev;
>>> +
>>> +	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>>> +	dev = &rte_eth_devices[port_id];
>>> +	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->rx_descriptor_status, -ENOTSUP);
>>> +
>> May be it makes sense to range check queue_id here to avoid such code in
>> each PMD?
> If we keep this API, yes. If we switch to a queue pointer as proposed
> above, we will assume (and highlight in the API doc) that the pointer
> must be valid, like for Rx/Tx funcs.

I've simply seen patches which add the queue id range check in the 
generic place.
But I think switching to queue pointer is a better idea.

Thanks,
Andrew.