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

Stokes, Ian ian.stokes at intel.com
Tue Jun 1 18:58:13 UTC 2021


> 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

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