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

[Yan, Zhirun]

> -----Original Message-----
> From: Jerin Jacob <jerinjacobk at gmail.com>
> Sent: Wednesday, June 7, 2023 4:15 PM
> To: Yan, Zhirun <zhirun.yan at intel.com>
> Cc: dev at dpdk.org; jerinj at marvell.com; kirankumark at marvell.com;
> ndabilpuram at marvell.com; stephen at networkplumber.org;
> pbhagavatula at marvell.com; Liang, Cunming <cunming.liang at intel.com>; Wang,
> Haiyue <haiyue.wang at intel.com>; mattias.ronnblom
> <mattias.ronnblom at 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 at 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 at intel.com>
> > Signed-off-by: Cunming Liang <cunming.liang at intel.com>
> > Signed-off-by: Zhirun Yan <zhirun.yan at 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);
> }