Re: [ovs-dev] [v13 02/12] dpif-netdev: Split HWOL out to own header file.
Hi Flavio,
Thanks for the review. My comments are inline,
Cian
> -Original Message-
> From: Flavio Leitner
> Sent: Friday 18 June 2021 13:12
> To: Ferriter, Cian
> Cc: ovs-dev@openvswitch.org; i.maxim...@ovn.org
> Subject: Re: [ovs-dev] [v13 02/12] dpif-netdev: Split HWOL out to own header
> file.
>
>
> Hello,
>
> I have some comments inline.
>
> On Thu, Jun 17, 2021 at 05:18:15PM +0100, Cian Ferriter wrote:
> > From: Harry van Haaren
> >
> > This commit moves the datapath lookup functions required for
> > hardware offload to a seperate file. This allows other DPIF
>
>
> Spelling error.
>
Good spot, I'll fix this in the next version.
> > implementations to access the lookup functions, encouraging
> > code reuse.
> >
> > Signed-off-by: Harry van Haaren
> >
> > ---
> >
> > Cc: Gaetan Rivet
> > Cc: Sriharsha Basavapatna
> >
> > v13:
> > - Minor code refactor to address review comments.
> > ---
> > lib/automake.mk| 1 +
> > lib/dpif-netdev-private-hwol.h | 63 ++
> > lib/dpif-netdev.c | 38 ++--
> > 3 files changed, 66 insertions(+), 36 deletions(-)
> > create mode 100644 lib/dpif-netdev-private-hwol.h
> >
> > diff --git a/lib/automake.mk b/lib/automake.mk
> > index fdba3c6c0..3a33cdd5c 100644
> > --- a/lib/automake.mk
> > +++ b/lib/automake.mk
> > @@ -115,6 +115,7 @@ lib_libopenvswitch_la_SOURCES = \
> > lib/dpif-netdev-private-dfc.h \
> > lib/dpif-netdev-private-dpcls.h \
> > lib/dpif-netdev-private-flow.h \
> > + lib/dpif-netdev-private-hwol.h \
> > lib/dpif-netdev-private-thread.h \
> > lib/dpif-netdev-private.h \
> > lib/dpif-netdev-perf.c \
> > diff --git a/lib/dpif-netdev-private-hwol.h b/lib/dpif-netdev-private-hwol.h
> > new file mode 100644
> > index 0..b93297a74
> > --- /dev/null
> > +++ b/lib/dpif-netdev-private-hwol.h
> > @@ -0,0 +1,63 @@
> > +/*
> > + * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013, 2015 Nicira, Inc.
> > + * Copyright (c) 2021 Intel Corporation.
> > + *
> > + * Licensed under the Apache License, Version 2.0 (the "License");
> > + * you may not use this file except in compliance with the License.
> > + * You may obtain a copy of the License at:
> > + *
> > + * http://www.apache.org/licenses/LICENSE-2.0
> > + *
> > + * Unless required by applicable law or agreed to in writing, software
> > + * distributed under the License is distributed on an "AS IS" BASIS,
> > + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> > + * See the License for the specific language governing permissions and
> > + * limitations under the License.
> > + */
> > +
> > +#ifndef DPIF_NETDEV_PRIVATE_HWOL_H
> > +#define DPIF_NETDEV_PRIVATE_HWOL_H 1
> > +
> > +#include "dpif-netdev-private-flow.h"
> > +
> > +#define MAX_FLOW_MARK (UINT32_MAX - 1)
> > +#define INVALID_FLOW_MARK 0
> > +/* Zero flow mark is used to indicate the HW to remove the mark. A packet
> > + * marked with zero mark is received in SW without a mark at all, so it
> > + * cannot be used as a valid mark.
> > + */
> > +
> > +struct megaflow_to_mark_data {
> > +const struct cmap_node node;
> > +ovs_u128 mega_ufid;
> > +uint32_t mark;
> > +};
> > +
> > +struct flow_mark {
> > +struct cmap megaflow_to_mark;
> > +struct cmap mark_to_flow;
> > +struct id_pool *pool;
> > +};
> > +
> > +/* allocated in dpif-netdev.c */
> > +extern struct flow_mark flow_mark;
> > +
> > +static inline struct dp_netdev_flow *
> > +mark_to_flow_find(const struct dp_netdev_pmd_thread *pmd,
> > + const uint32_t mark)
> > +{
> > +struct dp_netdev_flow *flow;
> > +
> > +CMAP_FOR_EACH_WITH_HASH (flow, mark_node, hash_int(mark, 0),
> > + _mark.mark_to_flow) {
> > +if (flow->mark == mark && flow->pmd_id == pmd->core_id &&
> > +flow->dead == false) {
> > +return flow;
> > +}
> > +}
> > +
> > +return NULL;
> > +}
>
> Wouldn't this be better in a separate .c file? Because although the
> structure flow_mark is here, it is allocated in dpif-netdev.c and
> we have a fairly large function inline. This seems enough to start
Re: [ovs-dev] [v13 02/12] dpif-netdev: Split HWOL out to own header file.
Hello,
I have some comments inline.
On Thu, Jun 17, 2021 at 05:18:15PM +0100, Cian Ferriter wrote:
> From: Harry van Haaren
>
> This commit moves the datapath lookup functions required for
> hardware offload to a seperate file. This allows other DPIF
Spelling error.
> implementations to access the lookup functions, encouraging
> code reuse.
>
> Signed-off-by: Harry van Haaren
>
> ---
>
> Cc: Gaetan Rivet
> Cc: Sriharsha Basavapatna
>
> v13:
> - Minor code refactor to address review comments.
> ---
> lib/automake.mk| 1 +
> lib/dpif-netdev-private-hwol.h | 63 ++
> lib/dpif-netdev.c | 38 ++--
> 3 files changed, 66 insertions(+), 36 deletions(-)
> create mode 100644 lib/dpif-netdev-private-hwol.h
>
> diff --git a/lib/automake.mk b/lib/automake.mk
> index fdba3c6c0..3a33cdd5c 100644
> --- a/lib/automake.mk
> +++ b/lib/automake.mk
> @@ -115,6 +115,7 @@ lib_libopenvswitch_la_SOURCES = \
> lib/dpif-netdev-private-dfc.h \
> lib/dpif-netdev-private-dpcls.h \
> lib/dpif-netdev-private-flow.h \
> + lib/dpif-netdev-private-hwol.h \
> lib/dpif-netdev-private-thread.h \
> lib/dpif-netdev-private.h \
> lib/dpif-netdev-perf.c \
> diff --git a/lib/dpif-netdev-private-hwol.h b/lib/dpif-netdev-private-hwol.h
> new file mode 100644
> index 0..b93297a74
> --- /dev/null
> +++ b/lib/dpif-netdev-private-hwol.h
> @@ -0,0 +1,63 @@
> +/*
> + * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013, 2015 Nicira, Inc.
> + * Copyright (c) 2021 Intel Corporation.
> + *
> + * Licensed under the Apache License, Version 2.0 (the "License");
> + * you may not use this file except in compliance with the License.
> + * You may obtain a copy of the License at:
> + *
> + * http://www.apache.org/licenses/LICENSE-2.0
> + *
> + * Unless required by applicable law or agreed to in writing, software
> + * distributed under the License is distributed on an "AS IS" BASIS,
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> +
> +#ifndef DPIF_NETDEV_PRIVATE_HWOL_H
> +#define DPIF_NETDEV_PRIVATE_HWOL_H 1
> +
> +#include "dpif-netdev-private-flow.h"
> +
> +#define MAX_FLOW_MARK (UINT32_MAX - 1)
> +#define INVALID_FLOW_MARK 0
> +/* Zero flow mark is used to indicate the HW to remove the mark. A packet
> + * marked with zero mark is received in SW without a mark at all, so it
> + * cannot be used as a valid mark.
> + */
> +
> +struct megaflow_to_mark_data {
> +const struct cmap_node node;
> +ovs_u128 mega_ufid;
> +uint32_t mark;
> +};
> +
> +struct flow_mark {
> +struct cmap megaflow_to_mark;
> +struct cmap mark_to_flow;
> +struct id_pool *pool;
> +};
> +
> +/* allocated in dpif-netdev.c */
> +extern struct flow_mark flow_mark;
> +
> +static inline struct dp_netdev_flow *
> +mark_to_flow_find(const struct dp_netdev_pmd_thread *pmd,
> + const uint32_t mark)
> +{
> +struct dp_netdev_flow *flow;
> +
> +CMAP_FOR_EACH_WITH_HASH (flow, mark_node, hash_int(mark, 0),
> + _mark.mark_to_flow) {
> +if (flow->mark == mark && flow->pmd_id == pmd->core_id &&
> +flow->dead == false) {
> +return flow;
> +}
> +}
> +
> +return NULL;
> +}
Wouldn't this be better in a separate .c file? Because although the
structure flow_mark is here, it is allocated in dpif-netdev.c and
we have a fairly large function inline. This seems enough to start
a .c file to me.
Thanks,
fbl
> +
> +
> +#endif /* dpif-netdev-private-hwol.h */
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index affeeacdc..e913f4efc 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -18,6 +18,7 @@
> #include "dpif-netdev.h"
> #include "dpif-netdev-private.h"
> #include "dpif-netdev-private-dfc.h"
> +#include "dpif-netdev-private-hwol.h"
>
> #include
> #include
> @@ -1953,26 +1954,8 @@ dp_netdev_pmd_find_dpcls(struct dp_netdev_pmd_thread
> *pmd,
> return cls;
> }
>
> -#define MAX_FLOW_MARK (UINT32_MAX - 1)
> -#define INVALID_FLOW_MARK 0
> -/* Zero flow mark is used to indicate the HW to remove the mark. A packet
> - * marked with zero mark is received in SW without a mark at all, so it
> - * cannot be used as a valid mark.
> - */
>
> -struct megaflow_to_mark_data {
> -const struct cmap_node node;
> -ovs_u128 mega_ufid;
> -uint32_t mark;
> -};
> -
> -struct flow_mark {
> -struct cmap megaflow_to_mark;
> -struct cmap mark_to_flow;
> -struct id_pool *pool;
> -};
> -
> -static struct flow_mark flow_mark = {
> +struct flow_mark flow_mark = {
> .megaflow_to_mark = CMAP_INITIALIZER,
> .mark_to_flow = CMAP_INITIALIZER,
> };
> @@ -2141,23 +2124,6 @@ flow_mark_flush(struct dp_netdev_pmd_thread *pmd)
[ovs-dev] [v13 02/12] dpif-netdev: Split HWOL out to own header file.
From: Harry van Haaren
This commit moves the datapath lookup functions required for
hardware offload to a seperate file. This allows other DPIF
implementations to access the lookup functions, encouraging
code reuse.
Signed-off-by: Harry van Haaren
---
Cc: Gaetan Rivet
Cc: Sriharsha Basavapatna
v13:
- Minor code refactor to address review comments.
---
lib/automake.mk| 1 +
lib/dpif-netdev-private-hwol.h | 63 ++
lib/dpif-netdev.c | 38 ++--
3 files changed, 66 insertions(+), 36 deletions(-)
create mode 100644 lib/dpif-netdev-private-hwol.h
diff --git a/lib/automake.mk b/lib/automake.mk
index fdba3c6c0..3a33cdd5c 100644
--- a/lib/automake.mk
+++ b/lib/automake.mk
@@ -115,6 +115,7 @@ lib_libopenvswitch_la_SOURCES = \
lib/dpif-netdev-private-dfc.h \
lib/dpif-netdev-private-dpcls.h \
lib/dpif-netdev-private-flow.h \
+ lib/dpif-netdev-private-hwol.h \
lib/dpif-netdev-private-thread.h \
lib/dpif-netdev-private.h \
lib/dpif-netdev-perf.c \
diff --git a/lib/dpif-netdev-private-hwol.h b/lib/dpif-netdev-private-hwol.h
new file mode 100644
index 0..b93297a74
--- /dev/null
+++ b/lib/dpif-netdev-private-hwol.h
@@ -0,0 +1,63 @@
+/*
+ * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013, 2015 Nicira, Inc.
+ * Copyright (c) 2021 Intel Corporation.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at:
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#ifndef DPIF_NETDEV_PRIVATE_HWOL_H
+#define DPIF_NETDEV_PRIVATE_HWOL_H 1
+
+#include "dpif-netdev-private-flow.h"
+
+#define MAX_FLOW_MARK (UINT32_MAX - 1)
+#define INVALID_FLOW_MARK 0
+/* Zero flow mark is used to indicate the HW to remove the mark. A packet
+ * marked with zero mark is received in SW without a mark at all, so it
+ * cannot be used as a valid mark.
+ */
+
+struct megaflow_to_mark_data {
+const struct cmap_node node;
+ovs_u128 mega_ufid;
+uint32_t mark;
+};
+
+struct flow_mark {
+struct cmap megaflow_to_mark;
+struct cmap mark_to_flow;
+struct id_pool *pool;
+};
+
+/* allocated in dpif-netdev.c */
+extern struct flow_mark flow_mark;
+
+static inline struct dp_netdev_flow *
+mark_to_flow_find(const struct dp_netdev_pmd_thread *pmd,
+ const uint32_t mark)
+{
+struct dp_netdev_flow *flow;
+
+CMAP_FOR_EACH_WITH_HASH (flow, mark_node, hash_int(mark, 0),
+ _mark.mark_to_flow) {
+if (flow->mark == mark && flow->pmd_id == pmd->core_id &&
+flow->dead == false) {
+return flow;
+}
+}
+
+return NULL;
+}
+
+
+#endif /* dpif-netdev-private-hwol.h */
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index affeeacdc..e913f4efc 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -18,6 +18,7 @@
#include "dpif-netdev.h"
#include "dpif-netdev-private.h"
#include "dpif-netdev-private-dfc.h"
+#include "dpif-netdev-private-hwol.h"
#include
#include
@@ -1953,26 +1954,8 @@ dp_netdev_pmd_find_dpcls(struct dp_netdev_pmd_thread
*pmd,
return cls;
}
-#define MAX_FLOW_MARK (UINT32_MAX - 1)
-#define INVALID_FLOW_MARK 0
-/* Zero flow mark is used to indicate the HW to remove the mark. A packet
- * marked with zero mark is received in SW without a mark at all, so it
- * cannot be used as a valid mark.
- */
-struct megaflow_to_mark_data {
-const struct cmap_node node;
-ovs_u128 mega_ufid;
-uint32_t mark;
-};
-
-struct flow_mark {
-struct cmap megaflow_to_mark;
-struct cmap mark_to_flow;
-struct id_pool *pool;
-};
-
-static struct flow_mark flow_mark = {
+struct flow_mark flow_mark = {
.megaflow_to_mark = CMAP_INITIALIZER,
.mark_to_flow = CMAP_INITIALIZER,
};
@@ -2141,23 +2124,6 @@ flow_mark_flush(struct dp_netdev_pmd_thread *pmd)
}
}
-static struct dp_netdev_flow *
-mark_to_flow_find(const struct dp_netdev_pmd_thread *pmd,
- const uint32_t mark)
-{
-struct dp_netdev_flow *flow;
-
-CMAP_FOR_EACH_WITH_HASH (flow, mark_node, hash_int(mark, 0),
- _mark.mark_to_flow) {
-if (flow->mark == mark && flow->pmd_id == pmd->core_id &&
-flow->dead == false) {
-return flow;
-}
-}
-
-return NULL;
-}
-
static struct dp_flow_offload_item *
dp_netdev_alloc_flow_offload(struct dp_netdev_pmd_thread *pmd,
struct dp_netdev_flow *flow,
--
2.32.0
