> -----Original Message-----
> From: Jerin Jacob <jerinjac...@gmail.com>
> Sent: Tuesday, June 6, 2023 1:48 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 v7 04/17] graph: add get/set graph worker model APIs
>
> On Tue, Jun 6, 2023 at 10:00 AM Yan, Zhirun <zhirun....@intel.com> wrote:
> >
> >
> >
> > > -----Original Message-----
> > > From: Jerin Jacob <jerinjac...@gmail.com>
> > > Sent: Monday, June 5, 2023 8:38 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 v7 04/17] graph: add get/set graph worker model
> > > APIs
> > >
> > > On Mon, Jun 5, 2023 at 4:56 PM Zhirun Yan <zhirun....@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.w...@intel.com>
> > > > Signed-off-by: Cunming Liang <cunming.li...@intel.com>
> > > > Signed-off-by: Zhirun Yan <zhirun....@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....@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.
