[ovs-dev] [v12 02/16] dpif-netdev: Split HWOL out to own header file.

Ferriter, Cian cian.ferriter at intel.com
Thu Jun 10 14:15:06 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.

> > +
> >  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