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

[Yan, Zhirun]

> -----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;
  return -EINVAL;
}
STAILQ_FOREACH(graph, graph_head, next)
    graph->graph->model = 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,
should not in fastpath. If different models coexisted, then the graph object must be passed.

I will change it. Thanks.

> > +
> > +       return graph->graph->model;
> 
> > +}
> > diff --git a/lib/graph/rte_graph_worker_common.h
> > b/lib/graph/rte_graph_worker_common.h
> > index 41428974db..72d132bae4 100644
> > --- a/lib/graph/rte_graph_worker_common.h
> > +++ b/lib/graph/rte_graph_worker_common.h
> > @@ -29,6 +29,16 @@
> >  extern "C" {
> >  #endif
> >
> > +/** Graph worker models */
> > +enum rte_graph_worker_model {
> > +       RTE_GRAPH_MODEL_DEFAULT,
> > +       /**< Default graph model*/
> > +       RTE_GRAPH_MODEL_RTC = RTE_GRAPH_MODEL_DEFAULT,
> > +       /**< Run-To-Completion model. It is the default model. */
> > +       RTE_GRAPH_MODEL_MCORE_DISPATCH
> > +       /**< Dispatch model to support cross-core dispatching within
> > +core affinity. */ };
> > +
> >  /**
> >   * @internal
> >   *
> > @@ -50,6 +60,7 @@ struct rte_graph {
> >         /** Number of packets to capture per core. */
> >         uint64_t nb_pkt_to_capture;
> >         char pcap_filename[RTE_GRAPH_PCAP_FILE_SZ];  /**< Pcap
> > filename. */
> > +       enum rte_graph_worker_model model; /**< graph model */
> 
> Used in fastpath, use in fastpath area
> [main]dell[dpdk.org] $ git diff
> diff --git a/lib/graph/rte_graph_worker.h b/lib/graph/rte_graph_worker.h index
> 438595b15c..462bbfa705 100644
> --- a/lib/graph/rte_graph_worker.h
> +++ b/lib/graph/rte_graph_worker.h
> @@ -41,6 +41,7 @@ struct rte_graph {
>         rte_node_t nb_nodes;         /**< Number of nodes in the graph. */
>         rte_graph_off_t *cir_start;  /**< Pointer to circular buffer. */
>         rte_graph_off_t nodes_start; /**< Offset at which node memory starts. */
> +       enum rte_graph_worker_model  model;
>         rte_graph_t id; /**< Graph identifier. */
>         int socket;     /**< Socket ID where memory is allocated. */
>         char name[RTE_GRAPH_NAMESIZE];  /**< Name of the graph. */
> 
> >         uint64_t fence;                 /**< Fence. */
> >  } __rte_cache_aligned;
> >
> > @@ -490,6 +501,14 @@ rte_node_next_stream_move(struct rte_graph
> *graph, struct rte_node *src,
> >         }
> >  }
> >
> > +__rte_experimental
> > +enum rte_graph_worker_model
> > +rte_graph_worker_model_get(void);
> > +
> > +__rte_experimental
> > +int
> > +rte_graph_worker_model_set(enum rte_graph_worker_model model);
> 
> 
> Add proper Doxygen comment for both.
> 
> Also check the build issue at
> http://mails.dpdk.org/archives/test-report/2023-June/404496.html

Got it, will fix in next version. Thanks.