| Message ID | 20210617161825.94741-3-cian.ferriter@intel.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | DPIF Framework + Optimizations | expand |
Hello, I have some comments inline. On Thu, Jun 17, 2021 at 05:18:15PM +0100, Cian Ferriter wrote: > From: Harry van Haaren <harry.van.haaren@intel.com> > > 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 <harry.van.haaren@intel.com> > > --- > > Cc: Gaetan Rivet <gaetanr@nvidia.com> > Cc: Sriharsha Basavapatna <sriharsha.basavapatna@broadcom.com> > > 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 000000000..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), > + &flow_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 <ctype.h> > #include <errno.h> > @@ -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), > - &flow_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 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Hi Flavio, Thanks for the review. My comments are inline, Cian > -----Original Message----- > From: Flavio Leitner <fbl@sysclose.org> > Sent: Friday 18 June 2021 13:12 > To: Ferriter, Cian <cian.ferriter@intel.com> > Cc: ovs-dev@openvswitch.org; i.maximets@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 <harry.van.haaren@intel.com> > > > > 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 <harry.van.haaren@intel.com> > > > > --- > > > > Cc: Gaetan Rivet <gaetanr@nvidia.com> > > Cc: Sriharsha Basavapatna <sriharsha.basavapatna@broadcom.com> > > > > 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 000000000..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), > > + &flow_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. > The mark_to_flow_find() function looks like a good inline candidate to us since we can avoid function call overhead per packet. So keeping it in a header file so it will be inlined seems like the best approach. > 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 <ctype.h> > > #include <errno.h> > > @@ -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), > > - &flow_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 > > > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > -- > fbl
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 000000000..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), + &flow_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 <ctype.h> #include <errno.h> @@ -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), - &flow_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,