[ovs-dev] [PATCH v2 2/2] ofproto: Add ref counting for variable length mf_fields.

[Yi-Hung Wei]

On Fri, Mar 10, 2017 at 4:28 PM, Joe Stringer <joe at ovn.org> wrote:

> On 9 March 2017 at 10:25, Yi-Hung Wei <yihung.wei at 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 <jarno at ovn.org>
> > Suggested-by: Joe Stringer <joe at ovn.org>
> > Signed-off-by: Yi-Hung Wei <yihung.wei at 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.