All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] eventdev: add producer enqueue hint
@ 2017-06-12 11:46 Jerin Jacob
  2017-06-26 15:44 ` Eads, Gage
  0 siblings, 1 reply; 5+ messages in thread
From: Jerin Jacob @ 2017-06-12 11:46 UTC (permalink / raw)
  To: dev
  Cc: bruce.richardson, harry.van.haaren, hemant.agrawal, gage.eads,
	nipun.gupta, narender.vangati, nikhil.rao, Jerin Jacob

Some PMD like OCTEONTX can have optimized handling of
events if the PMD knows it is a producer pattern in advance.
For instance, OCTEONTX PMD can support burst mode if
op == RTE_EVENT_OP_NEW.

Since the event producer initialize(set all_op_new == 1) the event
object before the main producer loop, This scheme does not call for
any performance regression on other PMDs.

Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
---
Another option is to add a flag in enqueue API or have parallel enqueue API.
---
 drivers/event/octeontx/ssovf_worker.c | 12 ++++++++++--
 lib/librte_eventdev/rte_eventdev.h    | 10 +++++++++-
 2 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/drivers/event/octeontx/ssovf_worker.c b/drivers/event/octeontx/ssovf_worker.c
index ad3fe684d..209c595cf 100644
--- a/drivers/event/octeontx/ssovf_worker.c
+++ b/drivers/event/octeontx/ssovf_worker.c
@@ -196,8 +196,16 @@ ssows_enq(void *port, const struct rte_event *ev)
 uint16_t __hot
 ssows_enq_burst(void *port, const struct rte_event ev[], uint16_t nb_events)
 {
-	RTE_SET_USED(nb_events);
-	return ssows_enq(port, ev);
+	uint16_t i;
+	struct ssows *ws = port;
+
+	if (ev[0].all_op_new) {
+		rte_smp_wmb();
+		for (i = 0; i < nb_events; i++)
+			ssows_new_event(ws, &ev[i]);
+		return i;
+	} else
+		return ssows_enq(port, ev);
 }
 
 void
diff --git a/lib/librte_eventdev/rte_eventdev.h b/lib/librte_eventdev/rte_eventdev.h
index a248fe90e..1c1a46593 100644
--- a/lib/librte_eventdev/rte_eventdev.h
+++ b/lib/librte_eventdev/rte_eventdev.h
@@ -933,7 +933,15 @@ struct rte_event {
 			 * and is undefined on dequeue.
 			 * @see RTE_EVENT_OP_NEW, (RTE_EVENT_OP_*)
 			 */
-			uint8_t rsvd:4;
+			uint8_t all_op_new:1;
+			/**< Valid only with event enqueue operation - This hint
+			 * indicates that the enqueue request has only the
+			 * events with op == RTE_EVENT_OP_NEW.
+			 * The event producer, typically use this pattern to
+			 * inject the events to eventdev.
+			 * @see RTE_EVENT_OP_NEW rte_event_enqueue_burst()
+			 */
+			uint8_t rsvd:3;
 			/**< Reserved for future use */
 			uint8_t sched_type:2;
 			/**< Scheduler synchronization type (RTE_SCHED_TYPE_*)
-- 
2.13.1

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH] eventdev: add producer enqueue hint
  2017-06-12 11:46 [PATCH] eventdev: add producer enqueue hint Jerin Jacob
@ 2017-06-26 15:44 ` Eads, Gage
  2017-06-27  8:08   ` Jerin Jacob
  0 siblings, 1 reply; 5+ messages in thread
From: Eads, Gage @ 2017-06-26 15:44 UTC (permalink / raw)
  To: Jerin Jacob, dev
  Cc: Richardson, Bruce, Van Haaren, Harry, hemant.agrawal,
	nipun.gupta, Vangati, Narender, Rao, Nikhil



> -----Original Message-----
> From: Jerin Jacob [mailto:jerin.jacob@caviumnetworks.com]
> Sent: Monday, June 12, 2017 6:46 AM
> To: dev@dpdk.org
> Cc: Richardson, Bruce <bruce.richardson@intel.com>; Van Haaren, Harry
> <harry.van.haaren@intel.com>; hemant.agrawal@nxp.com; Eads, Gage
> <gage.eads@intel.com>; nipun.gupta@nxp.com; Vangati, Narender
> <narender.vangati@intel.com>; Rao, Nikhil <nikhil.rao@intel.com>; Jerin Jacob
> <jerin.jacob@caviumnetworks.com>
> Subject: [dpdk-dev] [PATCH] eventdev: add producer enqueue hint
> 
> Some PMD like OCTEONTX can have optimized handling of events if the PMD
> knows it is a producer pattern in advance.
> For instance, OCTEONTX PMD can support burst mode if op ==
> RTE_EVENT_OP_NEW.
> 
> Since the event producer initialize(set all_op_new == 1) the event object before
> the main producer loop, This scheme does not call for any performance
> regression on other PMDs.
> 
> Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> ---
> Another option is to add a flag in enqueue API or have parallel enqueue API.
> ---
>  drivers/event/octeontx/ssovf_worker.c | 12 ++++++++++--
>  lib/librte_eventdev/rte_eventdev.h    | 10 +++++++++-
>  2 files changed, 19 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/event/octeontx/ssovf_worker.c
> b/drivers/event/octeontx/ssovf_worker.c
> index ad3fe684d..209c595cf 100644
> --- a/drivers/event/octeontx/ssovf_worker.c
> +++ b/drivers/event/octeontx/ssovf_worker.c
> @@ -196,8 +196,16 @@ ssows_enq(void *port, const struct rte_event *ev)
> uint16_t __hot  ssows_enq_burst(void *port, const struct rte_event ev[],
> uint16_t nb_events)  {
> -	RTE_SET_USED(nb_events);
> -	return ssows_enq(port, ev);
> +	uint16_t i;
> +	struct ssows *ws = port;
> +
> +	if (ev[0].all_op_new) {
> +		rte_smp_wmb();
> +		for (i = 0; i < nb_events; i++)
> +			ssows_new_event(ws, &ev[i]);
> +		return i;
> +	} else
> +		return ssows_enq(port, ev);
>  }
> 
>  void
> diff --git a/lib/librte_eventdev/rte_eventdev.h
> b/lib/librte_eventdev/rte_eventdev.h
> index a248fe90e..1c1a46593 100644
> --- a/lib/librte_eventdev/rte_eventdev.h
> +++ b/lib/librte_eventdev/rte_eventdev.h
> @@ -933,7 +933,15 @@ struct rte_event {
>  			 * and is undefined on dequeue.
>  			 * @see RTE_EVENT_OP_NEW, (RTE_EVENT_OP_*)
>  			 */
> -			uint8_t rsvd:4;
> +			uint8_t all_op_new:1;
> +			/**< Valid only with event enqueue operation - This hint
> +			 * indicates that the enqueue request has only the
> +			 * events with op == RTE_EVENT_OP_NEW.
> +			 * The event producer, typically use this pattern to
> +			 * inject the events to eventdev.
> +			 * @see RTE_EVENT_OP_NEW
> rte_event_enqueue_burst()
> +			 */
> +			uint8_t rsvd:3;
>  			/**< Reserved for future use */
>  			uint8_t sched_type:2;
>  			/**< Scheduler synchronization type
> (RTE_SCHED_TYPE_*)
> --
> 2.13.1

I slightly prefer the parallel enqueue API -- I can see folks making the mistake of setting all_op_new without setting the op to RTE_EVENT_OP_NEW, and later adding a "forward-only" enqueue API could be interesting for the sw PMD -- but this looks fine to me. Curious if others have any thoughts.

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] eventdev: add producer enqueue hint
  2017-06-26 15:44 ` Eads, Gage
@ 2017-06-27  8:08   ` Jerin Jacob
  2017-06-27  8:44     ` Van Haaren, Harry
  0 siblings, 1 reply; 5+ messages in thread
From: Jerin Jacob @ 2017-06-27  8:08 UTC (permalink / raw)
  To: Eads, Gage
  Cc: dev, Richardson, Bruce, Van Haaren, Harry, hemant.agrawal,
	nipun.gupta, Vangati, Narender, Rao, Nikhil

-----Original Message-----
> Date: Mon, 26 Jun 2017 15:44:10 +0000
> From: "Eads, Gage" <gage.eads@intel.com>
> To: Jerin Jacob <jerin.jacob@caviumnetworks.com>, "dev@dpdk.org"
>  <dev@dpdk.org>
> CC: "Richardson, Bruce" <bruce.richardson@intel.com>, "Van Haaren, Harry"
>  <harry.van.haaren@intel.com>, "hemant.agrawal@nxp.com"
>  <hemant.agrawal@nxp.com>, "nipun.gupta@nxp.com" <nipun.gupta@nxp.com>,
>  "Vangati, Narender" <narender.vangati@intel.com>, "Rao, Nikhil"
>  <nikhil.rao@intel.com>
> Subject: RE: [dpdk-dev] [PATCH] eventdev: add producer enqueue hint
> 
> 
> 
> > -----Original Message-----
> > From: Jerin Jacob [mailto:jerin.jacob@caviumnetworks.com]
> > Sent: Monday, June 12, 2017 6:46 AM
> > To: dev@dpdk.org
> > Cc: Richardson, Bruce <bruce.richardson@intel.com>; Van Haaren, Harry
> > <harry.van.haaren@intel.com>; hemant.agrawal@nxp.com; Eads, Gage
> > <gage.eads@intel.com>; nipun.gupta@nxp.com; Vangati, Narender
> > <narender.vangati@intel.com>; Rao, Nikhil <nikhil.rao@intel.com>; Jerin Jacob
> > <jerin.jacob@caviumnetworks.com>
> > Subject: [dpdk-dev] [PATCH] eventdev: add producer enqueue hint
> > 
> > Some PMD like OCTEONTX can have optimized handling of events if the PMD
> > knows it is a producer pattern in advance.
> > For instance, OCTEONTX PMD can support burst mode if op ==
> > RTE_EVENT_OP_NEW.
> > 
> > Since the event producer initialize(set all_op_new == 1) the event object before
> > the main producer loop, This scheme does not call for any performance
> > regression on other PMDs.
> > 
> > Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> > ---
> > Another option is to add a flag in enqueue API or have parallel enqueue API.
> > ---
> >  drivers/event/octeontx/ssovf_worker.c | 12 ++++++++++--
> >  lib/librte_eventdev/rte_eventdev.h    | 10 +++++++++-
> >  2 files changed, 19 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/event/octeontx/ssovf_worker.c
> > b/drivers/event/octeontx/ssovf_worker.c
> > index ad3fe684d..209c595cf 100644
> > --- a/drivers/event/octeontx/ssovf_worker.c
> > +++ b/drivers/event/octeontx/ssovf_worker.c
> > @@ -196,8 +196,16 @@ ssows_enq(void *port, const struct rte_event *ev)
> > uint16_t __hot  ssows_enq_burst(void *port, const struct rte_event ev[],
> > uint16_t nb_events)  {
> > -	RTE_SET_USED(nb_events);
> > -	return ssows_enq(port, ev);
> > +	uint16_t i;
> > +	struct ssows *ws = port;
> > +
> > +	if (ev[0].all_op_new) {
> > +		rte_smp_wmb();
> > +		for (i = 0; i < nb_events; i++)
> > +			ssows_new_event(ws, &ev[i]);
> > +		return i;
> > +	} else
> > +		return ssows_enq(port, ev);
> >  }
> > 
> >  void
> > diff --git a/lib/librte_eventdev/rte_eventdev.h
> > b/lib/librte_eventdev/rte_eventdev.h
> > index a248fe90e..1c1a46593 100644
> > --- a/lib/librte_eventdev/rte_eventdev.h
> > +++ b/lib/librte_eventdev/rte_eventdev.h
> > @@ -933,7 +933,15 @@ struct rte_event {
> >  			 * and is undefined on dequeue.
> >  			 * @see RTE_EVENT_OP_NEW, (RTE_EVENT_OP_*)
> >  			 */
> > -			uint8_t rsvd:4;
> > +			uint8_t all_op_new:1;
> > +			/**< Valid only with event enqueue operation - This hint
> > +			 * indicates that the enqueue request has only the
> > +			 * events with op == RTE_EVENT_OP_NEW.
> > +			 * The event producer, typically use this pattern to
> > +			 * inject the events to eventdev.
> > +			 * @see RTE_EVENT_OP_NEW
> > rte_event_enqueue_burst()
> > +			 */
> > +			uint8_t rsvd:3;
> >  			/**< Reserved for future use */
> >  			uint8_t sched_type:2;
> >  			/**< Scheduler synchronization type
> > (RTE_SCHED_TYPE_*)
> > --
> > 2.13.1
> 
> I slightly prefer the parallel enqueue API -- I can see folks making the mistake of setting all_op_new without setting the op to RTE_EVENT_OP_NEW, and later adding a "forward-only" enqueue API could be interesting for the sw PMD -- but this looks fine to me. Curious if others have any thoughts.

If forward-only parallel enqueue API interesting for the SW PMD then I
can drop this one and introduce forward-only API. Let me know if others
have any thoughts?

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] eventdev: add producer enqueue hint
  2017-06-27  8:08   ` Jerin Jacob
@ 2017-06-27  8:44     ` Van Haaren, Harry
  2017-06-27 10:17       ` Jerin Jacob
  0 siblings, 1 reply; 5+ messages in thread
From: Van Haaren, Harry @ 2017-06-27  8:44 UTC (permalink / raw)
  To: Jerin Jacob, Eads, Gage
  Cc: dev, Richardson, Bruce, hemant.agrawal, nipun.gupta, Vangati,
	Narender, Rao, Nikhil

> From: Jerin Jacob [mailto:jerin.jacob@caviumnetworks.com]
> Sent: Tuesday, June 27, 2017 9:08 AM
> To: Eads, Gage <gage.eads@intel.com>
> Cc: dev@dpdk.org; Richardson, Bruce <bruce.richardson@intel.com>; Van Haaren, Harry
> <harry.van.haaren@intel.com>; hemant.agrawal@nxp.com; nipun.gupta@nxp.com; Vangati,
> Narender <narender.vangati@intel.com>; Rao, Nikhil <nikhil.rao@intel.com>
> Subject: Re: [dpdk-dev] [PATCH] eventdev: add producer enqueue hint

<snip some patch code>

> > >  void
> > > diff --git a/lib/librte_eventdev/rte_eventdev.h
> > > b/lib/librte_eventdev/rte_eventdev.h
> > > index a248fe90e..1c1a46593 100644
> > > --- a/lib/librte_eventdev/rte_eventdev.h
> > > +++ b/lib/librte_eventdev/rte_eventdev.h
> > > @@ -933,7 +933,15 @@ struct rte_event {
> > >  			 * and is undefined on dequeue.
> > >  			 * @see RTE_EVENT_OP_NEW, (RTE_EVENT_OP_*)
> > >  			 */
> > > -			uint8_t rsvd:4;
> > > +			uint8_t all_op_new:1;
> > > +			/**< Valid only with event enqueue operation - This hint
> > > +			 * indicates that the enqueue request has only the
> > > +			 * events with op == RTE_EVENT_OP_NEW.
> > > +			 * The event producer, typically use this pattern to
> > > +			 * inject the events to eventdev.
> > > +			 * @see RTE_EVENT_OP_NEW
> > > rte_event_enqueue_burst()
> > > +			 */
> > > +			uint8_t rsvd:3;
> > >  			/**< Reserved for future use */
> > >  			uint8_t sched_type:2;
> > >  			/**< Scheduler synchronization type
> > > (RTE_SCHED_TYPE_*)
> > > --
> > > 2.13.1
> >
> > I slightly prefer the parallel enqueue API -- I can see folks making the mistake of
> setting all_op_new without setting the op to RTE_EVENT_OP_NEW, and later adding a
> "forward-only" enqueue API could be interesting for the sw PMD -- but this looks fine to
> me. Curious if others have any thoughts.
> 
> If forward-only parallel enqueue API interesting for the SW PMD then I
> can drop this one and introduce forward-only API. Let me know if others
> have any thoughts?


To make sure I understand correctly, the "parallel API" idea is to add a new function pointer per-PMD, and dedicate it to enqueueing a burst of packets with the same OP? So the end result would be function(s) in the public API like this:

rte_event_enqueue_burst_new(port, new_events, n_events);
rte_event_enqueue_burst_forward(port, new_events, n_events);

Given these are a "specialization" of the generic enqueue_burst() function, the PMD is not obliged to implement them. If they are NULL, the eventdev.c infrastructure can just point the burst_new() and burst_forward() to the generic enqueue without any performance delta?

The cost is some added code in the public header and infrastructure.
The gain is that we don't overload the current API with new behavior. 


Assuming my description of the parallel proposal above is correct, +1 for the parallel function approach. I like APIs that "do what they say on the tin" :)

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] eventdev: add producer enqueue hint
  2017-06-27  8:44     ` Van Haaren, Harry
@ 2017-06-27 10:17       ` Jerin Jacob
  0 siblings, 0 replies; 5+ messages in thread
From: Jerin Jacob @ 2017-06-27 10:17 UTC (permalink / raw)
  To: Van Haaren, Harry
  Cc: Eads, Gage, dev, Richardson, Bruce, hemant.agrawal, nipun.gupta,
	Vangati, Narender, Rao, Nikhil

-----Original Message-----
> Date: Tue, 27 Jun 2017 08:44:34 +0000
> From: "Van Haaren, Harry" <harry.van.haaren@intel.com>
> To: Jerin Jacob <jerin.jacob@caviumnetworks.com>, "Eads, Gage"
>  <gage.eads@intel.com>
> CC: "dev@dpdk.org" <dev@dpdk.org>, "Richardson, Bruce"
>  <bruce.richardson@intel.com>, "hemant.agrawal@nxp.com"
>  <hemant.agrawal@nxp.com>, "nipun.gupta@nxp.com" <nipun.gupta@nxp.com>,
>  "Vangati, Narender" <narender.vangati@intel.com>, "Rao, Nikhil"
>  <nikhil.rao@intel.com>
> Subject: RE: [dpdk-dev] [PATCH] eventdev: add producer enqueue hint
> 
> > From: Jerin Jacob [mailto:jerin.jacob@caviumnetworks.com]
> > Sent: Tuesday, June 27, 2017 9:08 AM
> > To: Eads, Gage <gage.eads@intel.com>
> > Cc: dev@dpdk.org; Richardson, Bruce <bruce.richardson@intel.com>; Van Haaren, Harry
> > <harry.van.haaren@intel.com>; hemant.agrawal@nxp.com; nipun.gupta@nxp.com; Vangati,
> > Narender <narender.vangati@intel.com>; Rao, Nikhil <nikhil.rao@intel.com>
> > Subject: Re: [dpdk-dev] [PATCH] eventdev: add producer enqueue hint
> 
> <snip some patch code>
> 
> > > >  void
> > > > diff --git a/lib/librte_eventdev/rte_eventdev.h
> > > > b/lib/librte_eventdev/rte_eventdev.h
> > > > index a248fe90e..1c1a46593 100644
> > > > --- a/lib/librte_eventdev/rte_eventdev.h
> > > > +++ b/lib/librte_eventdev/rte_eventdev.h
> > > > @@ -933,7 +933,15 @@ struct rte_event {
> > > >  			 * and is undefined on dequeue.
> > > >  			 * @see RTE_EVENT_OP_NEW, (RTE_EVENT_OP_*)
> > > >  			 */
> > > > -			uint8_t rsvd:4;
> > > > +			uint8_t all_op_new:1;
> > > > +			/**< Valid only with event enqueue operation - This hint
> > > > +			 * indicates that the enqueue request has only the
> > > > +			 * events with op == RTE_EVENT_OP_NEW.
> > > > +			 * The event producer, typically use this pattern to
> > > > +			 * inject the events to eventdev.
> > > > +			 * @see RTE_EVENT_OP_NEW
> > > > rte_event_enqueue_burst()
> > > > +			 */
> > > > +			uint8_t rsvd:3;
> > > >  			/**< Reserved for future use */
> > > >  			uint8_t sched_type:2;
> > > >  			/**< Scheduler synchronization type
> > > > (RTE_SCHED_TYPE_*)
> > > > --
> > > > 2.13.1
> > >
> > > I slightly prefer the parallel enqueue API -- I can see folks making the mistake of
> > setting all_op_new without setting the op to RTE_EVENT_OP_NEW, and later adding a
> > "forward-only" enqueue API could be interesting for the sw PMD -- but this looks fine to
> > me. Curious if others have any thoughts.
> > 
> > If forward-only parallel enqueue API interesting for the SW PMD then I
> > can drop this one and introduce forward-only API. Let me know if others
> > have any thoughts?
> 
> 
> To make sure I understand correctly, the "parallel API" idea is to add a new function pointer per-PMD, and dedicate it to enqueueing a burst of packets with the same OP? So the end result would be function(s) in the public API like this:
> 
> rte_event_enqueue_burst_new(port, new_events, n_events);
> rte_event_enqueue_burst_forward(port, new_events, n_events);
> 
> Given these are a "specialization" of the generic enqueue_burst() function, the PMD is not obliged to implement them. If they are NULL, the eventdev.c infrastructure can just point the burst_new() and burst_forward() to the generic enqueue without any performance delta?
> 
> The cost is some added code in the public header and infrastructure.
> The gain is that we don't overload the current API with new behavior. 
> 
> 
> Assuming my description of the parallel proposal above is correct, +1 for the parallel function approach. I like APIs that "do what they say on the tin" :)

Yes. We are on the same page. I will send the v2.

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2017-06-27 10:18 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-06-12 11:46 [PATCH] eventdev: add producer enqueue hint Jerin Jacob
2017-06-26 15:44 ` Eads, Gage
2017-06-27  8:08   ` Jerin Jacob
2017-06-27  8:44     ` Van Haaren, Harry
2017-06-27 10:17       ` Jerin Jacob

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.