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

Ferriter, Cian cian.ferriter at intel.com
Mon Jun 21 16:10:37 UTC 2021


Hi Flavio,

Thanks for the review. My comments are inline,

Cian

> -----Original Message-----
> From: Flavio Leitner <fbl at sysclose.org>
> Sent: Friday 18 June 2021 13:12
> To: Ferriter, Cian <cian.ferriter at intel.com>
> Cc: ovs-dev at openvswitch.org; i.maximets at 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 at 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 at intel.com>
> >
> > ---
> >
> > Cc: Gaetan Rivet <gaetanr at nvidia.com>
> > Cc: Sriharsha Basavapatna <sriharsha.basavapatna at 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 at openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> 
> --
> fbl


More information about the dev mailing list