[PATCH v6 12/15] graph: enable graph multicore dispatch scheduler model

[Yan, Zhirun]

> -----Original Message-----
> From: Jerin Jacob <jerinjacobk at gmail.com>
> Sent: Wednesday, May 24, 2023 4:46 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 v6 12/15] graph: enable graph multicore dispatch scheduler
> model
> 
> On Tue, May 9, 2023 at 11:35 AM Zhirun Yan <zhirun.yan at intel.com> wrote:
> >
> > This patch enables to chose new scheduler model.
> >
> > 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>
> 
> >  rte_graph_walk(struct rte_graph *graph)  {
> > -       rte_graph_walk_rtc(graph);
> > +       int model = rte_graph_worker_model_get();
> 
>  Any specific to reason to keep model value in LCORE variable , why not in  struct
> rte_graph?
> It is not specific to this patch. But good to understand as storing in
> rte_graph* will avoid cache miss.
> 
Yes, I can put it into rte_graph.

> 
> > +
> > +       if (model == RTE_GRAPH_MODEL_DEFAULT ||
> > +           model == RTE_GRAPH_MODEL_RTC)
> 
> I think, there can be three ways to do this
> 
> a) Store model in PER_LCORE or struct rte_graph and add runtime check
> b) Make it as function pointer for graph_walk
> 
> mcore_dispatch model is reusing all rte_node_enqueue_* functions, so for
> NOW only graph walk is different.
> But if need to integrate other graph models like eventdev backend(similar
> problem trying to solve in
> https://patches.dpdk.org/project/dpdk/patch/20230522091628.96236-2-
> mattias.ronnblom at ericsson.com/),
> I think, we need to change enqueue variants.
> 
Yes, there is no change for rte_node_enqueue_*.
I will follow this thread. And may make some contribution after this release.

> Both (a) and (b) has little performance impact in "current situation with this
> patch" and if we need to add similar check and function pointer for overriding
> node_enqueue_ functions it will have major impact.
> In order to have NO performance impact and able to overide node_enqueue_
> functions, I think, we can have the following scheme in application and library.
> 
> In application
> #define RTE_GRAPH_MODEL_SELECT RTE_GRAPH_MODEL_RTC #include
> <rte_graph_model.h>
> 
> In library:
> #if RTE_GRAPH_MODEL_SELECT == RTE_GRAPH_MODEL_RTC #define
> rte_graph_walk rte_graph_walk_rtc #else if RTE_GRAPH_MODEL_SELECT ==
> RTE_GRAPH_MODEL_MCORE_DISPATCH #define rte_graph_walk
> rte_graph_walk_mcore_dispatch
> 
> It is kind of compile time, But application can use function templating by proving
> different values RTE_GRAPH_MODEL_SELECT to make runtime if given
> application needs to support all modes at runtime.
> 
> 
> As an example:
> 
> app_my_graph_processing.h has application code  for graph walk and node
> processing.
> 
> app_worker_rtc.c
> ------------------------
> #define RTE_GRAPH_MODEL_SELECT RTE_GRAPH_MODEL_RTC #include
> <rte_graph_model.h> #include app_my_graph_processing.h
> 
> void app_worker_rtc()
> {
>           while (1) {
>                rte_graph_walk()
>           }
> }
> 
> app_worker_mcore_dispatch.c
> -----------------------------------------
> 
> #define RTE_GRAPH_MODEL_SELECT RTE_GRAPH_MODEL_MCORE_DISPATCH
> #include <rte_graph_model.h> #include app_my_graph_processing.h
> 
> void app_worker_mcore_dispatch()
> {
>           while (1) {
>                rte_graph_walk()
>           }
> }
> 
> in main()
> -------------
> 
> if (command line arg provided worker as rtc)

Got it.
And we could use rte_graph->model to choose rtc or dispatch for future. Then it could be possible for models coexistence as you said before.


> rte_eal_remote_launch(app_worker_rtc)
> else
> rte_eal_remote_launch(app_worker_mcore_dispatch)
> 
> -----------------------------------------
> With that note, ending review comment for this series.
> 
> In general patches look good high level, following items need to be sorted in
> next version. Then I think, it is good to merge in this release.
> 
> 1) Above points on fixing performance and supporting more graph model
> variants
> 2) Need to add UT for ALL new APIs in app/test/test_graph.c
> 3) Make sure no performance regression with app/test/test_graph_perf.c with
> new changes
> 4) Addressing existing comments in this series.
> 
> Thanks for great work.


Hi Jerin,

Thanks for your review. I will fix these in next version.