RE: [PATCH v6 07/15] graph: introduce graph clone API for other worker core

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

> -----Original Message-----
> From: Jerin Jacob <jerinjacobk@gmail.com>
> Sent: Wednesday, May 24, 2023 3:14 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>
> Subject: Re: [PATCH v6 07/15] graph: introduce graph clone API for other worker
> core
> 
> On Tue, May 9, 2023 at 11:35 AM Zhirun Yan <zhirun.yan@intel.com> wrote:
> >
> > This patch adds graph API for supporting to clone the graph object for
> > a specified worker core. The new graph will also clone all nodes.
> >
> > 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>
> > ---
> > +
> > +static rte_graph_t
> > +graph_clone(struct graph *parent_graph, const char *name) {
> > +       struct graph_node *graph_node;
> > +       struct graph *graph;
> > +
> > +       graph_spinlock_lock();
> > +
> > +       /* Don't allow to clone a node from a cloned graph */
> 
> Both clone_name() and graph_clone() kind of duplicate rte_node_clone(), Please
> check, Can we have common _private_ function to reuse just the common code
> between them


rte_graph_clone(), graph_clone() in graph.c and rte_node_clone(), node_clone() in node.c are different.
They are specific for node or graph.
I think they could remain the same.

Only clone_name(struct rte_node_register *reg, struct node *node, const char *name)
could be reuse.

And I will change the param to clone_name(char *name, char* ori_str, const char *append_str) and put
this func into graph_private.h
 

> 
> 
> > +
> >  rte_graph_t
> >  rte_graph_from_name(const char *name)  { diff --git
> > a/lib/graph/graph_private.h b/lib/graph/graph_private.h index
> > f63b339d81..52ca30ed56 100644
> > --- a/lib/graph/graph_private.h
> > +++ b/lib/graph/graph_private.h
> > @@ -99,6 +99,8 @@ struct graph {
> >         /**< Circular buffer mask for wrap around. */
> >         rte_graph_t id;
> >         /**< Graph identifier. */
> > +       rte_graph_t parent_id;
> > +       /**< Parent graph identifier. */
> >         unsigned int lcore_id;
> >         /**< Lcore identifier where the graph prefer to run on. */
> >         size_t mem_sz;
> > diff --git a/lib/graph/rte_graph.h b/lib/graph/rte_graph.h index
> > c523809d1f..2f86c17de7 100644
> > --- a/lib/graph/rte_graph.h
> > +++ b/lib/graph/rte_graph.h
> > @@ -247,6 +247,26 @@ rte_graph_t rte_graph_create(const char *name,
> > struct rte_graph_param *prm);  __rte_experimental  int
> > rte_graph_destroy(rte_graph_t id);
> >
> > +/**
> > + * Clone Graph.
> > + *
> > + * Clone a graph from static graph (graph created from
> > +rte_graph_create). And
> 
> rte_graph_create->rte_graph_create()
> 
> > + * all cloned graphs attached to the parent graph MUST be destroyed
> > + together
> 
> 
> Can we add reference count in the implementation to avoid this limition. If this
> too much for this release, we can try to add next release

Yes, I think it's good in next release. Thanks.

> 
> > + * for fast schedule design limitation (stop ALL graph walk firstly).
> > + *
> > + * @param id
> > + *   Static graph id to clone from.
> > + * @param name
> > + *   Name of the new graph. The library prepends the parent graph name to
> the
> > + * user-specified name. The final graph name will be,
> > + * "parent graph name" + "-" + name.
> > + *
> > + * @return
> > + *   Valid graph id on success, RTE_GRAPH_ID_INVALID otherwise.
> > + */
> > +__rte_experimental
> > +rte_graph_t rte_graph_clone(rte_graph_t id, const char *name);
> > +
> >  /**
> >   * Get graph id from graph name.
> >   *
> > diff --git a/lib/graph/version.map b/lib/graph/version.map index
> > 7de6f08f59..aaa86f66ed 100644
> > --- a/lib/graph/version.map
> > +++ b/lib/graph/version.map
> > @@ -7,6 +7,7 @@ EXPERIMENTAL {
> >
> >         rte_graph_create;
> >         rte_graph_destroy;
> > +       rte_graph_clone;
> 
> 
> Found new generic graph API, Please add test case for it at
> app/test/test_graph.c.
> 
Yes, I will add test.
> 
> 
> >         rte_graph_dump;
> >         rte_graph_export;
> >         rte_graph_from_name;
> > --
> > 2.37.2
> >