[v5,01/41] pipeline: add new SWX pipeline type

Message ID 20200923180645.55852-2-cristian.dumitrescu@intel.com (mailing list archive)
State Superseded, archived
Delegated to: David Marchand
Headers
Series Pipeline alignment with the P4 language |

Checks

Context Check Description
ci/checkpatch warning coding style issues

Commit Message

Cristian Dumitrescu Sept. 23, 2020, 6:06 p.m. UTC
  Add new improved Software Switch (SWX) pipeline type that supports
dynamically-defined packet headers, meta-data, actions and pipelines.
Actions and pipelines are defined through instructions.

Signed-off-by: Cristian Dumitrescu <cristian.dumitrescu@intel.com>
---
 lib/librte_pipeline/meson.build              | 10 ++-
 lib/librte_pipeline/rte_pipeline_version.map |  3 +
 lib/librte_pipeline/rte_swx_pipeline.c       | 70 +++++++++++++++++
 lib/librte_pipeline/rte_swx_pipeline.h       | 79 ++++++++++++++++++++
 4 files changed, 160 insertions(+), 2 deletions(-)
 create mode 100644 lib/librte_pipeline/rte_swx_pipeline.c
 create mode 100644 lib/librte_pipeline/rte_swx_pipeline.h
  

Comments

Stephen Hemminger Sept. 23, 2020, 6:24 p.m. UTC | #1
On Wed, 23 Sep 2020 19:06:05 +0100
Cristian Dumitrescu <cristian.dumitrescu@intel.com> wrote:

> +/*
> + * Pipeline.
> + */
> +struct rte_swx_pipeline {
> +	int build_done;
> +	int numa_node;
> +};
> +

Nit, could build_done be a bool type?

+void
+rte_swx_pipeline_free(struct rte_swx_pipeline *p)
+{
+	if (!p)
+		return;
+
+	free(p);
+}

The free() function in libc is defined to accept NULL as ok.
Please remove the if()
  
Cristian Dumitrescu Sept. 23, 2020, 6:37 p.m. UTC | #2
Hi Stephen,

> -----Original Message-----
> From: Stephen Hemminger <stephen@networkplumber.org>
> Sent: Wednesday, September 23, 2020 7:25 PM
> To: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>
> Cc: dev@dpdk.org; thomas@monjalon.net; david.marchand@redhat.com
> Subject: Re: [dpdk-dev] [PATCH v5 01/41] pipeline: add new SWX pipeline
> type
> 
> On Wed, 23 Sep 2020 19:06:05 +0100
> Cristian Dumitrescu <cristian.dumitrescu@intel.com> wrote:
> 
> > +/*
> > + * Pipeline.
> > + */
> > +struct rte_swx_pipeline {
> > +	int build_done;
> > +	int numa_node;
> > +};
> > +
> 
> Nit, could build_done be a bool type?
> 

As we discussed this in an earlier version of this patch set:
Isn't the difference between int and bool mostly cosmetic?
AFAIK we don't have a hard rule in DPDK about bool vs. int.
IMO doing this change now it likely not going to add any value.

> +void
> +rte_swx_pipeline_free(struct rte_swx_pipeline *p)
> +{
> +	if (!p)
> +		return;
> +
> +	free(p);
> +}
> 
> The free() function in libc is defined to accept NULL as ok.
> Please remove the if()

This is just the early function wrapper in patch 1 out of 41, mode code is added in this function by later patches that need the if statement. IMO this change will not add any value at all here.

Thanks,
Cristian
  
Stephen Hemminger Sept. 23, 2020, 8:23 p.m. UTC | #3
On Wed, 23 Sep 2020 18:37:19 +0000
"Dumitrescu, Cristian" <cristian.dumitrescu@intel.com> wrote:

> Hi Stephen,
> 
> > -----Original Message-----
> > From: Stephen Hemminger <stephen@networkplumber.org>
> > Sent: Wednesday, September 23, 2020 7:25 PM
> > To: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>
> > Cc: dev@dpdk.org; thomas@monjalon.net; david.marchand@redhat.com
> > Subject: Re: [dpdk-dev] [PATCH v5 01/41] pipeline: add new SWX pipeline
> > type
> > 
> > On Wed, 23 Sep 2020 19:06:05 +0100
> > Cristian Dumitrescu <cristian.dumitrescu@intel.com> wrote:
> >   
> > > +/*
> > > + * Pipeline.
> > > + */
> > > +struct rte_swx_pipeline {
> > > +	int build_done;
> > > +	int numa_node;
> > > +};
> > > +  
> > 
> > Nit, could build_done be a bool type?
> >   
> 
> As we discussed this in an earlier version of this patch set:
> Isn't the difference between int and bool mostly cosmetic?
> AFAIK we don't have a hard rule in DPDK about bool vs. int.
> IMO doing this change now it likely not going to add any value.

There is no policy and there probably doesn't need to be.
Original code was written using BSD style, and BSD
predates introduction of <stdbool.h>. Linux developers have been
favoring bool. 

Purely a human thing, compilers just treat bool == int
and allow assigning anything. Coverity or sparse might check though.

> > +void
> > +rte_swx_pipeline_free(struct rte_swx_pipeline *p)
> > +{
> > +	if (!p)
> > +		return;
> > +
> > +	free(p);
> > +}
> > 
> > The free() function in libc is defined to accept NULL as ok.
> > Please remove the if()  
> 
> This is just the early function wrapper in patch 1 out of 41, mode code is added in this function by later patches that need the if statement. IMO this change will not add any value at all here.

Sure, makes sense.
  

Patch

diff --git a/lib/librte_pipeline/meson.build b/lib/librte_pipeline/meson.build
index d70b1a023..880c2b274 100644
--- a/lib/librte_pipeline/meson.build
+++ b/lib/librte_pipeline/meson.build
@@ -1,6 +1,12 @@ 
 # SPDX-License-Identifier: BSD-3-Clause
 # Copyright(c) 2017 Intel Corporation
 
-sources = files('rte_pipeline.c', 'rte_port_in_action.c', 'rte_table_action.c')
-headers = files('rte_pipeline.h', 'rte_port_in_action.h', 'rte_table_action.h')
+sources = files('rte_pipeline.c',
+	'rte_port_in_action.c',
+	'rte_table_action.c',
+	'rte_swx_pipeline.c',)
+headers = files('rte_pipeline.h',
+	'rte_port_in_action.h',
+	'rte_table_action.h',
+	'rte_swx_pipeline.h',)
 deps += ['port', 'table', 'meter', 'sched', 'cryptodev']
diff --git a/lib/librte_pipeline/rte_pipeline_version.map b/lib/librte_pipeline/rte_pipeline_version.map
index 9ed80eb04..39593f1ee 100644
--- a/lib/librte_pipeline/rte_pipeline_version.map
+++ b/lib/librte_pipeline/rte_pipeline_version.map
@@ -55,4 +55,7 @@  EXPERIMENTAL {
 	rte_table_action_time_read;
 	rte_table_action_ttl_read;
 	rte_table_action_crypto_sym_session_get;
+	rte_swx_pipeline_config;
+	rte_swx_pipeline_build;
+	rte_swx_pipeline_free;
 };
diff --git a/lib/librte_pipeline/rte_swx_pipeline.c b/lib/librte_pipeline/rte_swx_pipeline.c
new file mode 100644
index 000000000..2319d4570
--- /dev/null
+++ b/lib/librte_pipeline/rte_swx_pipeline.c
@@ -0,0 +1,70 @@ 
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright(c) 2020 Intel Corporation
+ */
+#include <stdlib.h>
+#include <string.h>
+#include <stdio.h>
+#include <errno.h>
+
+#include <rte_common.h>
+
+#include "rte_swx_pipeline.h"
+
+#define CHECK(condition, err_code)                                             \
+do {                                                                           \
+	if (!(condition))                                                      \
+		return -(err_code);                                            \
+} while (0)
+
+#define CHECK_NAME(name, err_code)                                             \
+	CHECK((name) && (name)[0], err_code)
+
+/*
+ * Pipeline.
+ */
+struct rte_swx_pipeline {
+	int build_done;
+	int numa_node;
+};
+
+
+/*
+ * Pipeline.
+ */
+int
+rte_swx_pipeline_config(struct rte_swx_pipeline **p, int numa_node)
+{
+	struct rte_swx_pipeline *pipeline;
+
+	/* Check input parameters. */
+	CHECK(p, EINVAL);
+
+	/* Memory allocation. */
+	pipeline = calloc(1, sizeof(struct rte_swx_pipeline));
+	CHECK(pipeline, ENOMEM);
+
+	/* Initialization. */
+	pipeline->numa_node = numa_node;
+
+	*p = pipeline;
+	return 0;
+}
+
+void
+rte_swx_pipeline_free(struct rte_swx_pipeline *p)
+{
+	if (!p)
+		return;
+
+	free(p);
+}
+
+int
+rte_swx_pipeline_build(struct rte_swx_pipeline *p)
+{
+	CHECK(p, EINVAL);
+	CHECK(p->build_done == 0, EEXIST);
+
+	p->build_done = 1;
+	return 0;
+}
diff --git a/lib/librte_pipeline/rte_swx_pipeline.h b/lib/librte_pipeline/rte_swx_pipeline.h
new file mode 100644
index 000000000..ded26a4e4
--- /dev/null
+++ b/lib/librte_pipeline/rte_swx_pipeline.h
@@ -0,0 +1,79 @@ 
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright(c) 2020 Intel Corporation
+ */
+#ifndef __INCLUDE_RTE_SWX_PIPELINE_H__
+#define __INCLUDE_RTE_SWX_PIPELINE_H__
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+/**
+ * @file
+ * RTE SWX Pipeline
+ */
+
+#include <stddef.h>
+#include <stdint.h>
+
+#include <rte_compat.h>
+
+/*
+ * Pipeline setup and operation
+ */
+
+/** Pipeline opaque data structure. */
+struct rte_swx_pipeline;
+
+/**
+ * Pipeline configure
+ *
+ * @param[out] p
+ *   Pipeline handle. Must point to valid memory. Contains valid pipeline handle
+ *   when the function returns successfully.
+ * @param[in] numa_node
+ *   Non-Uniform Memory Access (NUMA) node.
+ * @return
+ *   0 on success or the following error codes otherwise:
+ *   -EINVAL: Invalid argument;
+ *   -ENOMEM: Not enough space/cannot allocate memory.
+ */
+__rte_experimental
+int
+rte_swx_pipeline_config(struct rte_swx_pipeline **p,
+			int numa_node);
+
+/**
+ * Pipeline build
+ *
+ * Once called, the pipeline build operation marks the end of pipeline
+ * configuration. At this point, all the internal data structures needed to run
+ * the pipeline are built.
+ *
+ * @param[in] p
+ *   Pipeline handle.
+ * @return
+ *   0 on success or the following error codes otherwise:
+ *   -EINVAL: Invalid argument;
+ *   -ENOMEM: Not enough space/cannot allocate memory;
+ *   -EEXIST: Pipeline was already built successfully.
+ */
+__rte_experimental
+int
+rte_swx_pipeline_build(struct rte_swx_pipeline *p);
+
+/**
+ * Pipeline free
+ *
+ * @param[in] p
+ *   Pipeline handle.
+ */
+__rte_experimental
+void
+rte_swx_pipeline_free(struct rte_swx_pipeline *p);
+
+#ifdef __cplusplus
+}
+#endif
+
+#endif