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?
> return -EINVAL;
> }
> STAILQ_FOREACH(graph, graph_head, next)
> graph->graph->model = model;
graph->model = model. Right?
>
> 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/
> 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.
