Re: [PATCH v6 05/15] graph: introduce graph node core affinity API

[Jerin Jacob <jerinjacobk@gmail.com>]

On Tue, May 9, 2023 at 11:34 AM Zhirun Yan <zhirun.yan@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.wang@intel.com>
> Signed-off-by: Cunming Liang <cunming.liang@intel.com>
> Signed-off-by: Zhirun Yan <zhirun.yan@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.*

>  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.


>         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.

> +
> +       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

> +#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.


> + *
> + * @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

> +
>         local: *;
>  };
> --
> 2.37.2
>