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

Flavio Leitner fbl at sysclose.org
Fri Jun 18 12:12:25 UTC 2021


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.

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

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