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

["Yan, Zhirun" <zhirun.yan@intel.com>]

> -----Original Message-----
> From: Jerin Jacob <jerinjacobk@gmail.com>
> Sent: Wednesday, June 7, 2023 3:43 PM
> To: Yan, Zhirun <zhirun.yan@intel.com>
> Cc: dev@dpdk.org; jerinj@marvell.com; kirankumark@marvell.com;
> ndabilpuram@marvell.com; stephen@networkplumber.org;
> pbhagavatula@marvell.com; Liang, Cunming <cunming.liang@intel.com>; Wang,
> Haiyue <haiyue.wang@intel.com>; mattias.ronnblom
> <mattias.ronnblom@ericsson.com>
> Subject: Re: [PATCH v9 04/17] graph: add get/set graph worker model APIs
> 
> On Wed, Jun 7, 2023 at 9:30 AM Zhirun Yan <zhirun.yan@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@intel.com>
> > Signed-off-by: Cunming Liang <cunming.liang@intel.com>
> > Signed-off-by: Zhirun Yan <zhirun.yan@intel.com>
> 
> >
> > +/** Graph worker models */
> > +/* If adding new entry, then update graph_model_is_valid API. */
> 
> When adding new model entry, update rte_graph_model_is_valid API logic
> 
Got it.

> > +#define RTE_GRAPH_MODEL_RTC 0 /**< Run-To-Completion model. It is the
> > +default model. */ #define RTE_GRAPH_MODEL_DEFAULT
> RTE_GRAPH_MODEL_RTC
> > +/**< Default graph model. */
> 
> This line can come after RTE_GRAPH_MODEL_MCORE_DISPATCH
> 
Ok.

> > +#define RTE_GRAPH_MODEL_MCORE_DISPATCH 1 /**< Dispatch model to
> > +support cross-core dispatching within core affinity. */
> > +
> >  /**
> >   * @internal
> >   *
> > @@ -41,6 +48,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. */
> > +       uint32_t model;              /**< graph model */
> 
> uin8_t is enough and add uint8_t and uint16_t reserved. So that this fastpath
> area can be used in future as needed.
> 
I agree.  Thanks.
> 
> >         rte_graph_t id; /**< Graph identifier. */
> >         int socket;     /**< Socket ID where memory is allocated. */
> >         char name[RTE_GRAPH_NAMESIZE];  /**< Name of the graph. */ @@
> > -490,6 +498,59 @@ rte_node_next_stream_move(struct rte_graph *graph,
> struct rte_node *src,
> >         }
> >  }
> >
> > +/**
> > + * Test the validity of model.
> > + *
> > + * @param id
> > + *   Node id to check.
> 
> It is not node id
> 
Will change it to model.

> > + *
> > + * @return
> > + *   true if graph model is valid, false otherwise.
> > + */
> > +static __rte_always_inline
> > +bool
> > +graph_model_is_valid(uint32_t model)
> 
> Public API in header file, use rte_graph_...
> Also, implementation can go to .c file. See below comment for no_check version.
> 
Ok. I will move it into rte_graph_worker.c.
> 
> 
> > +{
> > +       if (model > RTE_GRAPH_MODEL_MCORE_DISPATCH)
> > +               return false;
> > +
> > +       return true;
> > +}
> > +
> > +/**
> > + * @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 model
> > + *   Name of the graph worker model.
> > + *
> > + * @return
> > + *   0 on success, -1 otherwise.
> > + */
> > +__rte_experimental
> > +int rte_graph_worker_model_set(uint32_t model);
> > +
> > +/**
> > + * Get the graph worker model
> > + *
> > + * @note All graph will use the same model and this function will get
> > +model from the first one
> > + *
> > + * @param graph
> > + *   Graph pointer.
> > + *
> > + * @return
> > + *   Graph worker model on success.
> > + */
> > +__rte_experimental
> > +static inline uint32_t
> > +rte_graph_worker_model_get(struct rte_graph *graph) {
> > +       if (!graph_model_is_valid(graph->model))
> > +               return -EINVAL;
> 
> Introduce rte_graph_worker_model_no_check_get() to skip this check to use
> with fastpath.
> 
> rte_graph_worker_model_get can move to .c file.

Yes. Will move in next version.
Got it. rte_graph_worker_model_no_check_get() will be used in fast path.
Actually, I don’t find the performance impact about static inline, so should the new
API declared with static inline keywords or put it into .c file also?

> 
> > +
> > +       return graph->model;
> > +}
> 
> 
> > +
> >  #ifdef __cplusplus
> >  }
> >  #endif
> > diff --git a/lib/graph/version.map b/lib/graph/version.map index
> > 13b838752d..eea73ec9ca 100644
> > --- a/lib/graph/version.map
> > +++ b/lib/graph/version.map
> > @@ -43,5 +43,8 @@ EXPERIMENTAL {
> >         rte_node_next_stream_put;
> >         rte_node_next_stream_move;
> >
> > +       rte_graph_worker_model_set;
> > +       rte_graph_worker_model_get;
> 
> Add rte_graph_model_is_valid() in next verion.
Yes.
> 
> 
> > +
> >         local: *;
> >  };
> > --
> > 2.37.2
> >