On 17 February 2017 at 17:47, Yi-Hung Wei <[email protected]> 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 <[email protected]> > Suggested-by: Daniele Di Proietto <[email protected]> > Signed-off-by: Yi-Hung Wei <[email protected]> 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 > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
