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

  This patch enables to chose new scheduler model.

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 | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)
  

On Tue, May 9, 2023 at 11:35 AM Zhirun Yan <zhirun.yan@intel.com> wrote:
>
> This patch enables to chose new scheduler model.
>
> 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>

>  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.


> +
> +       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@ericsson.com/),
I think, we need to change enqueue variants.

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)
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.
  

> -----Original Message-----
> From: Jerin Jacob <jerinjacobk@gmail.com>
> Sent: Wednesday, May 24, 2023 4:46 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 v6 12/15] graph: enable graph multicore dispatch scheduler
> model
> 
> On Tue, May 9, 2023 at 11:35 AM Zhirun Yan <zhirun.yan@intel.com> wrote:
> >
> > This patch enables to chose new scheduler model.
> >
> > 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>
> 
> >  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@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.