diff mbox

[dpdk-dev,4/4] ethdev: check support for rx_queue_count and descriptor_done fns

Message ID 1434108496-1993-5-git-send-email-bruce.richardson@intel.com (mailing list archive)
State Superseded, archived
Headers

Commit Message

Bruce Richardson June 12, 2015, 11:28 a.m. UTC 
  The functions rte_eth_rx_queue_count and rte_eth_descriptor_done are
supported by very few PMDs. Therefore, it is best to check for support
for the functions in the ethdev library, so as to avoid crashes
at run-time if the application goes to use those APIs. The performance
impact of this change should be very small as this is a predictable
branch in the function.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 lib/librte_ether/rte_ethdev.h | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

Comments

Roger Melton June 12, 2015, 5:32 p.m. UTC | #1 
Hi Bruce,  Comment in-line.  Regards, Roger

On 6/12/15 7:28 AM, Bruce Richardson wrote:
> The functions rte_eth_rx_queue_count and rte_eth_descriptor_done are
> supported by very few PMDs. Therefore, it is best to check for support
> for the functions in the ethdev library, so as to avoid crashes
> at run-time if the application goes to use those APIs. The performance
> impact of this change should be very small as this is a predictable
> branch in the function.
>
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> ---
>   lib/librte_ether/rte_ethdev.h | 8 ++++++--
>   1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
> index 827ca3e..9ad1b6a 100644
> --- a/lib/librte_ether/rte_ethdev.h
> +++ b/lib/librte_ether/rte_ethdev.h
> @@ -2496,6 +2496,8 @@ rte_eth_rx_burst(uint8_t port_id, uint16_t queue_id,
>    *  The queue id on the specific port.
>    * @return
>    *  The number of used descriptors in the specific queue.
> + *  NOTE: if function is not supported by device this call
> + *        returns (uint32_t)-ENOTSUP
>    */
>   static inline uint32_t

Why not change the return type to int32_t?
In this way, the caller isn't required to make the assumption that a 
large queue count indicates an error.  < 0 means error, other wise it's 
a valid queue count.

This approach would be consistent with other APIs.

>   rte_eth_rx_queue_count(uint8_t port_id, uint16_t queue_id)
> @@ -2507,8 +2509,9 @@ rte_eth_rx_queue_count(uint8_t port_id, uint16_t queue_id)
>   		RTE_PMD_DEBUG_TRACE("Invalid port_id=%d\n", port_id);
>   		return 0;
>   	}
> -	RTE_ETH_FPTR_OR_ERR_RET(*dev->dev_ops->rx_queue_count, 0);
>   #endif
> +	RTE_ETH_FPTR_OR_ERR_RET(*dev->dev_ops->rx_queue_count, (uint32_t)-ENOTSUP);
> +
>   	return (*dev->dev_ops->rx_queue_count)(dev, queue_id);
>   }
>   
> @@ -2525,6 +2528,7 @@ rte_eth_rx_queue_count(uint8_t port_id, uint16_t queue_id)
>    *  - (1) if the specific DD bit is set.
>    *  - (0) if the specific DD bit is not set.
>    *  - (-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)
> @@ -2536,8 +2540,8 @@ rte_eth_rx_descriptor_done(uint8_t port_id, uint16_t queue_id, uint16_t offset)
>   		RTE_PMD_DEBUG_TRACE("Invalid port_id=%d\n", port_id);
>   		return -ENODEV;
>   	}
> -	RTE_ETH_FPTR_OR_ERR_RET(*dev->dev_ops->rx_descriptor_done, -ENOTSUP);
>   #endif
> +	RTE_ETH_FPTR_OR_ERR_RET(*dev->dev_ops->rx_descriptor_done, -ENOTSUP);
>   
>   	return (*dev->dev_ops->rx_descriptor_done)( \
>   		dev->data->rx_queues[queue_id], offset);
Bruce Richardson June 15, 2015, 10:14 a.m. UTC | #2 
On Fri, Jun 12, 2015 at 01:32:56PM -0400, Roger B. Melton wrote:
> Hi Bruce,  Comment in-line.  Regards, Roger
> 
> On 6/12/15 7:28 AM, Bruce Richardson wrote:
> >The functions rte_eth_rx_queue_count and rte_eth_descriptor_done are
> >supported by very few PMDs. Therefore, it is best to check for support
> >for the functions in the ethdev library, so as to avoid crashes
> >at run-time if the application goes to use those APIs. The performance
> >impact of this change should be very small as this is a predictable
> >branch in the function.
> >
> >Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> >---
> >  lib/librte_ether/rte_ethdev.h | 8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> >
> >diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
> >index 827ca3e..9ad1b6a 100644
> >--- a/lib/librte_ether/rte_ethdev.h
> >+++ b/lib/librte_ether/rte_ethdev.h
> >@@ -2496,6 +2496,8 @@ rte_eth_rx_burst(uint8_t port_id, uint16_t queue_id,
> >   *  The queue id on the specific port.
> >   * @return
> >   *  The number of used descriptors in the specific queue.
> >+ *  NOTE: if function is not supported by device this call
> >+ *        returns (uint32_t)-ENOTSUP
> >   */
> >  static inline uint32_t
> 
> Why not change the return type to int32_t?
> In this way, the caller isn't required to make the assumption that a large
> queue count indicates an error.  < 0 means error, other wise it's a valid
> queue count.
> 
> This approach would be consistent with other APIs.
> 

Yes, good point, I should see about that. One thing I'm unsure of, though, is
does this count as ABI breakage? I don't see how it should break any older
apps, since the return type is the same size, but I'm not sure as we are 
changing the return type of the function.

Neil, can you perhaps comment here? Is changing uint32_t to int32_t ok, from
an ABI point of view?

Regards,
/Bruce
Thomas Monjalon July 6, 2015, 3:11 p.m. UTC | #3 
Neil, your ABI expertise is required for this patch.

2015-06-15 11:14, Bruce Richardson:
> On Fri, Jun 12, 2015 at 01:32:56PM -0400, Roger B. Melton wrote:
> > Hi Bruce,  Comment in-line.  Regards, Roger
> > 
> > On 6/12/15 7:28 AM, Bruce Richardson wrote:
> > >The functions rte_eth_rx_queue_count and rte_eth_descriptor_done are
> > >supported by very few PMDs. Therefore, it is best to check for support
> > >for the functions in the ethdev library, so as to avoid crashes
> > >at run-time if the application goes to use those APIs. The performance
> > >impact of this change should be very small as this is a predictable
> > >branch in the function.
> > >
> > >Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> > >---
> > >  lib/librte_ether/rte_ethdev.h | 8 ++++++--
> > >  1 file changed, 6 insertions(+), 2 deletions(-)
> > >
> > >diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
> > >index 827ca3e..9ad1b6a 100644
> > >--- a/lib/librte_ether/rte_ethdev.h
> > >+++ b/lib/librte_ether/rte_ethdev.h
> > >@@ -2496,6 +2496,8 @@ rte_eth_rx_burst(uint8_t port_id, uint16_t queue_id,
> > >   *  The queue id on the specific port.
> > >   * @return
> > >   *  The number of used descriptors in the specific queue.
> > >+ *  NOTE: if function is not supported by device this call
> > >+ *        returns (uint32_t)-ENOTSUP
> > >   */
> > >  static inline uint32_t
> > 
> > Why not change the return type to int32_t?
> > In this way, the caller isn't required to make the assumption that a large
> > queue count indicates an error.  < 0 means error, other wise it's a valid
> > queue count.
> > 
> > This approach would be consistent with other APIs.
> > 
> 
> Yes, good point, I should see about that. One thing I'm unsure of, though, is
> does this count as ABI breakage? I don't see how it should break any older
> apps, since the return type is the same size, but I'm not sure as we are 
> changing the return type of the function.
> 
> Neil, can you perhaps comment here? Is changing uint32_t to int32_t ok, from
> an ABI point of view?
> 
> Regards,
> /Bruce
Thomas Monjalon July 26, 2015, 8:44 p.m. UTC | #4 
Neil, Bruce,
Can we move forward?

2015-07-06 17:11, Thomas Monjalon:
> Neil, your ABI expertise is required for this patch.
> 
> 2015-06-15 11:14, Bruce Richardson:
> > On Fri, Jun 12, 2015 at 01:32:56PM -0400, Roger B. Melton wrote:
> > > Hi Bruce,  Comment in-line.  Regards, Roger
> > > 
> > > On 6/12/15 7:28 AM, Bruce Richardson wrote:
> > > >The functions rte_eth_rx_queue_count and rte_eth_descriptor_done are
> > > >supported by very few PMDs. Therefore, it is best to check for support
> > > >for the functions in the ethdev library, so as to avoid crashes
> > > >at run-time if the application goes to use those APIs. The performance
> > > >impact of this change should be very small as this is a predictable
> > > >branch in the function.
> > > >
> > > >Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> > > >---
> > > >  lib/librte_ether/rte_ethdev.h | 8 ++++++--
> > > >  1 file changed, 6 insertions(+), 2 deletions(-)
> > > >
> > > >diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
> > > >index 827ca3e..9ad1b6a 100644
> > > >--- a/lib/librte_ether/rte_ethdev.h
> > > >+++ b/lib/librte_ether/rte_ethdev.h
> > > >@@ -2496,6 +2496,8 @@ rte_eth_rx_burst(uint8_t port_id, uint16_t queue_id,
> > > >   *  The queue id on the specific port.
> > > >   * @return
> > > >   *  The number of used descriptors in the specific queue.
> > > >+ *  NOTE: if function is not supported by device this call
> > > >+ *        returns (uint32_t)-ENOTSUP
> > > >   */
> > > >  static inline uint32_t
> > > 
> > > Why not change the return type to int32_t?
> > > In this way, the caller isn't required to make the assumption that a large
> > > queue count indicates an error.  < 0 means error, other wise it's a valid
> > > queue count.
> > > 
> > > This approach would be consistent with other APIs.
> > > 
> > 
> > Yes, good point, I should see about that. One thing I'm unsure of, though, is
> > does this count as ABI breakage? I don't see how it should break any older
> > apps, since the return type is the same size, but I'm not sure as we are 
> > changing the return type of the function.
> > 
> > Neil, can you perhaps comment here? Is changing uint32_t to int32_t ok, from
> > an ABI point of view?
> > 
> > Regards,
> > /Bruce
> 
>
diff mbox

Patch

diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
index 827ca3e..9ad1b6a 100644
--- a/lib/librte_ether/rte_ethdev.h
+++ b/lib/librte_ether/rte_ethdev.h
@@ -2496,6 +2496,8 @@  rte_eth_rx_burst(uint8_t port_id, uint16_t queue_id,
  *  The queue id on the specific port.
  * @return
  *  The number of used descriptors in the specific queue.
+ *  NOTE: if function is not supported by device this call
+ *        returns (uint32_t)-ENOTSUP
  */
 static inline uint32_t
 rte_eth_rx_queue_count(uint8_t port_id, uint16_t queue_id)
@@ -2507,8 +2509,9 @@  rte_eth_rx_queue_count(uint8_t port_id, uint16_t queue_id)
 		RTE_PMD_DEBUG_TRACE("Invalid port_id=%d\n", port_id);
 		return 0;
 	}
-	RTE_ETH_FPTR_OR_ERR_RET(*dev->dev_ops->rx_queue_count, 0);
 #endif
+	RTE_ETH_FPTR_OR_ERR_RET(*dev->dev_ops->rx_queue_count, (uint32_t)-ENOTSUP);
+
 	return (*dev->dev_ops->rx_queue_count)(dev, queue_id);
 }
 
@@ -2525,6 +2528,7 @@  rte_eth_rx_queue_count(uint8_t port_id, uint16_t queue_id)
  *  - (1) if the specific DD bit is set.
  *  - (0) if the specific DD bit is not set.
  *  - (-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)
@@ -2536,8 +2540,8 @@  rte_eth_rx_descriptor_done(uint8_t port_id, uint16_t queue_id, uint16_t offset)
 		RTE_PMD_DEBUG_TRACE("Invalid port_id=%d\n", port_id);
 		return -ENODEV;
 	}
-	RTE_ETH_FPTR_OR_ERR_RET(*dev->dev_ops->rx_descriptor_done, -ENOTSUP);
 #endif
+	RTE_ETH_FPTR_OR_ERR_RET(*dev->dev_ops->rx_descriptor_done, -ENOTSUP);
 
 	return (*dev->dev_ops->rx_descriptor_done)( \
 		dev->data->rx_queues[queue_id], offset);