> -----Original Message-----
> From: Jerin Jacob <jerinjac...@gmail.com>
> Sent: Wednesday, May 24, 2023 2:36 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>
> Subject: Re: [PATCH v6 05/15] graph: introduce graph node core affinity API
>
> On Tue, May 9, 2023 at 11:34 AM Zhirun Yan <zhirun....@intel.com> wrote:
> >
> > Add lcore_id for node to hold affinity core id and impl
> > rte_graph_model_dispatch_lcore_affinity_set to set node affinity with
> > specific lcore.
> >
> > 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/graph_private.h | 1 +
> > lib/graph/meson.build | 1 +
> > lib/graph/node.c | 1 +
> > lib/graph/rte_graph_model_dispatch.c | 30 +++++++++++++++++++
> > lib/graph/rte_graph_model_dispatch.h | 43 ++++++++++++++++++++++++++++
>
> Could you change all the new model file to prefix _mcore_ like below to align
> with enum name.
> rte_graph_model_mcore_dispatch.*
Yes, I will do in next version.
>
> > lib/graph/version.map | 2 ++
> > 6 files changed, 78 insertions(+)
> > create mode 100644 lib/graph/rte_graph_model_dispatch.c
> > create mode 100644 lib/graph/rte_graph_model_dispatch.h
> >
> > diff --git a/lib/graph/graph_private.h b/lib/graph/graph_private.h
> > index eacdef45f0..bd4c576324 100644
> > --- a/lib/graph/graph_private.h
> > +++ b/lib/graph/graph_private.h
> > @@ -51,6 +51,7 @@ struct node {
> > STAILQ_ENTRY(node) next; /**< Next node in the list. */
> > char name[RTE_NODE_NAMESIZE]; /**< Name of the node. */
> > uint64_t flags; /**< Node configuration flag. */
> > + unsigned int lcore_id; /**< Node runs on the Lcore ID */
>
> Could you move to end of existing fast path variables. Also, please extend the
> comments for new variable introduced ONLY for dispatch model. Comment
> should express it is only for dispatch model.
>
Got it, will do in next version.
>
> > rte_node_process_t process; /**< Node process function. */
> > rte_node_init_t init; /**< Node init function. */
> > rte_node_fini_t fini; /**< Node fini function. */
> > diff --git a/lib/graph/meson.build b/lib/graph/meson.build index
> > 9fab8243da..c729d984b6 100644
> > --- a/lib/graph/meson.build
> > +++ b/lib/graph/meson.build
> > @@ -16,6 +16,7 @@ sources = files(
> > 'graph_populate.c',
> > 'graph_pcap.c',
> > 'rte_graph_worker.c',
> > + 'rte_graph_model_dispatch.c',
> > )
> > headers = files('rte_graph.h', 'rte_graph_worker.h')
> >
> > diff --git a/lib/graph/node.c b/lib/graph/node.c index
> > 149414dcd9..339b4a0da5 100644
> > --- a/lib/graph/node.c
> > +++ b/lib/graph/node.c
> > @@ -100,6 +100,7 @@ __rte_node_register(const struct rte_node_register
> *reg)
> > goto free;
> > }
> >
> > + node->lcore_id = RTE_MAX_LCORE;
> > node->id = node_id++;
> >
> > /* Add the node at tail */
> > diff --git a/lib/graph/rte_graph_model_dispatch.c
> > b/lib/graph/rte_graph_model_dispatch.c
> > new file mode 100644
> > index 0000000000..3364a76ed4
> > --- /dev/null
> > +++ b/lib/graph/rte_graph_model_dispatch.c
> > @@ -0,0 +1,30 @@
> > +/* SPDX-License-Identifier: BSD-3-Clause
> > + * Copyright(C) 2023 Intel Corporation */
> > +
> > +#include "graph_private.h"
> > +#include "rte_graph_model_dispatch.h"
> > +
> > +int
> > +rte_graph_model_dispatch_lcore_affinity_set(const char *name,
> > +unsigned int lcore_id) {
> > + struct node *node;
> > + int ret = -EINVAL;
>
>
> Shouldn't we need to check the is model is dispatch before proceeding?
> If so, same comment applicable for all function created solely for new model.
>
> Also, in case some _exiting_ API not valid for dispatch model, please add the
> check for the same.
Yes, I will add check for all function if it is only used by new model.
Actually, most existing API are valid, I will double check.
>
> > +
> > + if (lcore_id >= RTE_MAX_LCORE)
> > + return ret;
> > +
> > + graph_spinlock_lock();
> > +
> > + STAILQ_FOREACH(node, node_list_head_get(), next) {
> > + if (strncmp(node->name, name, RTE_NODE_NAMESIZE) == 0) {
> > + node->lcore_id = lcore_id;
> > + ret = 0;
> > + break;
> > + }
> > + }
> > +
> > + graph_spinlock_unlock();
> > +
> > + return ret;
> > +}
> > diff --git a/lib/graph/rte_graph_model_dispatch.h
> > b/lib/graph/rte_graph_model_dispatch.h
> > new file mode 100644
> > index 0000000000..179624e972
> > --- /dev/null
> > +++ b/lib/graph/rte_graph_model_dispatch.h
> > @@ -0,0 +1,43 @@
> > +/* SPDX-License-Identifier: BSD-3-Clause
> > + * Copyright(C) 2023 Intel Corporation */
> > +
> > +#ifndef _RTE_GRAPH_MODEL_DISPATCH_H_
> > +#define _RTE_GRAPH_MODEL_DISPATCH_H_
>
> _RTE_GRAPH_MODEL_MCORE_DISPATCH_H_
>
> > +
> > +/**
> > + * @file rte_graph_model_dispatch.h
> > + *
> > + * @warning
> > + * @b EXPERIMENTAL:
> > + * All functions in this file may be changed or removed without prior
> > notice.
> > + *
> > + * This API allows to set core affinity with the node.
> > + */
> > +#include "rte_graph_worker_common.h"
>
> Move this under below "extern "C
>
Yes.
> > +#ifdef __cplusplus
> > +extern "C" {
> > +#endif
> > +
> > +/**
> > + * Set lcore affinity with the node.
>
> Please change API description for all the API introduced for dispatch model to
> explicitly mention, it is valid only for dispatch model.
>
> Something like, "Set lcore affinity with the node for dispatch model" or so.
Got it.
>
>
> > + *
> > + * @param name
> > + * Valid node name. In the case of the cloned node, the name will be
> > + * "parent node name" + "-" + name.
> > + * @param lcore_id
> > + * The lcore ID value.
> > + *
> > + * @return
> > + * 0 on success, error otherwise.
> > + */
> > +__rte_experimental
> > +int rte_graph_model_dispatch_lcore_affinity_set(const char *name,
> > + unsigned int
> > +lcore_id);
> > +
> > +#ifdef __cplusplus
> > +}
> > +#endif
> > +
> > +#endif /* _RTE_GRAPH_MODEL_DISPATCH_H_ */
> > diff --git a/lib/graph/version.map b/lib/graph/version.map index
> > eea73ec9ca..1f090be74e 100644
> > --- a/lib/graph/version.map
> > +++ b/lib/graph/version.map
> > @@ -46,5 +46,7 @@ EXPERIMENTAL {
> > rte_graph_worker_model_set;
> > rte_graph_worker_model_get;
> >
> > + rte_graph_model_dispatch_lcore_affinity_set;
>
> Could you change all the new model API to prefix _mcore_ like below to align
> with enum name.
>
> rte_graph_model_mcore_dispatch_lcore_affinity_set
Yes, will do in next version.
>
> > +
> > local: *;
> > };
> > --
> > 2.37.2
> >
