[ovs-dev] [v12 02/16] dpif-netdev: Split HWOL out to own header file.
Stokes, Ian
ian.stokes at intel.com
Tue Jun 15 17:27:07 UTC 2021
> Hi Ian,
>
> Thanks for the review. My responses are inline.
>
> > -----Original Message-----
> > From: Stokes, Ian <ian.stokes at intel.com>
> > Sent: Tuesday 1 June 2021 19:58
> > To: Ferriter, Cian <cian.ferriter at intel.com>; ovs-dev at openvswitch.org; Van
> Haaren, Harry <harry.van.haaren at intel.com>
> > Cc: i.maximets at ovn.org; Gaetan Rivet <gaetanr at nvidia.com>;
> elibr at nvidia.com
> > Subject: RE: [ovs-dev] [v12 02/16] dpif-netdev: Split HWOL out to own header
> file.
> >
> > > From: Harry van Haaren <harry.van.haaren at intel.com>
> > >
> > > 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 <harry.van.haaren at intel.com>
> >
> > 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.
>
Thanks.
Ian
> > > +
> > > 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
> > > dev at openvswitch.org
> > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
More information about the dev
mailing list