[PATCH v7 04/17] graph: add get/set graph worker model APIs

[Yan, Zhirun]

> -----Original Message-----
> From: Jerin Jacob <jerinjacobk at gmail.com>
> Sent: Tuesday, June 6, 2023 1:48 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 v7 04/17] graph: add get/set graph worker model APIs
> 
> On Tue, Jun 6, 2023 at 10:00 AM Yan, Zhirun <zhirun.yan at intel.com> wrote:
> >
> >
> >
> > > -----Original Message-----
> > > From: Jerin Jacob <jerinjacobk at gmail.com>
> > > Sent: Monday, June 5, 2023 8:38 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 v7 04/17] graph: add get/set graph worker model
> > > APIs
> > >
> > > On Mon, Jun 5, 2023 at 4:56 PM Zhirun Yan <zhirun.yan at intel.com> wrote:
> > > >
> > > > Add new get/set APIs to configure graph worker model which is used
> > > > to determine which model will be chosen.
> > > >
> > > > 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/meson.build               |  1 +
> > > >  lib/graph/rte_graph_worker.c        | 70
> +++++++++++++++++++++++++++++
> > > >  lib/graph/rte_graph_worker_common.h | 19 ++++++++
> > > >  lib/graph/version.map               |  3 ++
> > > >  4 files changed, 93 insertions(+)  create mode 100644
> > > > lib/graph/rte_graph_worker.c
> > > >
> > > > diff --git a/lib/graph/meson.build b/lib/graph/meson.build index
> > > > 3526d1b5d4..9fab8243da 100644
> > > > --- a/lib/graph/meson.build
> > > > +++ b/lib/graph/meson.build
> > > > @@ -15,6 +15,7 @@ sources = files(
> > > >          'graph_stats.c',
> > > >          'graph_populate.c',
> > > >          'graph_pcap.c',
> > > > +        'rte_graph_worker.c',
> > > >  )
> > > >  headers = files('rte_graph.h', 'rte_graph_worker.h')
> > > >
> > > > diff --git a/lib/graph/rte_graph_worker.c
> > > > b/lib/graph/rte_graph_worker.c new file mode 100644 index
> > > > 0000000000..fc188e7cfa
> > > > --- /dev/null
> > > > +++ b/lib/graph/rte_graph_worker.c
> > > > @@ -0,0 +1,70 @@
> > > > +/* SPDX-License-Identifier: BSD-3-Clause
> > > > + * Copyright(C) 2023 Intel Corporation  */
> > > > +
> > > > +/**
> > > > + * @file graph_worker.c
> > > > + *
> > > > + * @warning
> > > > + * @b EXPERIMENTAL:
> > > > + * All functions in this file may be changed or removed without prior
> notice.
> > > > + *
> > > > + * These API enable to set/get graph walking model.
> > > > + *
> > > > + */
> > > > +
> > > > +#include "rte_graph_worker_common.h"
> > > > +#include "graph_private.h"
> > > > +
> > > > +/**
> > > > + * @note This function does not perform any locking, and is only safe to
> call
> > > > + *    before graph running. It will set all graphs the same model.
> > > > + *
> > > > + * @param name
> > > > + *   Name of the graph worker model.
> > > > + *
> > > > + * @return
> > > > + *   0 on success, -1 otherwise.
> > > < 0 otherwise
> > >
> > > Doxygen comment is not required .c file.
> > >
> > Yes, I will move the declaration into rte_graph_worker.h
> >
> > >
> > > > + */
> > > > +int
> > > > +rte_graph_worker_model_set(enum rte_graph_worker_model model) {
> > > > +       struct graph_head *graph_head = graph_list_head_get();
> > > > +       struct graph *graph;
> > > > +       int ret = 0;
> > > > +
> > > > +       if (model == RTE_GRAPH_MODEL_DEFAULT || model ==
> > > RTE_GRAPH_MODEL_RTC ||
> > > > +           model == RTE_GRAPH_MODEL_MCORE_DISPATCH)
> > > > +               STAILQ_FOREACH(graph, graph_head, next)
> > > > +                       graph->graph->model = model;
> > > > +       else {
> > > > +               STAILQ_FOREACH(graph, graph_head, next)
> > > > +                       graph->graph->model = RTE_GRAPH_MODEL_DEFAULT;
> > > > +               ret = -1;
> > >
> > > Why returning -1 here?
> > > Also, why "else" case needed as RTE_GRAPH_MODEL_RTC ==
> > > RTE_GRAPH_MODEL_DEFAULT
> >
> > Actually, the "else" offers a way to recover if this func called with
> > model > RTE_GRAPH_MODEL_MCORE_DISPATCH, or another not supported
> > value. Then the app could have ability to run in default RTC model or user app
> could reset the model.
> >
> > >
> > > Why not
> > >
> > > struct graph_head *graph_head = graph_list_head_get(); struct graph
> > > *graph;
> > >
> > > [1]
> > > if (model > RTE_GRAPH_MODEL_MCORE_DISPATCH)
> > >        return -EINVAL;
> > >
> > > STAILQ_FOREACH(graph, graph_head, next)
> > >               graph->graph->model = model;
> > >
> > >
> > > For [1], Please add internal helper function graph_model_is_valid()
> > > to use everywhere as needed.
> > >
> >
> > I will add graph_model_is_valid().
> >
> > And change it to
> >
> > If (!graph_model_is_valid()) {
> >   model = RTE_GRAPH_MODEL_DEFAULT;
> 
> Since it returning from below, Do we need to update model?
> 
No, no need to update it. I reset model here cause I want to reset all graph->model,
But miss the graph loop. I think it is no need to update all graph->model also.
I will remove it and return directly.

> >   return -EINVAL;
> > }
> > STAILQ_FOREACH(graph, graph_head, next)
> >     graph->graph->model = model;
> 
> graph->model = model. Right?
No. This graph is struct graph, and we put model into struct rte_graph.
So it is 
(struct graph) graph -> (struct rte_graph *) graph -> model

> 
> 
> >
> > return 0;
> >
> > > > +               }
> > > > +
> > > > +       return ret;
> > > > +}
> > > > +
> > > > +/**
> > > > + * Get the graph worker model
> > > > + *
> > > > + * @note All graph will use the same model and this function will
> > > > +get model from the first one
> > > > + *
> > > > + * @param name
> > > > + *   Name of the graph worker model.
> > > > + *
> > > > + * @return
> > > > + *   Graph worker model on success.
> > > > + */
> > > > +inline
> > > > +enum rte_graph_worker_model
> > > > +rte_graph_worker_model_get(void)
> > > > +{
> > > > +       struct graph_head *graph_head = graph_list_head_get();
> > > > +       struct graph *graph;
> > > > +
> > > > +       graph = STAILQ_FIRST(graph_head);
> > >
> > > This can be used in fastpath, So lets pass graph object and make
> > > inline function to return graph->model
> > >
> >
> > Some functions don't have the graph object like graph_stats_*. No
> > param could make this API easy to call for current impl. And I think
> > this func is mainly for configuration,
> 
> But, you are using this in fastpath here.
> https://patches.dpdk.org/project/dpdk/patch/20230605111923.3772260-14-
> zhirun.yan at intel.com/
> 

Yes, this is in "else" case. In my opinion, it  is for those who care flexibility
more than performance(they choose GRAPH_MODEL_MCORE_RUNTIME_SELECT).

You are right. Passing the graph object is better.

> > should not in fastpath. If different models coexisted, then the graph object
> must be passed.
> 
> I think, all fastpath cases graph->model can tell the model.
> graph_stats_* API etc still
> has rte_graph object so it should be possible to reuse there.

Yes, I understand that. I will change it as you said. Thanks.