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

[Jerin Jacob <jerinjacobk@gmail.com>]

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.