[v9,04/17] graph: add get/set graph worker model APIs
Checks
Commit Message
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>
---
lib/graph/meson.build | 1 +
lib/graph/rte_graph_worker.c | 21 ++++++++++
lib/graph/rte_graph_worker_common.h | 61 +++++++++++++++++++++++++++++
lib/graph/version.map | 3 ++
4 files changed, 86 insertions(+)
create mode 100644 lib/graph/rte_graph_worker.c
Comments
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
> +#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
> +#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.
> 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
> + *
> + * @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.
> +{
> + 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.
> +
> + 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.
> +
> local: *;
> };
> --
> 2.37.2
>
> -----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
> >
On Wed, Jun 7, 2023 at 5:55 PM Yan, Zhirun <zhirun.yan@intel.com> wrote:
>
> > > +__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
May be due to burst size 32 or 256.it will start impacting if burst
size is less.
> API declared with static inline keywords or put it into .c file also?
Keep in line fastpath function in .h as inline.
> -----Original Message-----
> From: Jerin Jacob <jerinjacobk@gmail.com>
> Sent: Wednesday, June 7, 2023 9:29 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 5:55 PM Yan, Zhirun <zhirun.yan@intel.com> wrote:
> >
> > > > +__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
>
> May be due to burst size 32 or 256.it will start impacting if burst size is less.
>
In my test, the impact still small.
But I understand your point. If the walk() be called much more, it will call much get().
The burst size will impact the node->obj[] size, and then cause much walk loop. Thanks.
>
> > API declared with static inline keywords or put it into .c file also?
>
> Keep in line fastpath function in .h as inline.
The declare of inline is just a suggestion for compiler. The compiler will decide to inline or not.
Got it. I will change in next version.
@@ -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')
new file mode 100644
@@ -0,0 +1,21 @@
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright(C) 2023 Intel Corporation
+ */
+
+#include "rte_graph_worker_common.h"
+#include "graph_private.h"
+
+int
+rte_graph_worker_model_set(uint32_t model)
+{
+ struct graph_head *graph_head = graph_list_head_get();
+ struct graph *graph;
+
+ if (!graph_model_is_valid(model))
+ return -EINVAL;
+
+ STAILQ_FOREACH(graph, graph_head, next)
+ graph->graph->model = model;
+
+ return 0;
+}
@@ -29,6 +29,13 @@
extern "C" {
#endif
+/** Graph worker models */
+/* If adding new entry, then update graph_model_is_valid API. */
+#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. */
+#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 */
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.
+ *
+ * @return
+ * true if graph model is valid, false otherwise.
+ */
+static __rte_always_inline
+bool
+graph_model_is_valid(uint32_t model)
+{
+ 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;
+
+ return graph->model;
+}
+
#ifdef __cplusplus
}
#endif
@@ -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;
+
local: *;
};