> -----Original Message-----
> From: Jerin Jacob <jerinjac...@gmail.com>
> Sent: Wednesday, May 24, 2023 4:46 PM
> To: Yan, Zhirun <zhirun....@intel.com>
> Cc: dev@dpdk.org; jer...@marvell.com; kirankum...@marvell.com;
> ndabilpu...@marvell.com; step...@networkplumber.org;
> pbhagavat...@marvell.com; Liang, Cunming <cunming.li...@intel.com>; Wang,
> Haiyue <haiyue.w...@intel.com>; mattias.ronnblom
> <mattias.ronnb...@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....@intel.com> wrote:
> >
> > This patch enables to chose new scheduler model.
> >
> > Signed-off-by: Haiyue Wang <haiyue.w...@intel.com>
> > Signed-off-by: Cunming Liang <cunming.li...@intel.com>
> > Signed-off-by: Zhirun Yan <zhirun....@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.ronnb...@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.
