[ovs-dev] [PATCH 2/3] meta-flow: Remove dependency of cmap from meta-flow.h.

Joe Stringer joe at ovn.org
Tue Feb 21 19:43:55 UTC 2017


On 17 February 2017 at 17:47, Yi-Hung Wei <yihung.wei at gmail.com> wrote:
> Previous patch 04f48a68 ("ofp-actions: Fix variable length meta-flow OXMs.")
> introduced dependency of an internal library (cmap.h) to ovs public
> interface (meta-flow.h) that may cause potential building problem. In this
> patch, we remove cmap from struct mf_field, and provide a wrapper struct
> vl_mff_map that resolve the dependency problem.
>
> Fixes: 04f48a68 ("ofp-actions: Fix variable length meta-flow OXMs.")

8 characters is probably enough for OVS tree, but be aware that when
dealing with Linux commits the abbreviated hash needs to be longer. I
have "core.abbrev" git config set to 12.

> Suggested-by: Joe Stringer <joe at ovn.org>
> Suggested-by: Daniele Di Proietto <diproiettod at vmware.com>
> Signed-off-by: Yi-Hung Wei <yihung.wei at gmail.com>

Thanks for the patch, a few minor comments below.

If you can, please keep the patch title within 50 characters. This
helps formatting to email with [PATCH] prefixes etc. and also in a
terminal git log given the indents it introduces. I renamed a couple
of patches in this series to achieve this.

I applied these changes and pushed the patch to master.

> ---
>  build-aux/extract-ofp-fields      |  2 +-
>  include/openvswitch/meta-flow.h   | 24 +----------------
>  include/openvswitch/ofp-actions.h |  2 ++
>  include/openvswitch/ofp-util.h    |  1 +
>  lib/automake.mk                   |  1 +
>  lib/meta-flow.c                   | 54 +++++++++++++++++++++++----------------
>  lib/nx-match.c                    |  1 +
>  lib/ofp-actions.c                 |  1 +
>  lib/vl-mff-map.h                  | 41 +++++++++++++++++++++++++++++
>  ofproto/ofproto-provider.h        |  2 +-
>  10 files changed, 82 insertions(+), 47 deletions(-)
>  create mode 100644 lib/vl-mff-map.h
>
> diff --git a/build-aux/extract-ofp-fields b/build-aux/extract-ofp-fields
> index 40f1bb2..498b887 100755
> --- a/build-aux/extract-ofp-fields
> +++ b/build-aux/extract-ofp-fields
> @@ -386,7 +386,7 @@ def make_meta_flow(meta_flow_h):
>          else:
>              output += ["    -1, /* not usable for prefix lookup */"]
>
> -        output += ["    {OVSRCU_INITIALIZER(NULL)},},"]
> +        output += ["},"]
>      for oline in output:
>          print(oline)
>
> diff --git a/include/openvswitch/meta-flow.h b/include/openvswitch/meta-flow.h
> index d5c0971..309d5e3 100644
> --- a/include/openvswitch/meta-flow.h
> +++ b/include/openvswitch/meta-flow.h
> @@ -23,16 +23,15 @@
>  #include <sys/types.h>
>  #include <netinet/in.h>
>  #include <netinet/ip6.h>
> -#include "cmap.h"
>  #include "openvswitch/flow.h"
>  #include "openvswitch/ofp-errors.h"
>  #include "openvswitch/packets.h"
> -#include "openvswitch/thread.h"
>  #include "openvswitch/util.h"
>
>  struct ds;
>  struct match;
>  struct ofputil_tlv_table_mod;
> +struct vl_mff_map;

This is not necessary.

>
>  /* Open vSwitch fields
>   * ===================
> @@ -1774,9 +1773,6 @@ struct mf_field {
>
>      int flow_be32ofs;  /* Field's be32 offset in "struct flow", if prefix tree
>                          * lookup is supported for the field, or -1. */
> -
> -    /* For variable length mf_fields only. In ofproto->vl_mff_map->cmap. */
> -    struct cmap_node cmap_node;
>  };
>
>  /* The representation of a field's value. */
> @@ -1853,14 +1849,6 @@ union mf_subvalue {
>  };
>  BUILD_ASSERT_DECL(sizeof(union mf_value) == sizeof (union mf_subvalue));
>
> -/* Variable length mf_fields mapping map. This is a single writer,
> - * multiple-reader hash table that a writer must hold the following mutex
> - * to access this map. */
> -struct vl_mff_map {
> -    struct cmap cmap;       /* Contains 'struct mf_field' */
> -    struct ovs_mutex mutex;
> -};
> -
>  bool mf_subvalue_intersect(const union mf_subvalue *a_value,
>                             const union mf_subvalue *a_mask,
>                             const union mf_subvalue *b_value,
> @@ -1987,14 +1975,4 @@ void mf_format_subvalue(const union mf_subvalue *subvalue, struct ds *s);
>  /* Field Arrays. */
>  void field_array_set(enum mf_field_id id, const union mf_value *,
>                       struct field_array *);
> -
> -/* Variable length fields. */
> -void mf_vl_mff_map_clear(struct vl_mff_map *vl_mff_map)
> -    OVS_REQUIRES(vl_mff_map->mutex);
> -enum ofperr mf_vl_mff_map_mod_from_tun_metadata(
> -    struct vl_mff_map *vl_mff_map, const struct ofputil_tlv_table_mod *)
> -    OVS_REQUIRES(vl_mff_map->mutex);
> -const struct mf_field * mf_get_vl_mff(const struct mf_field *,
> -                                      const struct vl_mff_map *);
> -bool mf_vl_mff_invalid(const struct mf_field *, const struct vl_mff_map *);
>  #endif /* meta-flow.h */
> diff --git a/include/openvswitch/ofp-actions.h b/include/openvswitch/ofp-actions.h
> index a3783c2..88f573d 100644
> --- a/include/openvswitch/ofp-actions.h
> +++ b/include/openvswitch/ofp-actions.h
> @@ -26,6 +26,8 @@
>  #include "openvswitch/ofp-errors.h"
>  #include "openvswitch/types.h"
>
> +struct vl_mff_map;
> +
>  /* List of OVS abstracted actions.
>   *
>   * This macro is used directly only internally by this header, but the list is
> diff --git a/include/openvswitch/ofp-util.h b/include/openvswitch/ofp-util.h
> index dd254e6..0c3a10a 100644
> --- a/include/openvswitch/ofp-util.h
> +++ b/include/openvswitch/ofp-util.h
> @@ -36,6 +36,7 @@
>  struct ofpbuf;
>  union ofp_action;
>  struct ofpact_set_field;
> +struct vl_mff_map;
>
>  /* Port numbers. */
>  enum ofperr ofputil_port_from_ofp11(ovs_be32 ofp11_port,
> diff --git a/lib/automake.mk b/lib/automake.mk
> index abc9d0d..b266af1 100644
> --- a/lib/automake.mk
> +++ b/lib/automake.mk
> @@ -284,6 +284,7 @@ lib_libopenvswitch_la_SOURCES = \
>         lib/vconn-stream.c \
>         lib/vconn.c \
>         lib/versions.h \
> +       lib/vl-mff-map.h \
>         lib/vlan-bitmap.c \
>         lib/vlan-bitmap.h \
>         lib/vlog.c \
> diff --git a/lib/meta-flow.c b/lib/meta-flow.c
> index b92950b..7945bd6 100644
> --- a/lib/meta-flow.c
> +++ b/lib/meta-flow.c
> @@ -38,6 +38,7 @@
>  #include "util.h"
>  #include "openvswitch/ofp-errors.h"
>  #include "openvswitch/vlog.h"
> +#include "vl-mff-map.h"
>
>  VLOG_DEFINE_THIS_MODULE(meta_flow);
>
> @@ -2641,6 +2642,13 @@ field_array_set(enum mf_field_id id, const union mf_value *value,
>      memcpy(fa->values + offset, value, value_size);
>  }
>
> +/* A wrapper for variable length mf_fields that is maintained by
> + * struct vl_mff_map.*/
> +struct vl_mf_field {
> +    struct mf_field mf;
> +    struct cmap_node cmap_node; /* In ofproto->vl_mff_map->cmap. */
> +};
> +
>  static inline uint32_t
>  mf_field_hash(uint32_t key)
>  {
> @@ -2651,23 +2659,24 @@ void
>  mf_vl_mff_map_clear(struct vl_mff_map *vl_mff_map)
>      OVS_REQUIRES(vl_mff_map->mutex)
>  {
> -    struct mf_field *mf;
> +    struct vl_mf_field *vmf;
>
> -    CMAP_FOR_EACH (mf, cmap_node, &vl_mff_map->cmap) {
> -        cmap_remove(&vl_mff_map->cmap, &mf->cmap_node, mf_field_hash(mf->id));
> -        ovsrcu_postpone(free, mf);
> +    CMAP_FOR_EACH (vmf, cmap_node, &vl_mff_map->cmap) {
> +        cmap_remove(&vl_mff_map->cmap, &vmf->cmap_node,
> +                    mf_field_hash(vmf->mf.id));
> +        ovsrcu_postpone(free, vmf);
>      }
>  }
>
> -static struct mf_field *
> +static struct vl_mf_field *
>  mf_get_vl_mff__(uint32_t id, const struct vl_mff_map *vl_mff_map)
>  {
> -    struct mf_field *field;
> +    struct vl_mf_field *vmf;
>
> -    CMAP_FOR_EACH_WITH_HASH (field, cmap_node, mf_field_hash(id),
> +    CMAP_FOR_EACH_WITH_HASH (vmf, cmap_node, mf_field_hash(id),
>                               &vl_mff_map->cmap) {
> -        if (field->id == id) {
> -            return field;
> +        if (vmf->mf.id == id) {
> +            return vmf;
>          }
>      }
>
> @@ -2682,7 +2691,7 @@ mf_get_vl_mff(const struct mf_field *mff,
>                const struct vl_mff_map *vl_mff_map)
>  {
>      if (mff && mff->variable_len && vl_mff_map) {
> -        return mf_get_vl_mff__(mff->id, vl_mff_map);
> +        return &(mf_get_vl_mff__(mff->id, vl_mff_map)->mf);

I don't think the extra parentheses are necessary.

>      }
>
>      return NULL;
> @@ -2704,7 +2713,7 @@ mf_vl_mff_map_mod_from_tun_metadata(struct vl_mff_map *vl_mff_map,
>
>      LIST_FOR_EACH (tlv_map, list_node, &ttm->mappings) {
>          unsigned int idx = MFF_TUN_METADATA0 + tlv_map->index;
> -        struct mf_field *mf;
> +        struct vl_mf_field *vmf;
>
>          if (idx >= MFF_TUN_METADATA0 + TUN_METADATA_NUM_OPTS) {
>              return OFPERR_NXTTMFC_BAD_FIELD_IDX;
> @@ -2712,21 +2721,22 @@ mf_vl_mff_map_mod_from_tun_metadata(struct vl_mff_map *vl_mff_map,
>
>          switch (ttm->command) {
>          case NXTTMC_ADD:
> -            mf = xmalloc(sizeof *mf);
> -            *mf = mf_fields[idx];
> -            mf->n_bytes = tlv_map->option_len;
> -            mf->n_bits = tlv_map->option_len * 8;
> -            mf->mapped = true;
> -
> -            cmap_insert(&vl_mff_map->cmap, &mf->cmap_node, mf_field_hash(idx));
> +            vmf = xmalloc(sizeof *vmf);
> +            vmf->mf = mf_fields[idx];
> +            vmf->mf.n_bytes = tlv_map->option_len;
> +            vmf->mf.n_bits = tlv_map->option_len * 8;
> +            vmf->mf.mapped = true;
> +
> +            cmap_insert(&vl_mff_map->cmap, &vmf->cmap_node,
> +                        mf_field_hash(idx));
>              break;
>
>          case NXTTMC_DELETE:
> -            mf = mf_get_vl_mff__(idx, vl_mff_map);
> -            if (mf) {
> -                cmap_remove(&vl_mff_map->cmap, &mf->cmap_node,
> +            vmf = mf_get_vl_mff__(idx, vl_mff_map);
> +            if (vmf) {
> +                cmap_remove(&vl_mff_map->cmap, &vmf->cmap_node,
>                              mf_field_hash(idx));
> -                ovsrcu_postpone(free, mf);
> +                ovsrcu_postpone(free, vmf);
>              }
>              break;
>
> diff --git a/lib/nx-match.c b/lib/nx-match.c
> index e9d649b..91401e2 100644
> --- a/lib/nx-match.c
> +++ b/lib/nx-match.c
> @@ -36,6 +36,7 @@
>  #include "tun-metadata.h"
>  #include "unaligned.h"
>  #include "util.h"
> +#include "vl-mff-map.h"

lib/nx-match.h also needed a forward-declaration of 'struct vl_mff_map'.

>
>  VLOG_DEFINE_THIS_MODULE(nx_match);
>
> diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
> index 11ab692..ce80f57 100644
> --- a/lib/ofp-actions.c
> +++ b/lib/ofp-actions.c
> @@ -37,6 +37,7 @@
>  #include "openvswitch/vlog.h"
>  #include "unaligned.h"
>  #include "util.h"
> +#include "vl-mff-map.h"
>
>  VLOG_DEFINE_THIS_MODULE(ofp_actions);
>
> diff --git a/lib/vl-mff-map.h b/lib/vl-mff-map.h
> new file mode 100644
> index 0000000..35033aa
> --- /dev/null
> +++ b/lib/vl-mff-map.h
> @@ -0,0 +1,41 @@
> +/*
> + * Copyright (c) 2010-2017 Nicira, Inc.

2017 should suffice, I think all of this code is new.

> + *
> + * 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 VL_MFF_MAP_H
> +#define VL_MFF_MAP_H 1
> +
> +#include "cmap.h"
> +#include "openvswitch/thread.h"
> +
> +/* Variable length mf_fields mapping map. This is a single writer,
> + * multiple-reader hash table that a writer must hold the following mutex
> + * to access this map. */
> +struct vl_mff_map {
> +    struct cmap cmap;       /* Contains 'struct mf_field' */
> +    struct ovs_mutex mutex;
> +};
> +
> +/* Variable length fields. */
> +void mf_vl_mff_map_clear(struct vl_mff_map *vl_mff_map)
> +    OVS_REQUIRES(vl_mff_map->mutex);
> +enum ofperr mf_vl_mff_map_mod_from_tun_metadata(
> +    struct vl_mff_map *vl_mff_map, const struct ofputil_tlv_table_mod *)
> +    OVS_REQUIRES(vl_mff_map->mutex);
> +const struct mf_field * mf_get_vl_mff(const struct mf_field *,
> +                                      const struct vl_mff_map *);
> +bool mf_vl_mff_invalid(const struct mf_field *, const struct vl_mff_map *);
> +
> +#endif /* vl-mff-map.h */
> diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
> index 4808974..7d3e929 100644
> --- a/ofproto/ofproto-provider.h
> +++ b/ofproto/ofproto-provider.h
> @@ -52,6 +52,7 @@
>  #include "timeval.h"
>  #include "tun-metadata.h"
>  #include "versions.h"
> +#include "vl-mff-map.h"
>
>  struct match;
>  struct ofputil_flow_mod;
> @@ -59,7 +60,6 @@ struct bfd_cfg;
>  struct meter;
>  struct ofoperation;
>  struct ofproto_packet_out;
> -struct vl_mff_map;
>  struct smap;
>
>  extern struct ovs_mutex ofproto_mutex;
> --
> 2.7.4
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev


More information about the dev mailing list