diff mbox series

[v9,13/17] graph: enable graph multicore dispatch scheduler model

Message ID 20230607035144.1214492-14-zhirun.yan@intel.com (mailing list archive)
State Changes Requested, archived
Delegated to: Thomas Monjalon
Headers
Series graph enhancement for multi-core dispatch |

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

Yan, Zhirun June 7, 2023, 3:51 a.m. UTC 
  This patch enables to chose new scheduler model. Must define
RTE_GRAPH_MODEL_SELECT before including rte_graph_worker.h
to enable specific model choosing.

Signed-off-by: Haiyue Wang <haiyue.wang@intel.com>
Signed-off-by: Cunming Liang <cunming.liang@intel.com>
Signed-off-by: Zhirun Yan <zhirun.yan@intel.com>
---
 lib/graph/rte_graph_worker.h | 12 ++++++++++++
 1 file changed, 12 insertions(+)

Comments

Jerin Jacob June 7, 2023, 8:15 a.m. UTC | #1 
On Wed, Jun 7, 2023 at 9:30 AM Zhirun Yan <zhirun.yan@intel.com> wrote:
>
> This patch enables to chose new scheduler model. Must define
> RTE_GRAPH_MODEL_SELECT before including rte_graph_worker.h
> to enable specific model choosing.
>
> Signed-off-by: Haiyue Wang <haiyue.wang@intel.com>
> Signed-off-by: Cunming Liang <cunming.liang@intel.com>
> Signed-off-by: Zhirun Yan <zhirun.yan@intel.com>
> ---
>  lib/graph/rte_graph_worker.h | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>

>  rte_graph_walk(struct rte_graph *graph)
>  {
> +#if !defined(RTE_GRAPH_MODEL_SELECT) || RTE_GRAPH_MODEL_SELECT == RTE_GRAPH_MODEL_RTC

Is nt defined instead of !defined?

Use bracket around RTE_GRAPH_MODEL_SELECT == RTE_GRAPH_MODEL_RTC.


>         rte_graph_walk_rtc(graph);
> +#elif defined(RTE_GRAPH_MODEL_SELECT) && RTE_GRAPH_MODEL_SELECT == RTE_GRAPH_MODEL_MCORE_DISPATCH

Use bracket around RTE_GRAPH_MODEL_SELECT ==

> +       rte_graph_walk_mcore_dispatch(graph);
> +#else
> +       int model = rte_graph_worker_model_get(graph);

Introduce rte_graph_worker_model_no_check_get() as commented earlier.
> +
> +       if (model == RTE_GRAPH_MODEL_RTC)
> +               rte_graph_walk_rtc(graph);
> +       else if (model == RTE_GRAPH_MODEL_MCORE_DISPATCH)
> +               rte_graph_walk_mcore_dispatch(graph);

I think, switch case better to support new model in future. Please
check the performance before changing.

i.e

switch ( rte_graph_worker_model_no_check_get())
{
case RTE_GRAPH_MODEL_MCORE_DISPATCH:
        rte_graph_walk_mcore_dispatch(graph)
        break;
default:
       rte_graph_walk_rtc(graph);
}
Yan, Zhirun June 7, 2023, 12:25 p.m. UTC | #2 
> -----Original Message-----
> From: Jerin Jacob <jerinjacobk@gmail.com>
> Sent: Wednesday, June 7, 2023 4:15 PM
> To: Yan, Zhirun <zhirun.yan@intel.com>
> Cc: dev@dpdk.org; jerinj@marvell.com; kirankumark@marvell.com;
> ndabilpuram@marvell.com; stephen@networkplumber.org;
> pbhagavatula@marvell.com; Liang, Cunming <cunming.liang@intel.com>; Wang,
> Haiyue <haiyue.wang@intel.com>; mattias.ronnblom
> <mattias.ronnblom@ericsson.com>
> Subject: Re: [PATCH v9 13/17] graph: enable graph multicore dispatch scheduler
> model
> 
> On Wed, Jun 7, 2023 at 9:30 AM Zhirun Yan <zhirun.yan@intel.com> wrote:
> >
> > This patch enables to chose new scheduler model. Must define
> > RTE_GRAPH_MODEL_SELECT before including rte_graph_worker.h to enable
> > specific model choosing.
> >
> > Signed-off-by: Haiyue Wang <haiyue.wang@intel.com>
> > Signed-off-by: Cunming Liang <cunming.liang@intel.com>
> > Signed-off-by: Zhirun Yan <zhirun.yan@intel.com>
> > ---
> >  lib/graph/rte_graph_worker.h | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> >
> 
> >  rte_graph_walk(struct rte_graph *graph)  {
> > +#if !defined(RTE_GRAPH_MODEL_SELECT) || RTE_GRAPH_MODEL_SELECT
> ==
> > +RTE_GRAPH_MODEL_RTC
> 
> Is nt defined instead of !defined?
>

!defined(XX) means not defined XX.
What is nt defined means?

> Use bracket around RTE_GRAPH_MODEL_SELECT == RTE_GRAPH_MODEL_RTC.
> 
Ok.

> 
> >         rte_graph_walk_rtc(graph);
> > +#elif defined(RTE_GRAPH_MODEL_SELECT) && RTE_GRAPH_MODEL_SELECT
> ==
> > +RTE_GRAPH_MODEL_MCORE_DISPATCH
> 
> Use bracket around RTE_GRAPH_MODEL_SELECT ==
Ok.
> 
> > +       rte_graph_walk_mcore_dispatch(graph);
> > +#else
> > +       int model = rte_graph_worker_model_get(graph);
> 
> Introduce rte_graph_worker_model_no_check_get() as commented earlier.

Got it.

> > +
> > +       if (model == RTE_GRAPH_MODEL_RTC)
> > +               rte_graph_walk_rtc(graph);
> > +       else if (model == RTE_GRAPH_MODEL_MCORE_DISPATCH)
> > +               rte_graph_walk_mcore_dispatch(graph);
> 
> I think, switch case better to support new model in future. Please check the
> performance before changing.
> 

Yes, I agree.
And I checked the performance with switch case and rte_graph_worker_model_no_check_get()
I get very similar performance. The improvements is <0.1%. I guess the performance impact will
be less if the workerload goes more complicated, cause the node process in walk will spent more
time.
And I will change in next version.

> i.e
> 
> switch ( rte_graph_worker_model_no_check_get())
> {
> case RTE_GRAPH_MODEL_MCORE_DISPATCH:
>         rte_graph_walk_mcore_dispatch(graph)
>         break;
> default:
>        rte_graph_walk_rtc(graph);
> }
Jerin Jacob June 7, 2023, 1:26 p.m. UTC | #3 
On Wed, Jun 7, 2023 at 5:55 PM Yan, Zhirun <zhirun.yan@intel.com> wrote:

> >
> > >  rte_graph_walk(struct rte_graph *graph)  {
> > > +#if !defined(RTE_GRAPH_MODEL_SELECT) || RTE_GRAPH_MODEL_SELECT
> > ==
> > > +RTE_GRAPH_MODEL_RTC
> >
> > Is nt defined instead of !defined?
> >
>
> !defined(XX) means not defined XX.
> What is nt defined means?

#undef RTE_GRAPH_MODEL_SELECT or not #define RTE_GRAPH_MODEL_SELECT
anywhere in .c file.

>
> > Use bracket around RTE_GRAPH_MODEL_SELECT == RTE_GRAPH_MODEL_RTC.
> >
> Ok.
>
> >
> > >         rte_graph_walk_rtc(graph);
> > > +#elif defined(RTE_GRAPH_MODEL_SELECT) && RTE_GRAPH_MODEL_SELECT
> > ==
> > > +RTE_GRAPH_MODEL_MCORE_DISPATCH
> >
> > Use bracket around RTE_GRAPH_MODEL_SELECT ==
> Ok.
> >
> > > +       rte_graph_walk_mcore_dispatch(graph);
> > > +#else
> > > +       int model = rte_graph_worker_model_get(graph);
> >
> > Introduce rte_graph_worker_model_no_check_get() as commented earlier.
>
> Got it.
>
Yan, Zhirun June 8, 2023, 3:08 a.m. UTC | #4 
> -----Original Message-----
> From: Jerin Jacob <jerinjacobk@gmail.com>
> Sent: Wednesday, June 7, 2023 9:26 PM
> To: Yan, Zhirun <zhirun.yan@intel.com>
> Cc: dev@dpdk.org; jerinj@marvell.com; kirankumark@marvell.com;
> ndabilpuram@marvell.com; stephen@networkplumber.org;
> pbhagavatula@marvell.com; Liang, Cunming <cunming.liang@intel.com>; Wang,
> Haiyue <haiyue.wang@intel.com>; mattias.ronnblom
> <mattias.ronnblom@ericsson.com>
> Subject: Re: [PATCH v9 13/17] graph: enable graph multicore dispatch scheduler
> model
> 
> On Wed, Jun 7, 2023 at 5:55 PM Yan, Zhirun <zhirun.yan@intel.com> wrote:
> 
> > >
> > > >  rte_graph_walk(struct rte_graph *graph)  {
> > > > +#if !defined(RTE_GRAPH_MODEL_SELECT) ||
> RTE_GRAPH_MODEL_SELECT
> > > ==
> > > > +RTE_GRAPH_MODEL_RTC
> > >
> > > Is nt defined instead of !defined?
> > >
> >
> > !defined(XX) means not defined XX.
> > What is nt defined means?
> 
> #undef RTE_GRAPH_MODEL_SELECT or not #define
> RTE_GRAPH_MODEL_SELECT anywhere in .c file.
> 

In the implementation, RTE_GRAPH_MODEL_SELECT is only defined once in app
I think #if !define(XX) is a judgement, #undef XX is an action.
Here should be #if !define(XX)
For this impl, I treat not define as default and go into rtc_walk().


So If we treat not defined RTE_GRAPH_MODEL_SELECT as runtime pick.
The #else case should cover: 1. Not defined and 2. Defined other type.
It should be as follow:

rte_graph_walk()
{
#if defined(RTE_GRAPH_MODEL_SELECT) && RTE_GRAPH_MODEL_SELECT == RTE_GRAPH_MODEL_RTC
	rte_graph_walk_rtc();

#elif defined(RTE_GRAPH_MODEL_SELECT) && RTE_GRAPH_MODEL_SELECT == RTE_GRAPH_MODEL_MCORE_DISPATCH
	rte_graph_walk_mcore_dispatch(graph);

#else
	const int model = rte_graph_worker_model_no_check_get();

	switch (model) {
	case RTE_GRAPH_MODEL_MCORE_DISPATCH:
		rte_graph_walk_mcore_dispatch();
		break;
	default:
		rte_graph_walk_rtc();
	}
#endif
}

> >
> > > Use bracket around RTE_GRAPH_MODEL_SELECT ==
> RTE_GRAPH_MODEL_RTC.
> > >
> > Ok.
> >
> > >
> > > >         rte_graph_walk_rtc(graph);
> > > > +#elif defined(RTE_GRAPH_MODEL_SELECT) &&
> RTE_GRAPH_MODEL_SELECT
> > > ==
> > > > +RTE_GRAPH_MODEL_MCORE_DISPATCH
> > >
> > > Use bracket around RTE_GRAPH_MODEL_SELECT ==
> > Ok.
> > >
> > > > +       rte_graph_walk_mcore_dispatch(graph);
> > > > +#else
> > > > +       int model = rte_graph_worker_model_get(graph);
> > >
> > > Introduce rte_graph_worker_model_no_check_get() as commented earlier.
> >
> > Got it.
> >
Jerin Jacob June 8, 2023, 5:33 a.m. UTC | #5 
On Thu, Jun 8, 2023 at 8:39 AM Yan, Zhirun <zhirun.yan@intel.com> wrote:
>
>
>
> > -----Original Message-----
> > From: Jerin Jacob <jerinjacobk@gmail.com>
> > Sent: Wednesday, June 7, 2023 9:26 PM
> > To: Yan, Zhirun <zhirun.yan@intel.com>
> > Cc: dev@dpdk.org; jerinj@marvell.com; kirankumark@marvell.com;
> > ndabilpuram@marvell.com; stephen@networkplumber.org;
> > pbhagavatula@marvell.com; Liang, Cunming <cunming.liang@intel.com>; Wang,
> > Haiyue <haiyue.wang@intel.com>; mattias.ronnblom
> > <mattias.ronnblom@ericsson.com>
> > Subject: Re: [PATCH v9 13/17] graph: enable graph multicore dispatch scheduler
> > model
> >
> > On Wed, Jun 7, 2023 at 5:55 PM Yan, Zhirun <zhirun.yan@intel.com> wrote:
> >
> > > >
> > > > >  rte_graph_walk(struct rte_graph *graph)  {
> > > > > +#if !defined(RTE_GRAPH_MODEL_SELECT) ||
> > RTE_GRAPH_MODEL_SELECT
> > > > ==
> > > > > +RTE_GRAPH_MODEL_RTC
> > > >
> > > > Is nt defined instead of !defined?
> > > >
> > >
> > > !defined(XX) means not defined XX.
> > > What is nt defined means?
> >
> > #undef RTE_GRAPH_MODEL_SELECT or not #define
> > RTE_GRAPH_MODEL_SELECT anywhere in .c file.
> >
>
> In the implementation, RTE_GRAPH_MODEL_SELECT is only defined once in app
> I think #if !define(XX) is a judgement, #undef XX is an action.
> Here should be #if !define(XX)
> For this impl, I treat not define as default and go into rtc_walk().
>
>
> So If we treat not defined RTE_GRAPH_MODEL_SELECT as runtime pick.
> The #else case should cover: 1. Not defined and 2. Defined other type.
> It should be as follow:


Ack. We are aligned, You can send the next version. Keep my existing
Acked-by on the patches which is already reviewed..
I should be able to give Acked-by on the pending one to complete the review.


>
> rte_graph_walk()
> {
> #if defined(RTE_GRAPH_MODEL_SELECT) && RTE_GRAPH_MODEL_SELECT == RTE_GRAPH_MODEL_RTC

( RTE_GRAPH_MODEL_SELECT == RTE_GRAPH_MODEL_RTC)

>         rte_graph_walk_rtc();
>
> #elif defined(RTE_GRAPH_MODEL_SELECT) && RTE_GRAPH_MODEL_SELECT == RTE_GRAPH_MODEL_MCORE_DISPATCH

(RTE_GRAPH_MODEL_SELECT == RTE_GRAPH_MODEL_MCORE_DISPATCH)

>         rte_graph_walk_mcore_dispatch(graph);
>
> #else
>         const int model =;
>
>         switch (model) {

switch ( rte_graph_worker_model_no_check_get()) {
as model not used anywhere else belwo , model is changing to uint8_t

>         case RTE_GRAPH_MODEL_MCORE_DISPATCH:
>                 rte_graph_walk_mcore_dispatch();
>                 break;
>         default:
>                 rte_graph_walk_rtc();
>         }
> #endif
> }
>
> > >
> > > > Use bracket around RTE_GRAPH_MODEL_SELECT ==
> > RTE_GRAPH_MODEL_RTC.
> > > >
> > > Ok.
> > >
> > > >
> > > > >         rte_graph_walk_rtc(graph);
> > > > > +#elif defined(RTE_GRAPH_MODEL_SELECT) &&
> > RTE_GRAPH_MODEL_SELECT
> > > > ==
> > > > > +RTE_GRAPH_MODEL_MCORE_DISPATCH
> > > >
> > > > Use bracket around RTE_GRAPH_MODEL_SELECT ==
> > > Ok.
> > > >
> > > > > +       rte_graph_walk_mcore_dispatch(graph);
> > > > > +#else
> > > > > +       int model = rte_graph_worker_model_get(graph);
> > > >
> > > > Introduce rte_graph_worker_model_no_check_get() as commented earlier.
> > >
> > > Got it.
> > >
Yan, Zhirun June 8, 2023, 7:06 a.m. UTC | #6 
> -----Original Message-----
> From: Jerin Jacob <jerinjacobk@gmail.com>
> Sent: Thursday, June 8, 2023 1:34 PM
> To: Yan, Zhirun <zhirun.yan@intel.com>
> Cc: dev@dpdk.org; jerinj@marvell.com; kirankumark@marvell.com;
> ndabilpuram@marvell.com; stephen@networkplumber.org;
> pbhagavatula@marvell.com; Liang, Cunming <cunming.liang@intel.com>; Wang,
> Haiyue <haiyue.wang@intel.com>; mattias.ronnblom
> <mattias.ronnblom@ericsson.com>
> Subject: Re: [PATCH v9 13/17] graph: enable graph multicore dispatch scheduler
> model
> 
> On Thu, Jun 8, 2023 at 8:39 AM Yan, Zhirun <zhirun.yan@intel.com> wrote:
> >
> >
> >
> > > -----Original Message-----
> > > From: Jerin Jacob <jerinjacobk@gmail.com>
> > > Sent: Wednesday, June 7, 2023 9:26 PM
> > > To: Yan, Zhirun <zhirun.yan@intel.com>
> > > Cc: dev@dpdk.org; jerinj@marvell.com; kirankumark@marvell.com;
> > > ndabilpuram@marvell.com; stephen@networkplumber.org;
> > > pbhagavatula@marvell.com; Liang, Cunming <cunming.liang@intel.com>;
> > > Wang, Haiyue <haiyue.wang@intel.com>; mattias.ronnblom
> > > <mattias.ronnblom@ericsson.com>
> > > Subject: Re: [PATCH v9 13/17] graph: enable graph multicore dispatch
> > > scheduler model
> > >
> > > On Wed, Jun 7, 2023 at 5:55 PM Yan, Zhirun <zhirun.yan@intel.com> wrote:
> > >
> > > > >
> > > > > >  rte_graph_walk(struct rte_graph *graph)  {
> > > > > > +#if !defined(RTE_GRAPH_MODEL_SELECT) ||
> > > RTE_GRAPH_MODEL_SELECT
> > > > > ==
> > > > > > +RTE_GRAPH_MODEL_RTC
> > > > >
> > > > > Is nt defined instead of !defined?
> > > > >
> > > >
> > > > !defined(XX) means not defined XX.
> > > > What is nt defined means?
> > >
> > > #undef RTE_GRAPH_MODEL_SELECT or not #define
> RTE_GRAPH_MODEL_SELECT
> > > anywhere in .c file.
> > >
> >
> > In the implementation, RTE_GRAPH_MODEL_SELECT is only defined once in
> > app I think #if !define(XX) is a judgement, #undef XX is an action.
> > Here should be #if !define(XX)
> > For this impl, I treat not define as default and go into rtc_walk().
> >
> >
> > So If we treat not defined RTE_GRAPH_MODEL_SELECT as runtime pick.
> > The #else case should cover: 1. Not defined and 2. Defined other type.
> > It should be as follow:
> 
> 
> Ack. We are aligned, You can send the next version. Keep my existing Acked-by
> on the patches which is already reviewed..
> I should be able to give Acked-by on the pending one to complete the review.
> 
Thanks, I will send new patch set.
> 
> >
> > rte_graph_walk()
> > {
> > #if defined(RTE_GRAPH_MODEL_SELECT) && RTE_GRAPH_MODEL_SELECT ==
> > RTE_GRAPH_MODEL_RTC
> 
> ( RTE_GRAPH_MODEL_SELECT == RTE_GRAPH_MODEL_RTC)
> 
> >         rte_graph_walk_rtc();
> >
> > #elif defined(RTE_GRAPH_MODEL_SELECT) && RTE_GRAPH_MODEL_SELECT
> ==
> > RTE_GRAPH_MODEL_MCORE_DISPATCH
> 
> (RTE_GRAPH_MODEL_SELECT == RTE_GRAPH_MODEL_MCORE_DISPATCH)
> 
> >         rte_graph_walk_mcore_dispatch(graph);
> >
> > #else
> >         const int model =;
> >
> >         switch (model) {
> 
> switch ( rte_graph_worker_model_no_check_get()) { as model not used
> anywhere else belwo , model is changing to uint8_t
> 
Got it.
> >         case RTE_GRAPH_MODEL_MCORE_DISPATCH:
> >                 rte_graph_walk_mcore_dispatch();
> >                 break;
> >         default:
> >                 rte_graph_walk_rtc();
> >         }
> > #endif
> > }
> >
> > > >
> > > > > Use bracket around RTE_GRAPH_MODEL_SELECT ==
> > > RTE_GRAPH_MODEL_RTC.
> > > > >
> > > > Ok.
> > > >
> > > > >
> > > > > >         rte_graph_walk_rtc(graph);
> > > > > > +#elif defined(RTE_GRAPH_MODEL_SELECT) &&
> > > RTE_GRAPH_MODEL_SELECT
> > > > > ==
> > > > > > +RTE_GRAPH_MODEL_MCORE_DISPATCH
> > > > >
> > > > > Use bracket around RTE_GRAPH_MODEL_SELECT ==
> > > > Ok.
> > > > >
> > > > > > +       rte_graph_walk_mcore_dispatch(graph);
> > > > > > +#else
> > > > > > +       int model = rte_graph_worker_model_get(graph);
> > > > >
> > > > > Introduce rte_graph_worker_model_no_check_get() as commented
> earlier.
> > > >
> > > > Got it.
> > > >
diff mbox series

Patch

diff --git a/lib/graph/rte_graph_worker.h b/lib/graph/rte_graph_worker.h
index 5b58f7bda9..69bdd0e074 100644
--- a/lib/graph/rte_graph_worker.h
+++ b/lib/graph/rte_graph_worker.h
@@ -11,6 +11,7 @@  extern "C" {
 #endif
 
 #include "rte_graph_model_rtc.h"
+#include "rte_graph_model_mcore_dispatch.h"
 
 /**
  * Perform graph walk on the circular buffer and invoke the process function
@@ -25,7 +26,18 @@  __rte_experimental
 static inline void
 rte_graph_walk(struct rte_graph *graph)
 {
+#if !defined(RTE_GRAPH_MODEL_SELECT) || RTE_GRAPH_MODEL_SELECT == RTE_GRAPH_MODEL_RTC
 	rte_graph_walk_rtc(graph);
+#elif defined(RTE_GRAPH_MODEL_SELECT) && RTE_GRAPH_MODEL_SELECT == RTE_GRAPH_MODEL_MCORE_DISPATCH
+	rte_graph_walk_mcore_dispatch(graph);
+#else
+	int model = rte_graph_worker_model_get(graph);
+
+	if (model == RTE_GRAPH_MODEL_RTC)
+		rte_graph_walk_rtc(graph);
+	else if (model == RTE_GRAPH_MODEL_MCORE_DISPATCH)
+		rte_graph_walk_mcore_dispatch(graph);
+#endif
 }
 
 #ifdef __cplusplus