[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