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

Message ID 1488388752-1819-2-git-send-email-olivier.matz@6wind.com (mailing list archive)
State Superseded, archived
Headers

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK

Commit Message

Olivier Matz March 1, 2017, 5:19 p.m. UTC
  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(+)
  

Comments

Andrew Rybchenko March 1, 2017, 6:22 p.m. UTC | #1
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?

> + *
> + * @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.

> + *  - (-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?
What should be returned if the queue is stopped?

> + */
> +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?

> +	return (*dev->dev_ops->rx_descriptor_status)(dev, queue_id, offset);
> +}
> +
> +#define RTE_ETH_TX_DESC_FULL 0 /**< Desc filled by pmd for hw, waiting xmit. */
> +#define RTE_ETH_TX_DESC_DONE 1 /**< Desc done, packet is transmitted. */

I see no value suitable for descriptor which is never used.

> +/**
> + * Check the status of a Tx descriptor in the queue.
> + *
> + * @param port_id
> + *  The port identifier of the Ethernet device.
> + * @param queue_id
> + *  The Tx queue identifier on this port.
> + * @param offset
> + *  The offset of the descriptor starting from tail (0 is the place where
> + *  the next packet will be send).
> + *
> + * @return
> + *  - (RTE_ETH_DESC_FULL) Descriptor is being processed by the hw, i.e.
> + *    in the transmit queue.
> + *  - (RTE_ETH_DESC_DONE) Hardware is done with this descriptor, it can be
> + *    reused by the driver.
> + *  - (-ENODEV) if *port_id* invalid.
> + *  - (-EINVAL) bad descriptor offset.
> + *  - (-ENOTSUP) if the device does not support this function.
> + */
> +static inline int rte_eth_tx_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->tx_descriptor_status, -ENOTSUP);
> +
> +	return (*dev->dev_ops->tx_descriptor_status)(dev, queue_id, offset);
> +}
> +
>   /**
>    * Send a burst of output packets on a transmit queue of an Ethernet device.
>    *
  
Olivier Matz March 2, 2017, 1:57 p.m. UTC | #2
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.



> 
> > + *
> > + * @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)


> 
> > + *  - (-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.


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


> 
> > + */
> > +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.


Olivier
  
Andrew Rybchenko March 2, 2017, 2:19 p.m. UTC | #3
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.
  
Olivier Matz March 2, 2017, 2:54 p.m. UTC | #4
> >>> + * 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.

Ok, I'll keep the first one at least, and see if I can reword the
second one to make it clear.

> >>> + *
> >>> + * @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.

Understood.

I can change _USED into _UNAVAIL (add it for Tx), with the following
description:

- (RTE_ETH_DESC_UNAVAIL): Descriptor is unavailable: either hold by driver
  and not yet returned to hw, or reserved by the hardware.
  
Andrew Rybchenko March 2, 2017, 3:05 p.m. UTC | #5
On 03/02/2017 05:54 PM, Olivier Matz wrote:
>>>>> + *
>>>>> + * @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.
> Understood.
>
> I can change _USED into _UNAVAIL (add it for Tx), with the following
> description:
>
> - (RTE_ETH_DESC_UNAVAIL): Descriptor is unavailable: either hold by driver
>    and not yet returned to hw, or reserved by the hardware.

Looks good. Do I understand correctly that it will be reported for 
descriptors which are not refilled (posted to HW) because of rx_free_thresh?
  
Olivier Matz March 2, 2017, 3:14 p.m. UTC | #6
On Thu, 2 Mar 2017 18:05:52 +0300, Andrew Rybchenko <arybchenko@solarflare.com> wrote:
> On 03/02/2017 05:54 PM, Olivier Matz wrote:
> >>>>> + *
> >>>>> + * @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.  
> > Understood.
> >
> > I can change _USED into _UNAVAIL (add it for Tx), with the following
> > description:
> >
> > - (RTE_ETH_DESC_UNAVAIL): Descriptor is unavailable: either hold by driver
> >    and not yet returned to hw, or reserved by the hardware.  
> 
> Looks good. Do I understand correctly that it will be reported for 
> descriptors which are not refilled (posted to HW) because of rx_free_thresh?
> 

Yes
  

Patch

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
+ *
+ * @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).
+ *  - (-ENODEV) if *port_id* invalid.
+ *  - (-EINVAL) bad descriptor offset.
+ *  - (-ENOTSUP) if the device does not support this function.
+ */
+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);
+
+	return (*dev->dev_ops->rx_descriptor_status)(dev, queue_id, offset);
+}
+
+#define RTE_ETH_TX_DESC_FULL 0 /**< Desc filled by pmd for hw, waiting xmit. */
+#define RTE_ETH_TX_DESC_DONE 1 /**< Desc done, packet is transmitted. */
+
+/**
+ * Check the status of a Tx descriptor in the queue.
+ *
+ * @param port_id
+ *  The port identifier of the Ethernet device.
+ * @param queue_id
+ *  The Tx queue identifier on this port.
+ * @param offset
+ *  The offset of the descriptor starting from tail (0 is the place where
+ *  the next packet will be send).
+ *
+ * @return
+ *  - (RTE_ETH_DESC_FULL) Descriptor is being processed by the hw, i.e.
+ *    in the transmit queue.
+ *  - (RTE_ETH_DESC_DONE) Hardware is done with this descriptor, it can be
+ *    reused by the driver.
+ *  - (-ENODEV) if *port_id* invalid.
+ *  - (-EINVAL) bad descriptor offset.
+ *  - (-ENOTSUP) if the device does not support this function.
+ */
+static inline int rte_eth_tx_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->tx_descriptor_status, -ENOTSUP);
+
+	return (*dev->dev_ops->tx_descriptor_status)(dev, queue_id, offset);
+}
+
 /**
  * Send a burst of output packets on a transmit queue of an Ethernet device.
  *