[v6,04/15] 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 | 54 +++++++++++++++++++++++++++++
lib/graph/rte_graph_worker_common.h | 19 ++++++++++
lib/graph/version.map | 3 ++
4 files changed, 77 insertions(+)
create mode 100644 lib/graph/rte_graph_worker.c
Comments
On Tue, May 9, 2023 at 11:34 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>
> ---
> diff --git a/lib/graph/rte_graph_worker.c b/lib/graph/rte_graph_worker.c
> new file mode 100644
> index 0000000000..cabc101262
> --- /dev/null
> +++ b/lib/graph/rte_graph_worker.c
> @@ -0,0 +1,54 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(C) 2023 Intel Corporation
> + */
> +
> +#include "rte_graph_worker_common.h"
> +
> +RTE_DEFINE_PER_LCORE(enum rte_graph_worker_model, worker_model) = RTE_GRAPH_MODEL_DEFAULT;
> +
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change, or be removed, without prior notice
> + * Set the graph worker model
Just declaring this top of the header file enough to avoid duplicating
in every functions
as all functions in header is experimental. See lib/graph/rte_graph.h
> + *
> + * @note This function does not perform any locking, and is only safe to call
> + * before graph running.
> + *
> + * @param name
> + * Name of the graph worker model.
> + *
> + * @return
> + * 0 on success, -1 otherwise.
> + */
> +int
> +rte_graph_worker_model_set(enum rte_graph_worker_model model)
> +{
> + if (model >= RTE_GRAPH_MODEL_LIST_END)
> + goto fail;
> +
> + RTE_PER_LCORE(worker_model) = model;
Application needs to set this per core . Right?
Are we anticipating a case where one core runs one model and another
core runs with another model?
If not OR it is not practically possible, then, To make application
programmer life easy,
We could loop through all lore and set on all of them instead of
application setting on each
one separately.
> + return 0;
> +
> +fail:
> + RTE_PER_LCORE(worker_model) = RTE_GRAPH_MODEL_DEFAULT;
> + return -1;
> +}
> +
> +/** Graph worker models */
> +enum rte_graph_worker_model {
> + RTE_GRAPH_MODEL_DEFAULT,
Add Doxygen comment
> + RTE_GRAPH_MODEL_RTC = RTE_GRAPH_MODEL_DEFAULT,
Add Doxygen comment to explain what this mode does.
> + RTE_GRAPH_MODEL_MCORE_DISPATCH,
Add Doxygen comment to explain what this mode does.
> + RTE_GRAPH_MODEL_LIST_END
This can break the ABI if we add one in middle. Please remove this.
See lib/crytodev for
how to handle with _END symbols.
> -----Original Message-----
> From: Jerin Jacob <jerinjacobk@gmail.com>
> Sent: Wednesday, May 24, 2023 2:09 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 04/15] graph: add get/set graph worker model APIs
>
> On Tue, May 9, 2023 at 11:34 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>
> > ---
> > diff --git a/lib/graph/rte_graph_worker.c
> > b/lib/graph/rte_graph_worker.c new file mode 100644 index
> > 0000000000..cabc101262
> > --- /dev/null
> > +++ b/lib/graph/rte_graph_worker.c
> > @@ -0,0 +1,54 @@
> > +/* SPDX-License-Identifier: BSD-3-Clause
> > + * Copyright(C) 2023 Intel Corporation */
> > +
> > +#include "rte_graph_worker_common.h"
> > +
> > +RTE_DEFINE_PER_LCORE(enum rte_graph_worker_model, worker_model) =
> > +RTE_GRAPH_MODEL_DEFAULT;
> > +
> > +/**
> > + * @warning
> > + * @b EXPERIMENTAL: this API may change, or be removed, without prior
> > +notice
> > + * Set the graph worker model
>
> Just declaring this top of the header file enough to avoid duplicating in every
> functions as all functions in header is experimental. See lib/graph/rte_graph.h
>
Got it, I will do in next version.
>
> > + *
> > + * @note This function does not perform any locking, and is only safe to call
> > + * before graph running.
> > + *
> > + * @param name
> > + * Name of the graph worker model.
> > + *
> > + * @return
> > + * 0 on success, -1 otherwise.
> > + */
> > +int
> > +rte_graph_worker_model_set(enum rte_graph_worker_model model) {
> > + if (model >= RTE_GRAPH_MODEL_LIST_END)
> > + goto fail;
> > +
> > + RTE_PER_LCORE(worker_model) = model;
>
> Application needs to set this per core . Right?
Yes. Each worker needs to know its model.
> Are we anticipating a case where one core runs one model and another core
> runs with another model?
> If not OR it is not practically possible, then, To make application programmer
> life easy, We could loop through all lore and set on all of them instead of
> application setting on each one separately.
>
For current rtc and dispatch models, it is not necessary.
To some extent that models are mutually exclusive.
For this case:
Core 1: A->B->C (RTC)
Core 2: A' (DISPATCH)
Core 3: B' (DISPATCH)
Core 4: C' (DISPATCH)
It may change the graph topo, or need some prerequisites like RSS before input node A.
BTW, if there are some requirements with more models in future, we could add some attributions for graph, lcore, node.
Like taint/affinity for node and model. Then we could allow a node to appeal/repel a set of models.
I will change to put the model into struct rte_graph as you suggested in patch 12 for this release.
>
> > + return 0;
> > +
> > +fail:
> > + RTE_PER_LCORE(worker_model) = RTE_GRAPH_MODEL_DEFAULT;
> > + return -1;
> > +}
> > +
>
> > +/** Graph worker models */
> > +enum rte_graph_worker_model {
> > + RTE_GRAPH_MODEL_DEFAULT,
>
> Add Doxygen comment
> > + RTE_GRAPH_MODEL_RTC = RTE_GRAPH_MODEL_DEFAULT,
>
>
> Add Doxygen comment to explain what this mode does.
>
>
> > + RTE_GRAPH_MODEL_MCORE_DISPATCH,
>
> Add Doxygen comment to explain what this mode does.
>
Ok, I will add Doxygen comments for these models.
> > + RTE_GRAPH_MODEL_LIST_END
>
> This can break the ABI if we add one in middle. Please remove this.
> See lib/crytodev for
> how to handle with _END symbols.
Yes, I will remove this.
@@ -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,54 @@
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright(C) 2023 Intel Corporation
+ */
+
+#include "rte_graph_worker_common.h"
+
+RTE_DEFINE_PER_LCORE(enum rte_graph_worker_model, worker_model) = RTE_GRAPH_MODEL_DEFAULT;
+
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change, or be removed, without prior notice
+ * Set the graph worker model
+ *
+ * @note This function does not perform any locking, and is only safe to call
+ * before graph running.
+ *
+ * @param name
+ * Name of the graph worker model.
+ *
+ * @return
+ * 0 on success, -1 otherwise.
+ */
+int
+rte_graph_worker_model_set(enum rte_graph_worker_model model)
+{
+ if (model >= RTE_GRAPH_MODEL_LIST_END)
+ goto fail;
+
+ RTE_PER_LCORE(worker_model) = model;
+ return 0;
+
+fail:
+ RTE_PER_LCORE(worker_model) = RTE_GRAPH_MODEL_DEFAULT;
+ return -1;
+}
+
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change, or be removed, without prior notice
+ *
+ * Get the graph worker model
+ *
+ * @param name
+ * Name of the graph worker model.
+ *
+ * @return
+ * Graph worker model on success.
+ */
+inline
+enum rte_graph_worker_model
+rte_graph_worker_model_get(void)
+{
+ return RTE_PER_LCORE(worker_model);
+}
@@ -19,6 +19,7 @@
#include <rte_compat.h>
#include <rte_common.h>
#include <rte_cycles.h>
+#include <rte_per_lcore.h>
#include <rte_prefetch.h>
#include <rte_memcpy.h>
#include <rte_memory.h>
@@ -95,6 +96,16 @@ struct rte_node {
struct rte_node *nodes[] __rte_cache_min_aligned; /**< Next nodes. */
} __rte_cache_aligned;
+/** Graph worker models */
+enum rte_graph_worker_model {
+ RTE_GRAPH_MODEL_DEFAULT,
+ RTE_GRAPH_MODEL_RTC = RTE_GRAPH_MODEL_DEFAULT,
+ RTE_GRAPH_MODEL_MCORE_DISPATCH,
+ RTE_GRAPH_MODEL_LIST_END
+};
+
+RTE_DECLARE_PER_LCORE(enum rte_graph_worker_model, worker_model);
+
/**
* @internal
*
@@ -490,6 +501,14 @@ rte_node_next_stream_move(struct rte_graph *graph, struct rte_node *src,
}
}
+__rte_experimental
+enum rte_graph_worker_model
+rte_graph_worker_model_get(void);
+
+__rte_experimental
+int
+rte_graph_worker_model_set(enum rte_graph_worker_model 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: *;
};