[dpdk-dev] eventdev: Add rte_errno return values to the enqueue and dequeue functions
Checks
Commit Message
This change allows user software to differentiate between an invalid argument
(such as an invalid queue_id or sched_type in an enqueued event) and
backpressure from the event device.
The port and device ID checks are placed in RTE_LIBRTE_EVENTDEV_DEBUG header
guards to avoid the performance hit in non-debug execution.
Signed-off-by: Gage Eads <gage.eads@intel.com>
---
lib/librte_eventdev/rte_eventdev.h | 42 +++++++++++++++++++++++++++++++++++---
1 file changed, 39 insertions(+), 3 deletions(-)
Comments
On Fri, Feb 10, 2017 at 03:02:21PM -0600, Gage Eads wrote:
> This change allows user software to differentiate between an invalid argument
> (such as an invalid queue_id or sched_type in an enqueued event) and
> backpressure from the event device.
>
> The port and device ID checks are placed in RTE_LIBRTE_EVENTDEV_DEBUG header
> guards to avoid the performance hit in non-debug execution.
>
> Signed-off-by: Gage Eads <gage.eads@intel.com>
> ---
Do we have some idea of the performance hit from these? It may be too
soon to know, given we don't have many drivers to test with, but if
there is no perf hit seen with the SW driver, I think we should look to
just always do this, rather than having it compile-time off. If it does
prove to be a performance problem we can look to #ifdef it out later.
/Bruce
On Mon, Feb 13, 2017 at 10:38:55AM +0000, Bruce Richardson wrote:
> On Fri, Feb 10, 2017 at 03:02:21PM -0600, Gage Eads wrote:
> > This change allows user software to differentiate between an invalid argument
> > (such as an invalid queue_id or sched_type in an enqueued event) and
> > backpressure from the event device.
> >
> > The port and device ID checks are placed in RTE_LIBRTE_EVENTDEV_DEBUG header
> > guards to avoid the performance hit in non-debug execution.
> >
> > Signed-off-by: Gage Eads <gage.eads@intel.com>
> > ---
>
> Do we have some idea of the performance hit from these? It may be too
> soon to know, given we don't have many drivers to test with, but if
> there is no perf hit seen with the SW driver, I think we should look to
> just always do this, rather than having it compile-time off. If it does
IMO, It is better put to under compile-time like ethdev. It is
difficult predict the performance regression on wide range of cores that DPDK
runs now. I think we need to add following additional checks based on
Gage header file change
1) Per event queue ID is valid or not?
2) Per event's sched type doesn't match the capabilities of the destination queue.
> prove to be a performance problem we can look to #ifdef it out later.
>
> /Bruce
On Mon, Feb 13, 2017 at 05:18:11PM +0530, Jerin Jacob wrote:
> On Mon, Feb 13, 2017 at 10:38:55AM +0000, Bruce Richardson wrote:
> > On Fri, Feb 10, 2017 at 03:02:21PM -0600, Gage Eads wrote:
> > > This change allows user software to differentiate between an invalid argument
> > > (such as an invalid queue_id or sched_type in an enqueued event) and
> > > backpressure from the event device.
> > >
> > > The port and device ID checks are placed in RTE_LIBRTE_EVENTDEV_DEBUG header
> > > guards to avoid the performance hit in non-debug execution.
> > >
> > > Signed-off-by: Gage Eads <gage.eads@intel.com>
> > > ---
> >
> > Do we have some idea of the performance hit from these? It may be too
> > soon to know, given we don't have many drivers to test with, but if
> > there is no perf hit seen with the SW driver, I think we should look to
> > just always do this, rather than having it compile-time off. If it does
>
> IMO, It is better put to under compile-time like ethdev. It is
> difficult predict the performance regression on wide range of cores that DPDK
> runs now. I think we need to add following additional checks based on
> Gage header file change
>
> 1) Per event queue ID is valid or not?
> 2) Per event's sched type doesn't match the capabilities of the destination queue.
>
Ok, if we are expanding the number of checks then I definitely think it
needs to be compile-time selected.
/Bruce
>
> > prove to be a performance problem we can look to #ifdef it out later.
> >
> > /Bruce
> -----Original Message-----
> From: Richardson, Bruce
> Sent: Monday, February 13, 2017 6:08 AM
> To: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> Cc: Eads, Gage <gage.eads@intel.com>; dev@dpdk.org;
> hemant.agrawal@nxp.com; Van Haaren, Harry <harry.van.haaren@intel.com>;
> nipun.gupta@nxp.com
> Subject: Re: [PATCH] eventdev: Add rte_errno return values to the enqueue and
> dequeue functions
>
> On Mon, Feb 13, 2017 at 05:18:11PM +0530, Jerin Jacob wrote:
> > On Mon, Feb 13, 2017 at 10:38:55AM +0000, Bruce Richardson wrote:
> > > On Fri, Feb 10, 2017 at 03:02:21PM -0600, Gage Eads wrote:
> > > > This change allows user software to differentiate between an
> > > > invalid argument (such as an invalid queue_id or sched_type in an
> > > > enqueued event) and backpressure from the event device.
> > > >
> > > > The port and device ID checks are placed in
> > > > RTE_LIBRTE_EVENTDEV_DEBUG header guards to avoid the performance
> hit in non-debug execution.
> > > >
> > > > Signed-off-by: Gage Eads <gage.eads@intel.com>
> > > > ---
> > >
> > > Do we have some idea of the performance hit from these? It may be
> > > too soon to know, given we don't have many drivers to test with, but
> > > if there is no perf hit seen with the SW driver, I think we should
> > > look to just always do this, rather than having it compile-time off.
> > > If it does
> >
> > IMO, It is better put to under compile-time like ethdev. It is
> > difficult predict the performance regression on wide range of cores
> > that DPDK runs now. I think we need to add following additional checks
> > based on Gage header file change
> >
> > 1) Per event queue ID is valid or not?
> > 2) Per event's sched type doesn't match the capabilities of the destination
> queue.
> >
>
> Ok, if we are expanding the number of checks then I definitely think it needs to
> be compile-time selected.
My thinking is that, unlike checking the dev ID or port ID, per-event checks should be in the PMD itself since (presumably) it will loop over the events anyway. I also think it makes sense for the per-event error checking not to be compile-time selectable because a PMD needs to handle the invalid queue ID or sched_type case anyway.
>
> /Bruce
> >
> > > prove to be a performance problem we can look to #ifdef it out later.
> > >
> > > /Bruce
On Fri, Feb 10, 2017 at 03:02:21PM -0600, Gage Eads wrote:
> This change allows user software to differentiate between an invalid argument
> (such as an invalid queue_id or sched_type in an enqueued event) and
> backpressure from the event device.
>
> The port and device ID checks are placed in RTE_LIBRTE_EVENTDEV_DEBUG header
> guards to avoid the performance hit in non-debug execution.
>
> Signed-off-by: Gage Eads <gage.eads@intel.com>
> ---
> static inline uint16_t
> @@ -1127,6 +1133,21 @@ rte_event_enqueue_burst(uint8_t dev_id, uint8_t port_id,
> {
> struct rte_eventdev *dev = &rte_eventdevs[dev_id];
>
> + rte_errno = 0;
I don't think it is required. If at all required, move this under
RTE_LIBRTE_EVENTDEV_DEBUG to save store to rte_errno cycles on fastpath
> +#ifdef RTE_LIBRTE_EVENTDEV_DEBUG
> + if (rte_eventdevs[dev_id].attached == RTE_EVENTDEV_DETACHED) {
> + RTE_EDEV_LOG_DEBUG("Invalid dev_id=%d\n", dev_id);
> + rte_errno = -EINVAL;
> + return 0;
> + }
> +
> + if (port_id >= dev->data->nb_ports) {
> + RTE_EDEV_LOG_DEBUG("Invalid port_id=%d\n", port_id);
> + rte_errno = -EINVAL;
> + return 0;
> + }
> +#endif
> +
> /*
> * Allow zero cost non burst mode routine invocation if application
> * requests nb_events as const one
> @@ -1235,6 +1256,21 @@ rte_event_dequeue_burst(uint8_t dev_id, uint8_t port_id, struct rte_event ev[],
> {
> struct rte_eventdev *dev = &rte_eventdevs[dev_id];
>
> +#ifdef RTE_LIBRTE_EVENTDEV_DEBUG
> + rte_errno = 0;
> + if (rte_eventdevs[dev_id].attached == RTE_EVENTDEV_DETACHED) {
> + RTE_EDEV_LOG_DEBUG("Invalid dev_id=%d\n", dev_id);
> + rte_errno = -EINVAL;
> + return 0;
> + }
> +
> + if (port_id >= dev->data->nb_ports) {
> + RTE_EDEV_LOG_DEBUG("Invalid port_id=%d\n", port_id);
> + rte_errno = -EINVAL;
> + return 0;
> + }
> +#endif
> +
> /*
> * Allow zero cost non burst mode routine invocation if application
> * requests nb_events as const one
> --
> 2.7.4
>
> -----Original Message-----
> From: Jerin Jacob [mailto:jerin.jacob@caviumnetworks.com]
> Sent: Monday, February 13, 2017 10:10 PM
> To: Eads, Gage <gage.eads@intel.com>
> Cc: dev@dpdk.org; Richardson, Bruce <bruce.richardson@intel.com>;
> hemant.agrawal@nxp.com; Van Haaren, Harry <harry.van.haaren@intel.com>;
> nipun.gupta@nxp.com
> Subject: Re: [PATCH] eventdev: Add rte_errno return values to the enqueue and
> dequeue functions
>
> On Fri, Feb 10, 2017 at 03:02:21PM -0600, Gage Eads wrote:
> > This change allows user software to differentiate between an invalid
> > argument (such as an invalid queue_id or sched_type in an enqueued
> > event) and backpressure from the event device.
> >
> > The port and device ID checks are placed in RTE_LIBRTE_EVENTDEV_DEBUG
> > header guards to avoid the performance hit in non-debug execution.
> >
> > Signed-off-by: Gage Eads <gage.eads@intel.com>
> > ---
> > static inline uint16_t
> > @@ -1127,6 +1133,21 @@ rte_event_enqueue_burst(uint8_t dev_id, uint8_t
> > port_id, {
> > struct rte_eventdev *dev = &rte_eventdevs[dev_id];
> >
> > + rte_errno = 0;
>
> I don't think it is required. If at all required, move this under
> RTE_LIBRTE_EVENTDEV_DEBUG to save store to rte_errno cycles on fastpath
Agreed -- if we encounter an error we should set rte_errno, otherwise the return value will equal nb_events and the user shouldn't check it.
Will fix in v2.
>
> > +#ifdef RTE_LIBRTE_EVENTDEV_DEBUG
> > + if (rte_eventdevs[dev_id].attached == RTE_EVENTDEV_DETACHED) {
> > + RTE_EDEV_LOG_DEBUG("Invalid dev_id=%d\n", dev_id);
> > + rte_errno = -EINVAL;
> > + return 0;
> > + }
> > +
> > + if (port_id >= dev->data->nb_ports) {
> > + RTE_EDEV_LOG_DEBUG("Invalid port_id=%d\n", port_id);
> > + rte_errno = -EINVAL;
> > + return 0;
> > + }
> > +#endif
> > +
> > /*
> > * Allow zero cost non burst mode routine invocation if application
> > * requests nb_events as const one
> > @@ -1235,6 +1256,21 @@ rte_event_dequeue_burst(uint8_t dev_id, uint8_t
> > port_id, struct rte_event ev[], {
> > struct rte_eventdev *dev = &rte_eventdevs[dev_id];
> >
> > +#ifdef RTE_LIBRTE_EVENTDEV_DEBUG
> > + rte_errno = 0;
> > + if (rte_eventdevs[dev_id].attached == RTE_EVENTDEV_DETACHED) {
> > + RTE_EDEV_LOG_DEBUG("Invalid dev_id=%d\n", dev_id);
> > + rte_errno = -EINVAL;
> > + return 0;
> > + }
> > +
> > + if (port_id >= dev->data->nb_ports) {
> > + RTE_EDEV_LOG_DEBUG("Invalid port_id=%d\n", port_id);
> > + rte_errno = -EINVAL;
> > + return 0;
> > + }
> > +#endif
> > +
> > /*
> > * Allow zero cost non burst mode routine invocation if application
> > * requests nb_events as const one
> > --
> > 2.7.4
> >
@@ -245,6 +245,7 @@ extern "C" {
#include <rte_common.h>
#include <rte_memory.h>
+#include <rte_errno.h>
struct rte_mbuf; /* we just use mbuf pointers; no need to include rte_mbuf.h */
@@ -1116,9 +1117,14 @@ rte_event_schedule(uint8_t dev_id)
* The number of event objects actually enqueued on the event device. The
* return value can be less than the value of the *nb_events* parameter when
* the event devices queue is full or if invalid parameters are specified in a
- * *rte_event*. If return value is less than *nb_events*, the remaining events
- * at the end of ev[] are not consumed,and the caller has to take care of them
- *
+ * *rte_event*. If the return value is less than *nb_events*, the remaining
+ * events at the end of ev[] are not consumed and the caller has to take care
+ * of them, and rte_errno is set accordingly. Possible errno values include:
+ * -(-EINVAL) The port ID is invalid, device ID is invalid, an event's queue
+ * ID is invalid, or an event's sched type doesn't match the
+ * capabilities of the destination queue.
+ * -(-ENOSPC) The event port was backpressured and unable to enqueue
+ * one or more events.
* @see rte_event_port_enqueue_depth()
*/
static inline uint16_t
@@ -1127,6 +1133,21 @@ rte_event_enqueue_burst(uint8_t dev_id, uint8_t port_id,
{
struct rte_eventdev *dev = &rte_eventdevs[dev_id];
+ rte_errno = 0;
+#ifdef RTE_LIBRTE_EVENTDEV_DEBUG
+ if (rte_eventdevs[dev_id].attached == RTE_EVENTDEV_DETACHED) {
+ RTE_EDEV_LOG_DEBUG("Invalid dev_id=%d\n", dev_id);
+ rte_errno = -EINVAL;
+ return 0;
+ }
+
+ if (port_id >= dev->data->nb_ports) {
+ RTE_EDEV_LOG_DEBUG("Invalid port_id=%d\n", port_id);
+ rte_errno = -EINVAL;
+ return 0;
+ }
+#endif
+
/*
* Allow zero cost non burst mode routine invocation if application
* requests nb_events as const one
@@ -1235,6 +1256,21 @@ rte_event_dequeue_burst(uint8_t dev_id, uint8_t port_id, struct rte_event ev[],
{
struct rte_eventdev *dev = &rte_eventdevs[dev_id];
+#ifdef RTE_LIBRTE_EVENTDEV_DEBUG
+ rte_errno = 0;
+ if (rte_eventdevs[dev_id].attached == RTE_EVENTDEV_DETACHED) {
+ RTE_EDEV_LOG_DEBUG("Invalid dev_id=%d\n", dev_id);
+ rte_errno = -EINVAL;
+ return 0;
+ }
+
+ if (port_id >= dev->data->nb_ports) {
+ RTE_EDEV_LOG_DEBUG("Invalid port_id=%d\n", port_id);
+ rte_errno = -EINVAL;
+ return 0;
+ }
+#endif
+
/*
* Allow zero cost non burst mode routine invocation if application
* requests nb_events as const one