Hi Ian, Thanks for the review. My responses are inline.
> -----Original Message----- > From: Stokes, Ian <[email protected]> > Sent: Tuesday 1 June 2021 19:58 > To: Ferriter, Cian <[email protected]>; [email protected]; Van > Haaren, Harry <[email protected]> > Cc: [email protected]; Gaetan Rivet <[email protected]>; [email protected] > Subject: RE: [ovs-dev] [v12 02/16] dpif-netdev: Split HWOL out to own header > file. > > > From: Harry van Haaren <[email protected]> > > > > This commit moves the datapath lookup functions required for > > hardware offload to a separate file. This allows other DPIF > > implementations to access the lookup functions, encouraging > > code reuse. > > > > Signed-off-by: Harry van Haaren <[email protected]> > > Hi Harry/Cian, in general this looks ok to me. Would be interested to seek > others opinions on the refactor just so that they are aware > as well. > > One or two minor comments below? > > > --- > > lib/automake.mk | 1 + > > lib/dpif-netdev-private-hwol.h | 63 ++++++++++++++++++++++++++++++++++ > > lib/dpif-netdev.c | 39 ++------------------- > > 3 files changed, 67 insertions(+), 36 deletions(-) > > create mode 100644 lib/dpif-netdev-private-hwol.h > > > > diff --git a/lib/automake.mk b/lib/automake.mk > > index 9fa8712c3..0bef0cc69 100644 > > --- a/lib/automake.mk > > +++ b/lib/automake.mk > > @@ -114,6 +114,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 298bfe444..88f37c505 100644 > > --- a/lib/dpif-netdev.c > > +++ b/lib/dpif-netdev.c > > @@ -84,6 +84,8 @@ > > #include "util.h" > > #include "uuid.h" > > > > +#include "dpif-netdev-private-hwol.h" > > Should this not be added above were dpif-netdev-private-dfc.h etc. are also > added? > > BR > Ian > Yes, that makes sense. I'll move this. > > + > > VLOG_DEFINE_THIS_MODULE(dpif_netdev); > > > > /* Auto Load Balancing Defaults */ > > @@ -1954,26 +1956,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, > > }; > > @@ -2142,23 +2126,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.31.1 > > > > _______________________________________________ > > dev mailing list > > [email protected] > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
