On Fri, Mar 10, 2017 at 4:28 PM, Joe Stringer <j...@ovn.org> wrote: > On 9 March 2017 at 10:25, Yi-Hung Wei <yihung....@gmail.com> wrote: > > Currently, a controller may potentially trigger a segmentation fault if > it > > accidentally removes a TLV mapping that is still used by an active flow. > > To resolve this issue, in this patch, we maintain reference counting for > each > > dynamically allocated variable length mf_fields, so that vswitchd can > use this > > information to properly remove a TLV mapping, and to return an error if > the > > controller tries to remove a TLV mapping that is still used by any > active flow. > > > > To keep track of the usage of tun_metadata for each flow, two 'uint64_t' > > bitmaps are introduce for the flow match and flow action respectively. > We use > > 'uint64_t' as a bitmap since the 64 geneve TLV tunnel metadata are the > only > > available variable length mf_fields for now. We shall adopt general > bitmap when > > more variable length mf_fields are introduced. The bitmaps are configured > > during the flow decoding process, and vswitchd use these bitmaps to > increase or > > decrease the ref counting when the flow is created or deleted. > > > > VMWare-BZ: #1768370 > > Fixes: 04f48a68c428 ("ofp-actions: Fix variable length meta-flow OXMs.") > > Suggested-by: Jarno Rajahalme <ja...@ovn.org> > > Suggested-by: Joe Stringer <j...@ovn.org> > > Signed-off-by: Yi-Hung Wei <yihung....@gmail.com> > > --- > > Couple of minor comments to add from previous discussion, >
> <snip> > > > diff --git a/lib/meta-flow.c b/lib/meta-flow.c > > index 40704e628aaa..e844008f6294 100644 > > --- a/lib/meta-flow.c > > +++ b/lib/meta-flow.c > > @@ -27,6 +27,8 @@ > > #include "openvswitch/dynamic-string.h" > > #include "nx-match.h" > > #include "openvswitch/ofp-util.h" > > +#include "ofproto/ofproto-provider.h" > > We can drop this include now. > Sure. > > <snip> > > > @@ -2697,53 +2710,98 @@ mf_get_vl_mff(const struct mf_field *mff, > > return NULL; > > } > > > > -/* Updates the tun_metadata mf_field in 'vl_mff_map' according to 'ttm'. > > - * This function is supposed to be invoked after > tun_metadata_table_mod(). */ > > -enum ofperr > > -mf_vl_mff_map_mod_from_tun_metadata(struct vl_mff_map *vl_mff_map, > > - const struct ofputil_tlv_table_mod > *ttm) > > +static enum ofperr > > +mf_vl_mff_map_del(struct vl_mff_map *vl_mff_map, > > + const struct ofputil_tlv_table_mod *ttm, bool force) > > OVS_REQUIRES(vl_mff_map->mutex) > > { > > struct ofputil_tlv_map *tlv_map; > > + struct vl_mf_field *vmf; > > + unsigned int idx; > > > > - if (ttm->command == NXTTMC_CLEAR) { > > - mf_vl_mff_map_clear(vl_mff_map); > > - return 0; > > + if (!force) { > > + LIST_FOR_EACH (tlv_map, list_node, &ttm->mappings) { > > + idx = MFF_TUN_METADATA0 + tlv_map->index; > > Idly wondering whether it improves code readability to have some > simple functions like > > mff_to_idx(int) > idx_to_mff(int) > > which handle the +/- of MFF_TUN_METADATA0 in so many places around the > code. > > If you think it's worthwhile it could be a separate followup patch. > Sure, that make sense. Will have a follow up patch after this series is in. > > > @@ -6332,6 +6365,11 @@ ofpacts_pull_openflow_actions__(struct ofpbuf > *openflow, > > * you should call ofpacts_pull_openflow_instructions() instead of this > > * function. > > * > > + * 'vl_mff_map' and 'ofpacts_tlv_bitmap' are optional. If 'vl_mff_map' > is not > > + * NULL, it is used to drive the variable length mf_fields, and > > Where does it 'drive' the variable to? :-) > > (This comment might benefit from the feedback I had on similar > comments in the previous patch) > Thanks, it is update in v3. _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev