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

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@ovn.org>
Suggested-by: Joe Stringer <joe@ovn.org>
Signed-off-by: Yi-Hung Wei <yihung.wei@gmail.com>
---
 build-aux/extract-ofp-actions     |   9 +-
 include/openvswitch/ofp-actions.h |   3 +-
 include/openvswitch/ofp-errors.h  |   4 +
 include/openvswitch/ofp-util.h    |   1 +
 lib/learn.c                       |   6 ++
 lib/meta-flow.c                   | 196 +++++++++++++++++++++++++++++++-------
 lib/ofp-actions.c                 | 156 ++++++++++++++++++------------
 lib/ofp-util.c                    |  21 ++--
 lib/vl-mff-map.h                  |  10 +-
 ofproto/ofproto-provider.h        |   4 +
 ofproto/ofproto.c                 |  39 ++++++--
 ovn/controller/pinctrl.c          |   6 +-
 tests/tunnel.at                   |  76 ++++++++++++++-
 utilities/ovs-ofctl.c             |   2 +-
 14 files changed, 404 insertions(+), 129 deletions(-)

On 7 March 2017 at 09:39, Yi-Hung Wei <yihung.wei@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@ovn.org>
> Suggested-by: Joe Stringer <joe@ovn.org>
> Signed-off-by: Yi-Hung Wei <yihung.wei@gmail.com>
> ---

Thanks for the patch, feedback below.

>  build-aux/extract-ofp-actions     |   9 +-
>  include/openvswitch/ofp-actions.h |   3 +-
>  include/openvswitch/ofp-errors.h  |   4 +
>  include/openvswitch/ofp-util.h    |   1 +
>  lib/learn.c                       |   6 ++
>  lib/meta-flow.c                   | 196 +++++++++++++++++++++++++++++++-------
>  lib/ofp-actions.c                 | 156 ++++++++++++++++++------------
>  lib/ofp-util.c                    |  21 ++--
>  lib/vl-mff-map.h                  |  10 +-
>  ofproto/ofproto-provider.h        |   4 +
>  ofproto/ofproto.c                 |  39 ++++++--
>  ovn/controller/pinctrl.c          |   6 +-
>  tests/tunnel.at                   |  76 ++++++++++++++-
>  utilities/ovs-ofctl.c             |   2 +-
>  14 files changed, 404 insertions(+), 129 deletions(-)
>
> diff --git a/build-aux/extract-ofp-actions b/build-aux/extract-ofp-actions
> index 184447b99422..0062ab881dd5 100755
> --- a/build-aux/extract-ofp-actions
> +++ b/build-aux/extract-ofp-actions
> @@ -322,7 +322,8 @@ def extract_ofp_actions(fn, definitions):
>  static enum ofperr
>  ofpact_decode(const struct ofp_action_header *a, enum ofp_raw_action_type raw,
>                enum ofp_version version, uint64_t arg,
> -              const struct vl_mff_map *vl_mff_map, struct ofpbuf *out)
> +              const struct vl_mff_map *vl_mff_map,
> +              uint64_t *tlv_bitmap, struct ofpbuf *out)
>  {
>      switch (raw) {\
>  """
> @@ -343,7 +344,7 @@ ofpact_decode(const struct ofp_action_header *a, enum ofp_raw_action_type raw,
>                      else:
>                          arg = "arg"
>                  if arg_vl_mff_map:
> -                    print "        return decode_%s(%s, version, vl_mff_map, out);" % (enum, arg)
> +                    print "        return decode_%s(%s, version, vl_mff_map, tlv_bitmap, out);" % (enum, arg)
>                  else:
>                      print "        return decode_%s(%s, version, out);" % (enum, arg)
>              print
> @@ -365,7 +366,7 @@ ofpact_decode(const struct ofp_action_header *a, enum ofp_raw_action_type raw,
>                  else:
>                      prototype += "%s, enum ofp_version, " % base_argtype
>                  if arg_vl_mff_map:
> -                    prototype += 'const struct vl_mff_map *, '
> +                    prototype += 'const struct vl_mff_map *, uint64_t *, '
>              prototype += "struct ofpbuf *);"
>              print prototype
>
> @@ -374,7 +375,7 @@ static enum ofperr ofpact_decode(const struct ofp_action_header *,
>                                   enum ofp_raw_action_type raw,
>                                   enum ofp_version version,
>                                   uint64_t arg, const struct vl_mff_map *vl_mff_map,
> -                                 struct ofpbuf *out);
> +                                 uint64_t *tlv_bitmap, struct ofpbuf *out);
>  """
>
>  if __name__ == '__main__':
> diff --git a/include/openvswitch/ofp-actions.h b/include/openvswitch/ofp-actions.h
> index 88f573dcd74e..808715e9b1a4 100644
> --- a/include/openvswitch/ofp-actions.h
> +++ b/include/openvswitch/ofp-actions.h
> @@ -946,13 +946,14 @@ enum ofperr ofpacts_pull_openflow_actions(struct ofpbuf *openflow,
>                                            unsigned int actions_len,
>                                            enum ofp_version version,
>                                            const struct vl_mff_map *,
> +                                          uint64_t *,

Please name this variable, this is the public library. Particularly
since the field is a generic type (uint64_t), there's no way to get
the context of what it's used for in the header here. Also, please
update the comment for this function, specifying whether it is a
required field and what the effect of specifying this is.

>                                            struct ofpbuf *ofpacts);
>  enum ofperr
>  ofpacts_pull_openflow_instructions(struct ofpbuf *openflow,
>                                     unsigned int instructions_len,
>                                     enum ofp_version version,
>                                     const struct vl_mff_map *vl_mff_map,
> -                                   struct ofpbuf *ofpacts);
> +                                   uint64_t *, struct ofpbuf *ofpacts);

Same here.

>  enum ofperr ofpacts_check(struct ofpact[], size_t ofpacts_len,
>                            struct flow *, ofp_port_t max_ports,
>                            uint8_t table_id, uint8_t n_tables,
> diff --git a/include/openvswitch/ofp-errors.h b/include/openvswitch/ofp-errors.h
> index 81825817e843..a5bba8619bcb 100644
> --- a/include/openvswitch/ofp-errors.h
> +++ b/include/openvswitch/ofp-errors.h
> @@ -772,6 +772,10 @@ enum ofperr {
>       * to be mapped is the same as one assigned to a different field. */
>      OFPERR_NXTTMFC_DUP_ENTRY,
>
> +    /* NX1.0-1.1(1,537), NX1.2+(38).  Attempted to delete a TLV mapping that
> +     * is used by any active flow. */
> +    OFPERR_NXTTMFC_INVALID_TLV_DEL,
> +
>  /* ## ---------- ## */
>  /* ## NXT_RESUME ## */
>  /* ## ---------- ## */
> diff --git a/include/openvswitch/ofp-util.h b/include/openvswitch/ofp-util.h
> index e73a942a3e15..7cb9e7fd32bd 100644
> --- a/include/openvswitch/ofp-util.h
> +++ b/include/openvswitch/ofp-util.h
> @@ -326,6 +326,7 @@ struct ofputil_flow_mod {
>      uint16_t importance;     /* Eviction precedence. */
>      struct ofpact *ofpacts;  /* Series of "struct ofpact"s. */
>      size_t ofpacts_len;      /* Length of ofpacts, in bytes. */
> +    uint64_t ofpacts_tlv_bitmap; /* 1-bit for each present TLV in 'ofpacts'. */
>  };
>
>  enum ofperr ofputil_decode_flow_mod(struct ofputil_flow_mod *,
> diff --git a/lib/learn.c b/lib/learn.c
> index ce52c35f297a..0e86f1629d74 100644
> --- a/lib/learn.c
> +++ b/lib/learn.c
> @@ -29,6 +29,7 @@
>  #include "openvswitch/ofp-errors.h"
>  #include "openvswitch/ofp-util.h"
>  #include "openvswitch/ofpbuf.h"
> +#include "vl-mff-map.h"
>  #include "unaligned.h"
>
>
> @@ -107,6 +108,8 @@ learn_execute(const struct ofpact_learn *learn, const struct flow *flow,
>      fm->importance = 0;
>      fm->buffer_id = UINT32_MAX;
>      fm->out_port = OFPP_NONE;
> +    fm->match.flow.tunnel.metadata.present.map = 0;

This line should already be covered by the call to match_init_catchall() above.

> +    fm->ofpacts_tlv_bitmap = 0;
>      fm->flags = 0;
>      if (learn->flags & NX_LEARN_F_SEND_FLOW_REM) {
>          fm->flags |= OFPUTIL_FF_SEND_FLOW_REM;
> @@ -136,6 +139,8 @@ learn_execute(const struct ofpact_learn *learn, const struct flow *flow,
>          switch (spec->dst_type) {
>          case NX_LEARN_DST_MATCH:
>              mf_write_subfield(&spec->dst, &value, &fm->match);
> +            mf_vl_mff_set_tlv_bitmap(
> +                spec->dst.field, &fm->match.flow.tunnel.metadata.present.map);
>              break;
>
>          case NX_LEARN_DST_LOAD:
> @@ -145,6 +150,7 @@ learn_execute(const struct ofpact_learn *learn, const struct flow *flow,
>                           spec->n_bits);
>              bitwise_one(ofpact_set_field_mask(sf), spec->dst.field->n_bytes,
>                          spec->dst.ofs, spec->n_bits);
> +            mf_vl_mff_set_tlv_bitmap(spec->dst.field, &fm->ofpacts_tlv_bitmap);
>              break;
>
>          case NX_LEARN_DST_OUTPUT:

I think we should also set the tlv bitmap in this LEARN_DST_OUTPUT
case? IIUC this uses an OXM field to determine the output port. I
believe it's possible for someone to use a TLV to specify an output
action using LEARN - so it would set the bitmap in the same way that
the NX_LEARN_DST_LOAD case does.

> diff --git a/lib/meta-flow.c b/lib/meta-flow.c
> index 40704e628aaa..7807b3055f5b 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"
> +#include "ovs-atomic.h"
>  #include "ovs-rcu.h"
>  #include "ovs-thread.h"
>  #include "packets.h"
> @@ -2646,6 +2648,7 @@ field_array_set(enum mf_field_id id, const union mf_value *value,
>   * struct vl_mff_map.*/
>  struct vl_mf_field {
>      struct mf_field mf;
> +    struct ovs_refcount ref_cnt;
>      struct cmap_node cmap_node; /* In ofproto->vl_mff_map->cmap. */
>  };
>
> @@ -2655,17 +2658,25 @@ mf_field_hash(uint32_t key)
>      return hash_int(key, 0);
>  }
>
> -void
> -mf_vl_mff_map_clear(struct vl_mff_map *vl_mff_map)
> +enum ofperr
> +mf_vl_mff_map_clear(struct vl_mff_map *vl_mff_map, bool clear)
>      OVS_REQUIRES(vl_mff_map->mutex)
>  {
>      struct vl_mf_field *vmf;
>
>      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);
> +        if (clear) {
> +            cmap_remove(&vl_mff_map->cmap, &vmf->cmap_node,
> +                        mf_field_hash(vmf->mf.id));
> +            ovsrcu_postpone(free, vmf);
> +        } else {
> +            if (ovs_refcount_read(&vmf->ref_cnt) != 1) {
> +                return OFPERR_NXTTMFC_INVALID_TLV_DEL;
> +            }
> +        }
>      }
> +
> +    return 0;
>  }

I found this a bit confusing. mf_vl_mff_map_clear() has a boolean,
also named 'clear' that determines whether it will actually clear or
not. If you call a function like "clear(clear=false)", what would it
intuitively do?

I see two calling conventions - first, for TLV clear command from controller:
error = mf_vl_mff_map_clear(vl_mff_map, false);
if (error)
    return error;
error = mf_vl_mff_map_clear(vl_mff_map, true);

Second, from ofproto cleanup code:
mf_vl_mff_map_clear(vl_mff_map, true);

I suggest that this would be easier to understand if the function were
structured with the the bool to be 'force':

enum ofperr
mf_vl_mff_map_clear(struct vl_mff_map *vl_mff_map, bool force)
   OVS_REQUIRES(vl_mff_map->mutex)
{
   struct vl_mf_field *vmf;

    /* First, check whether any flows still refer to the current TLV
map. */
   if (!force) {
       CMAP_FOR_EACH (vmf, cmap_node, &vl_mff_map->cmap) {
           if (ovs_refcount_read(&vmf->ref_cnt) != 1) {
               return OFPERR_NXTTMFC_INVALID_TLV_DEL;
           }
       }
   }

   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);
   }

   return 0;
}

You may also consider refactoring the ovsrcu_postpone(free, vmf) into
a separate, vmf_delete() function which looks something like this:

static void vmf_delete(struct vl_mf_field *vmf)
{
   if (ovs_refcount_unref(&vmf->ref_cnt) == 1) {
        /* Postpone as this function is typically called immediately
after removing from cmap. */
       ovsrcu_postpone(free, vmf);
   } else {
       VLOG_WARN_RL(&rl, "Attempted to delete VMF %s but refcount is
nonzero!",
                 vmf->mf.name);
   }
}

This could be reused from the other delete locations as well.

>  static struct vl_mf_field *
> @@ -2697,53 +2708,100 @@ 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 del)

Similarly, I find it confusing to reason about a function that can be
called like "delete(delete=false)".

>      OVS_REQUIRES(vl_mff_map->mutex)
>  {
>      struct ofputil_tlv_map *tlv_map;
> -
> -    if (ttm->command == NXTTMC_CLEAR) {
> -        mf_vl_mff_map_clear(vl_mff_map);
> -        return 0;
> -    }
> +    struct vl_mf_field *vmf;
> +    unsigned int idx;
>
>      LIST_FOR_EACH (tlv_map, list_node, &ttm->mappings) {
> -        unsigned int idx = MFF_TUN_METADATA0 + tlv_map->index;
> -        struct vl_mf_field *vmf;
> -
> +        idx = MFF_TUN_METADATA0 + tlv_map->index;
>          if (idx >= MFF_TUN_METADATA0 + TUN_METADATA_NUM_OPTS) {
>              return OFPERR_NXTTMFC_BAD_FIELD_IDX;
>          }
>
> -        switch (ttm->command) {
> -        case NXTTMC_ADD:
> -            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:
> -            vmf = mf_get_vl_mff__(idx, vl_mff_map);
> -            if (vmf) {
> +        vmf = mf_get_vl_mff__(idx, vl_mff_map);
> +        if (vmf) {
> +            if (del == true) {
>                  cmap_remove(&vl_mff_map->cmap, &vmf->cmap_node,
>                              mf_field_hash(idx));
>                  ovsrcu_postpone(free, vmf);
> +            } else {
> +                if (ovs_refcount_read(&vmf->ref_cnt) != 1) {
> +                    return OFPERR_NXTTMFC_INVALID_TLV_DEL;
> +                }
>              }
> -            break;
> +        }
> +    }
> +
> +    return 0;
> +}
> +
> +static enum ofperr
> +mf_vl_mff_map_add(struct vl_mff_map *vl_mff_map,
> +                  const struct ofputil_tlv_table_mod *ttm)
> +    OVS_REQUIRES(vl_mff_map->mutex)
> +{
> +    struct ofputil_tlv_map *tlv_map;
> +    struct vl_mf_field *vmf;
> +    unsigned int idx;
> +
> +    LIST_FOR_EACH (tlv_map, list_node, &ttm->mappings) {
> +        idx = MFF_TUN_METADATA0 + tlv_map->index;
> +        if (idx >= MFF_TUN_METADATA0 + TUN_METADATA_NUM_OPTS) {
> +            return OFPERR_NXTTMFC_BAD_FIELD_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;
> +        ovs_refcount_init(&vmf->ref_cnt);
> +
> +        cmap_insert(&vl_mff_map->cmap, &vmf->cmap_node,
> +                    mf_field_hash(idx));
> +    }
> +
> +    return 0;
> +}
>
> -        case NXTTMC_CLEAR:
> -        default:
> -            OVS_NOT_REACHED();
> +/* 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().

I realise the above sentence is from the existing code, but it's
clearer if this comment is prescriptive, ie:

"This function must be invoked after tun_metadata_table_mod()."

> + * Returns OFPERR_NXTTMFC_BAD_FIELD_IDX, if the index for the vl_mf_field is
> + * invalid.
> + * Returns OFPERR_NXTTMFC_INVALID_TLV_DEL, if 'ttm' tries to delete an
> + * vl_mf_field that is still used by any active flow.*/
> +enum ofperr
> +mf_vl_mff_map_mod_from_tun_metadata(struct vl_mff_map *vl_mff_map,
> +                                    const struct ofputil_tlv_table_mod *ttm)
> +    OVS_REQUIRES(vl_mff_map->mutex)
> +{
> +    enum ofperr error;
> +
> +    switch (ttm->command) {
> +    case NXTTMC_ADD:
> +        return mf_vl_mff_map_add(vl_mff_map, ttm);
> +
> +    case NXTTMC_DELETE:
> +        error = mf_vl_mff_map_del(vl_mff_map, ttm, false);
> +        if (error) {
> +            return error;
>          }
> +        return mf_vl_mff_map_del(vl_mff_map, ttm, true);
> +
> +    case NXTTMC_CLEAR:
> +        error = mf_vl_mff_map_clear(vl_mff_map, false);
> +        if (error) {
> +            return error;
> +        }
> +        return mf_vl_mff_map_clear(vl_mff_map, true);
> +
> +    default:
> +        OVS_NOT_REACHED();
>      }
>
>      return 0;
> @@ -2756,3 +2814,67 @@ mf_vl_mff_invalid(const struct mf_field *mff, const struct vl_mff_map *map)
>  {
>      return map && mff && mff->variable_len && !mff->mapped;
>  }
> +
> +void
> +mf_vl_mff_set_tlv_bitmap(const struct mf_field *mff, uint64_t *tlv_bitmap)
> +{
> +    if (mff && mff->mapped) {
> +        ULLONG_SET1(*tlv_bitmap, mff->id - MFF_TUN_METADATA0);

I wonder if we should add some runtime check here to ovs_assert() that
we're not trying to set a bit beyond the length of the field. I think
that if the second parameter of the ULLONG_SET1() is >64 then some
tests will fail, but it's not as obvious as an assert right here where
you're attempting to set the bit.

> +    }
> +}
> +
> +/* If the 'mff' is a valid variable length mf_field, udpates the 'tlv_bitmap'.
> + * Returns OFPERR_NXFMFC_INVALID_TLV_FIELD if the variable length mf_field
> + * is invalid. */
> +enum ofperr
> +mf_check_vl_mff(const struct mf_field *mff, const struct vl_mff_map *map,
> +                uint64_t *tlv_bitmap)

Could we rename this to mf_set_vl_mff_bitmap()?

> +{
> +    if (mf_vl_mff_invalid(mff, map)) {
> +        return OFPERR_NXFMFC_INVALID_TLV_FIELD;
> +    }
> +    mf_vl_mff_set_tlv_bitmap(mff, tlv_bitmap);
> +
> +    return 0;
> +}
> +
> +static void
> +mf_vl_mff_ref_cnt_mod(const struct vl_mff_map *map, uint64_t tlv_bitmap,
> +                      bool ref)
> +{
> +    struct vl_mf_field *vmf;
> +    int i;
> +
> +    if (map) {
> +        ULLONG_FOR_EACH_1 (i, tlv_bitmap) {
> +            vmf = mf_get_vl_mff__(i + MFF_TUN_METADATA0, map);
> +            if (vmf) {
> +                if (ref) {
> +                    ovs_refcount_ref(&vmf->ref_cnt);
> +                } else {
> +                    ovs_refcount_unref(&vmf->ref_cnt);

We should warn if we're trying to unref and this is the final ref,
because then we're not freeing the object.

> +                }
> +            } else {
> +                VLOG_WARN("Invalid TLV index %d.", i);
> +            }
> +        }
> +    }
> +}
> +
> +void
> +mf_vl_mff_add_ref_cnt_from_rule(const struct rule *rule)
> +{
> +    mf_vl_mff_ref_cnt_mod(&rule->ofproto->vl_mff_map, rule->match_tlv_bitmap,
> +                          true);
> +    mf_vl_mff_ref_cnt_mod(&rule->ofproto->vl_mff_map, rule->ofpacts_tlv_bitmap,
> +                          true);
> +}
> +
> +void
> +mf_vl_mff_remove_ref_cnt_from_rule(const struct rule *rule)
> +{
> +    mf_vl_mff_ref_cnt_mod(&rule->ofproto->vl_mff_map, rule->match_tlv_bitmap,
> +                          false);
> +    mf_vl_mff_ref_cnt_mod(&rule->ofproto->vl_mff_map, rule->ofpacts_tlv_bitmap,
> +                          false);
> +}

These should probably just provide the vl_mff_map and bitmap rather
than introducing a dependency on ofproto/ofproto-provider.h and struct
rule.

Usually functions in ovs use the format foo_ref() and foo_unref() to
add/remove refcounts. Could you rename these to keep with that style?

> diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
> index ce80f57e8df3..584e5cd4e847 100644
> --- a/lib/ofp-actions.c
> +++ b/lib/ofp-actions.c
> @@ -405,7 +405,7 @@ static enum ofperr ofpacts_pull_openflow_actions__(
>      struct ofpbuf *openflow, unsigned int actions_len,
>      enum ofp_version version, uint32_t allowed_ovsinsts,
>      struct ofpbuf *ofpacts, enum ofpact_type outer_action,
> -    const struct vl_mff_map *vl_mff_map);
> +    const struct vl_mff_map *vl_mff_map, uint64_t *ofpacts_tlv_bitmap);
>  static char * OVS_WARN_UNUSED_RESULT ofpacts_parse_copy(
>      const char *s_, struct ofpbuf *ofpacts,
>      enum ofputil_protocol *usable_protocols,
> @@ -1117,13 +1117,20 @@ struct nx_action_output_reg2 {
>  };
>  OFP_ASSERT(sizeof(struct nx_action_output_reg2) == 24);
>
> +#define CHECK_VL_MFF(FIELD, VL_MFF_MAP, BITMAP, ERR)    \
> +ERR = mf_check_vl_mff(FIELD, VL_MFF_MAP, BITMAP);       \
> +if (ERR) {                                              \
> +    return ERR;                                         \
> +}
> +

I realise there's a few spots in OVS code that use a style like this,
and it's (a little) easier to write.. but for reading, it's easy to
miss the fact that this macro may cause functions to return. For
instance, we used to do some MPLS checking like this and we ended up
removing it to improve readability. I think we should just expand this
out in the 5 or 6 places that this is used in this file.

>  static enum ofperr
>  decode_NXAST_RAW_OUTPUT_REG(const struct nx_action_output_reg *naor,
>                              enum ofp_version ofp_version OVS_UNUSED,
>                              const struct vl_mff_map *vl_mff_map,
> -                            struct ofpbuf *out)
> +                            uint64_t *tlv_bitmap, struct ofpbuf *out)
>  {
>      struct ofpact_output_reg *output_reg;
> +    enum ofperr error;
>
>      if (!is_all_zeros(naor->zero, sizeof naor->zero)) {
>          return OFPERR_OFPBAC_BAD_ARGUMENT;
> @@ -1136,9 +1143,7 @@ decode_NXAST_RAW_OUTPUT_REG(const struct nx_action_output_reg *naor,
>      output_reg->src.n_bits = nxm_decode_n_bits(naor->ofs_nbits);
>      output_reg->max_len = ntohs(naor->max_len);
>
> -    if (mf_vl_mff_invalid(output_reg->src.field, vl_mff_map)) {
> -        return OFPERR_NXFMFC_INVALID_TLV_FIELD;
> -    }
> +    CHECK_VL_MFF(output_reg->src.field, vl_mff_map, tlv_bitmap, error);
>
>      return mf_check_src(&output_reg->src, NULL);
>  }
> @@ -1147,7 +1152,7 @@ static enum ofperr
>  decode_NXAST_RAW_OUTPUT_REG2(const struct nx_action_output_reg2 *naor,
>                               enum ofp_version ofp_version OVS_UNUSED,
>                               const struct vl_mff_map *vl_mff_map,
> -                             struct ofpbuf *out)
> +                             uint64_t *tlv_bitmap, struct ofpbuf *out)
>  {
>      struct ofpact_output_reg *output_reg;
>      output_reg = ofpact_put_OUTPUT_REG(out);
> @@ -1164,6 +1169,8 @@ decode_NXAST_RAW_OUTPUT_REG2(const struct nx_action_output_reg2 *naor,
>      if (error) {
>          return error;
>      }
> +    mf_vl_mff_set_tlv_bitmap(output_reg->src.field, tlv_bitmap);
> +

I wonder if it's cleaner to fold this into nx_pull_header().

It's not so obvious why the ofpacts decode functions in this file
inconsistently need to call the mf_check_vl_mff() and/or
mf_vl_mff_set_tlv_bitmap(), which makes it a little more difficult to
verify (and easier to mess up if new actions are added). It'd be nice
to make this more consistent.

>      if (!is_all_zeros(b.data, b.size)) {
>          return OFPERR_NXBRC_MUST_BE_ZERO;
>      }
> @@ -1286,7 +1293,8 @@ OFP_ASSERT(sizeof(struct nx_action_bundle) == 32);
>
>  static enum ofperr
>  decode_bundle(bool load, const struct nx_action_bundle *nab,
> -              const struct vl_mff_map *vl_mff_map, struct ofpbuf *ofpacts)
> +              const struct vl_mff_map *vl_mff_map, uint64_t *tlv_bitmap,
> +              struct ofpbuf *ofpacts)
>  {
>      static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
>      struct ofpact_bundle *bundle;
> @@ -1326,9 +1334,7 @@ decode_bundle(bool load, const struct nx_action_bundle *nab,
>          bundle->dst.field = mf_from_nxm_header(ntohl(nab->dst), vl_mff_map);
>          bundle->dst.ofs = nxm_decode_ofs(nab->ofs_nbits);
>          bundle->dst.n_bits = nxm_decode_n_bits(nab->ofs_nbits);
> -        if (mf_vl_mff_invalid(bundle->dst.field, vl_mff_map)) {
> -            return OFPERR_NXFMFC_INVALID_TLV_FIELD;
> -        }
> +        CHECK_VL_MFF(bundle->dst.field, vl_mff_map, tlv_bitmap, error);
>
>          if (bundle->dst.n_bits < 16) {
>              VLOG_WARN_RL(&rl, "bundle_load action requires at least 16 bit "
> @@ -1369,16 +1375,16 @@ decode_NXAST_RAW_BUNDLE(const struct nx_action_bundle *nab,
>                          enum ofp_version ofp_version OVS_UNUSED,
>                          struct ofpbuf *out)
>  {
> -    return decode_bundle(false, nab, NULL, out);
> +    return decode_bundle(false, nab, NULL, NULL, out);
>  }
>
>  static enum ofperr
>  decode_NXAST_RAW_BUNDLE_LOAD(const struct nx_action_bundle *nab,
>                               enum ofp_version ofp_version OVS_UNUSED,
>                               const struct vl_mff_map *vl_mff_map,
> -                             struct ofpbuf *out)
> +                             uint64_t *tlv_bitmap, struct ofpbuf *out)
>  {
> -    return decode_bundle(true, nab, vl_mff_map, out);
> +    return decode_bundle(true, nab, vl_mff_map, tlv_bitmap, out);
>  }
>
>  static void
> @@ -2282,7 +2288,7 @@ static enum ofperr
>  decode_copy_field__(ovs_be16 src_offset, ovs_be16 dst_offset, ovs_be16 n_bits,
>                      const void *action, ovs_be16 action_len, size_t oxm_offset,
>                      const struct vl_mff_map *vl_mff_map,
> -                    struct ofpbuf *ofpacts)
> +                    uint64_t *tlv_bitmap, struct ofpbuf *ofpacts)
>  {
>      struct ofpact_reg_move *move = ofpact_put_REG_MOVE(ofpacts);
>      move->ofpact.raw = ONFACT_RAW13_COPY_FIELD;
> @@ -2298,10 +2304,13 @@ decode_copy_field__(ovs_be16 src_offset, ovs_be16 dst_offset, ovs_be16 n_bits,
>      if (error) {
>          return error;
>      }
> +    mf_vl_mff_set_tlv_bitmap(move->src.field, tlv_bitmap);
> +
>      error = nx_pull_header(&b, vl_mff_map, &move->dst.field, NULL);
>      if (error) {
>          return error;
>      }
> +    mf_vl_mff_set_tlv_bitmap(move->dst.field, tlv_bitmap);
>
>      if (!is_all_zeros(b.data, b.size)) {
>          return OFPERR_NXBRC_MUST_BE_ZERO;
> @@ -2314,31 +2323,31 @@ static enum ofperr
>  decode_OFPAT_RAW15_COPY_FIELD(const struct ofp15_action_copy_field *oacf,
>                                enum ofp_version ofp_version OVS_UNUSED,
>                                const struct vl_mff_map *vl_mff_map,
> -                              struct ofpbuf *ofpacts)
> +                              uint64_t *tlv_bitmap, struct ofpbuf *ofpacts)
>  {
>      return decode_copy_field__(oacf->src_offset, oacf->dst_offset,
>                                 oacf->n_bits, oacf, oacf->len,
>                                 OBJECT_OFFSETOF(oacf, pad2), vl_mff_map,
> -                               ofpacts);
> +                               tlv_bitmap, ofpacts);
>  }
>
>  static enum ofperr
>  decode_ONFACT_RAW13_COPY_FIELD(const struct onf_action_copy_field *oacf,
>                                 enum ofp_version ofp_version OVS_UNUSED,
>                                 const struct vl_mff_map *vl_mff_map,
> -                               struct ofpbuf *ofpacts)
> +                               uint64_t *tlv_bitmap, struct ofpbuf *ofpacts)
>  {
>      return decode_copy_field__(oacf->src_offset, oacf->dst_offset,
>                                 oacf->n_bits, oacf, oacf->len,
>                                 OBJECT_OFFSETOF(oacf, pad3), vl_mff_map,
> -                               ofpacts);
> +                               tlv_bitmap, ofpacts);
>  }
>
>  static enum ofperr
>  decode_NXAST_RAW_REG_MOVE(const struct nx_action_reg_move *narm,
>                            enum ofp_version ofp_version OVS_UNUSED,
>                            const struct vl_mff_map *vl_mff_map,
> -                          struct ofpbuf *ofpacts)
> +                          uint64_t *tlv_bitmap, struct ofpbuf *ofpacts)
>  {
>      struct ofpact_reg_move *move = ofpact_put_REG_MOVE(ofpacts);
>      move->ofpact.raw = NXAST_RAW_REG_MOVE;
> @@ -2354,10 +2363,13 @@ decode_NXAST_RAW_REG_MOVE(const struct nx_action_reg_move *narm,
>      if (error) {
>          return error;
>      }
> +    mf_vl_mff_set_tlv_bitmap(move->src.field, tlv_bitmap);
> +
>      error = nx_pull_header(&b, vl_mff_map, &move->dst.field, NULL);
>      if (error) {
>          return error;
>      }
> +    mf_vl_mff_set_tlv_bitmap(move->dst.field, tlv_bitmap);
>
>      if (!is_all_zeros(b.data, b.size)) {
>          return OFPERR_NXBRC_MUST_BE_ZERO;
> @@ -2481,7 +2493,7 @@ OFP_ASSERT(sizeof(struct nx_action_reg_load) == 24);
>  static enum ofperr
>  decode_ofpat_set_field(const struct ofp12_action_set_field *oasf,
>                         bool may_mask, const struct vl_mff_map *vl_mff_map,
> -                       struct ofpbuf *ofpacts)
> +                       uint64_t *tlv_bitmap, struct ofpbuf *ofpacts)
>  {
>      struct ofpbuf b = ofpbuf_const_initializer(oasf, ntohs(oasf->len));
>      ofpbuf_pull(&b, OBJECT_OFFSETOF(oasf, pad));
> @@ -2495,6 +2507,8 @@ decode_ofpat_set_field(const struct ofp12_action_set_field *oasf,
>                  ? OFPERR_OFPBAC_BAD_SET_MASK
>                  : error);
>      }
> +    mf_vl_mff_set_tlv_bitmap(field, tlv_bitmap);
> +
>      if (!may_mask) {
>          memset(&mask, 0xff, field->n_bytes);
>      }
> @@ -2540,25 +2554,26 @@ static enum ofperr
>  decode_OFPAT_RAW12_SET_FIELD(const struct ofp12_action_set_field *oasf,
>                               enum ofp_version ofp_version OVS_UNUSED,
>                               const struct vl_mff_map *vl_mff_map,
> -                             struct ofpbuf *ofpacts)
> +                             uint64_t *tlv_bitmap, struct ofpbuf *ofpacts)
>  {
> -    return decode_ofpat_set_field(oasf, false, vl_mff_map, ofpacts);
> +    return decode_ofpat_set_field(oasf, false, vl_mff_map, tlv_bitmap,
> +                                  ofpacts);
>  }
>
>  static enum ofperr
>  decode_OFPAT_RAW15_SET_FIELD(const struct ofp12_action_set_field *oasf,
>                               enum ofp_version ofp_version OVS_UNUSED,
>                               const struct vl_mff_map *vl_mff_map,
> -                             struct ofpbuf *ofpacts)
> +                             uint64_t *tlv_bitmap, struct ofpbuf *ofpacts)
>  {
> -    return decode_ofpat_set_field(oasf, true, vl_mff_map, ofpacts);
> +    return decode_ofpat_set_field(oasf, true, vl_mff_map, tlv_bitmap, ofpacts);
>  }
>
>  static enum ofperr
>  decode_NXAST_RAW_REG_LOAD(const struct nx_action_reg_load *narl,
>                            enum ofp_version ofp_version OVS_UNUSED,
>                            const struct vl_mff_map *vl_mff_map,
> -                          struct ofpbuf *out)
> +                          uint64_t *tlv_bitmap, struct ofpbuf *out)
>  {
>      struct mf_subfield dst;
>      enum ofperr error;
> @@ -2566,9 +2581,7 @@ decode_NXAST_RAW_REG_LOAD(const struct nx_action_reg_load *narl,
>      dst.field = mf_from_nxm_header(ntohl(narl->dst), vl_mff_map);
>      dst.ofs = nxm_decode_ofs(narl->ofs_nbits);
>      dst.n_bits = nxm_decode_n_bits(narl->ofs_nbits);
> -    if (mf_vl_mff_invalid(dst.field, vl_mff_map)) {
> -        return OFPERR_NXFMFC_INVALID_TLV_FIELD;
> -    }
> +    CHECK_VL_MFF(dst.field, vl_mff_map, tlv_bitmap, error);
>
>      error = mf_check_dst(&dst, NULL);
>      if (error) {
> @@ -2596,7 +2609,7 @@ static enum ofperr
>  decode_NXAST_RAW_REG_LOAD2(const struct ext_action_header *eah,
>                             enum ofp_version ofp_version OVS_UNUSED,
>                             const struct vl_mff_map *vl_mff_map,
> -                           struct ofpbuf *out)
> +                           uint64_t *tlv_bitmap, struct ofpbuf *out)
>  {
>      struct ofpbuf b = ofpbuf_const_initializer(eah, ntohs(eah->len));
>      ofpbuf_pull(&b, OBJECT_OFFSETOF(eah, pad));
> @@ -2607,6 +2620,8 @@ decode_NXAST_RAW_REG_LOAD2(const struct ext_action_header *eah,
>      if (error) {
>          return error;
>      }
> +    mf_vl_mff_set_tlv_bitmap(field, tlv_bitmap);
> +
>      if (!is_all_zeros(b.data, b.size)) {
>          return OFPERR_OFPBAC_BAD_SET_ARGUMENT;
>      }
> @@ -3109,7 +3124,7 @@ OFP_ASSERT(sizeof(struct nx_action_stack) == 24);
>
>  static enum ofperr
>  decode_stack_action(const struct nx_action_stack *nasp,
> -                    const struct vl_mff_map *vl_mff_map,
> +                    const struct vl_mff_map *vl_mff_map, uint64_t *tlv_bitmap,
>                      struct ofpact_stack *stack_action)
>  {
>      stack_action->subfield.ofs = ntohs(nasp->offset);
> @@ -3121,6 +3136,8 @@ decode_stack_action(const struct nx_action_stack *nasp,
>      if (error) {
>          return error;
>      }
> +    mf_vl_mff_set_tlv_bitmap(stack_action->subfield.field, tlv_bitmap);
> +
>      stack_action->subfield.n_bits = ntohs(*(const ovs_be16 *) b.data);
>      ofpbuf_pull(&b, 2);
>      if (!is_all_zeros(b.data, b.size)) {
> @@ -3134,10 +3151,11 @@ static enum ofperr
>  decode_NXAST_RAW_STACK_PUSH(const struct nx_action_stack *nasp,
>                              enum ofp_version ofp_version OVS_UNUSED,
>                              const struct vl_mff_map *vl_mff_map,
> -                            struct ofpbuf *ofpacts)
> +                            uint64_t *tlv_bitmap, struct ofpbuf *ofpacts)
>  {
>      struct ofpact_stack *push = ofpact_put_STACK_PUSH(ofpacts);
> -    enum ofperr error = decode_stack_action(nasp, vl_mff_map, push);
> +    enum ofperr error = decode_stack_action(nasp, vl_mff_map, tlv_bitmap,
> +                                            push);
>      return error ? error : nxm_stack_push_check(push, NULL);
>  }
>
> @@ -3145,10 +3163,11 @@ static enum ofperr
>  decode_NXAST_RAW_STACK_POP(const struct nx_action_stack *nasp,
>                             enum ofp_version ofp_version OVS_UNUSED,
>                             const struct vl_mff_map *vl_mff_map,
> -                           struct ofpbuf *ofpacts)
> +                           uint64_t *tlv_bitmap, struct ofpbuf *ofpacts)
>  {
>      struct ofpact_stack *pop = ofpact_put_STACK_POP(ofpacts);
> -    enum ofperr error = decode_stack_action(nasp, vl_mff_map, pop);
> +    enum ofperr error = decode_stack_action(nasp, vl_mff_map, tlv_bitmap,
> +                                            pop);
>      return error ? error : nxm_stack_pop_check(pop, NULL);
>  }
>
> @@ -4279,14 +4298,14 @@ get_be32(const void **pp)
>
>  static enum ofperr
>  get_subfield(int n_bits, const void **p, struct mf_subfield *sf,
> -             const struct vl_mff_map *vl_mff_map)
> +             const struct vl_mff_map *vl_mff_map, uint64_t *tlv_bitmap)
>  {
> +    enum ofperr error;
> +
>      sf->field = mf_from_nxm_header(ntohl(get_be32(p)), vl_mff_map);
>      sf->ofs = ntohs(get_be16(p));
>      sf->n_bits = n_bits;
> -    if (mf_vl_mff_invalid(sf->field, vl_mff_map)) {
> -        return OFPERR_NXFMFC_INVALID_TLV_FIELD;
> -    }
> +    CHECK_VL_MFF(sf->field, vl_mff_map, tlv_bitmap, error);
>      return 0;
>  }
>
> @@ -4319,7 +4338,7 @@ static enum ofperr
>  decode_NXAST_RAW_LEARN(const struct nx_action_learn *nal,
>                         enum ofp_version ofp_version OVS_UNUSED,
>                         const struct vl_mff_map *vl_mff_map,
> -                       struct ofpbuf *ofpacts)
> +                       uint64_t *tlv_bitmap, struct ofpbuf *ofpacts)
>  {
>      struct ofpact_learn *learn;
>      const void *p, *end;
> @@ -4384,7 +4403,8 @@ decode_NXAST_RAW_LEARN(const struct nx_action_learn *nal,
>          unsigned int imm_bytes = 0;
>          enum ofperr error;
>          if (spec->src_type == NX_LEARN_SRC_FIELD) {
> -            error = get_subfield(spec->n_bits, &p, &spec->src, vl_mff_map);
> +            error = get_subfield(spec->n_bits, &p, &spec->src, vl_mff_map,
> +                                 tlv_bitmap);
>              if (error) {
>                  return error;
>              }
> @@ -4399,7 +4419,8 @@ decode_NXAST_RAW_LEARN(const struct nx_action_learn *nal,
>          /* Get the destination. */
>          if (spec->dst_type == NX_LEARN_DST_MATCH ||
>              spec->dst_type == NX_LEARN_DST_LOAD) {
> -            error = get_subfield(spec->n_bits, &p, &spec->dst, vl_mff_map);
> +            error = get_subfield(spec->n_bits, &p, &spec->dst, vl_mff_map,
> +                                 tlv_bitmap);
>              if (error) {
>                  return error;
>              }
> @@ -4649,11 +4670,12 @@ static enum ofperr
>  decode_NXAST_RAW_MULTIPATH(const struct nx_action_multipath *nam,
>                             enum ofp_version ofp_version OVS_UNUSED,
>                             const struct vl_mff_map *vl_mff_map,
> -                           struct ofpbuf *out)
> +                           uint64_t *tlv_bitmap, struct ofpbuf *out)
>  {
>      uint32_t n_links = ntohs(nam->max_link) + 1;
>      size_t min_n_bits = log_2_ceil(n_links);
>      struct ofpact_multipath *mp;
> +    enum ofperr error;
>
>      mp = ofpact_put_MULTIPATH(out);
>      mp->fields = ntohs(nam->fields);
> @@ -4665,9 +4687,7 @@ decode_NXAST_RAW_MULTIPATH(const struct nx_action_multipath *nam,
>      mp->dst.ofs = nxm_decode_ofs(nam->ofs_nbits);
>      mp->dst.n_bits = nxm_decode_n_bits(nam->ofs_nbits);
>
> -    if (mf_vl_mff_invalid(mp->dst.field, vl_mff_map)) {
> -        return OFPERR_NXFMFC_INVALID_TLV_FIELD;
> -    }
> +    CHECK_VL_MFF(mp->dst.field, vl_mff_map, tlv_bitmap, error);
>
>      if (!flow_hash_fields_valid(mp->fields)) {
>          VLOG_WARN_RL(&rl, "unsupported fields %d", (int) mp->fields);
> @@ -4855,7 +4875,7 @@ static enum ofperr
>  decode_NXAST_RAW_CLONE(const struct ext_action_header *eah,
>                         enum ofp_version ofp_version,
>                         const struct vl_mff_map *vl_mff_map,
> -                       struct ofpbuf *out)
> +                       uint64_t *tlv_bitmap, struct ofpbuf *out)
>  {
>      int error;
>      struct ofpbuf openflow;
> @@ -4869,7 +4889,7 @@ decode_NXAST_RAW_CLONE(const struct ext_action_header *eah,
>      error = ofpacts_pull_openflow_actions__(&openflow, openflow.size,
>                                              ofp_version,
>                                              1u << OVSINST_OFPIT11_APPLY_ACTIONS,
> -                                            out, 0, vl_mff_map);
> +                                            out, 0, vl_mff_map, tlv_bitmap);
>      clone = ofpbuf_push_uninit(out, sizeof *clone);
>      out->header = &clone->ofpact;
>      ofpact_finish_CLONE(out, &clone);
> @@ -5316,7 +5336,7 @@ OFP_ASSERT(sizeof(struct nx_action_conntrack) == 24);
>  static enum ofperr
>  decode_ct_zone(const struct nx_action_conntrack *nac,
>                 struct ofpact_conntrack *out,
> -               const struct vl_mff_map *vl_mff_map)
> +               const struct vl_mff_map *vl_mff_map, uint64_t *tlv_bitmap)
>  {
>      if (nac->zone_src) {
>          enum ofperr error;
> @@ -5325,9 +5345,7 @@ decode_ct_zone(const struct nx_action_conntrack *nac,
>                                                   vl_mff_map);
>          out->zone_src.ofs = nxm_decode_ofs(nac->zone_ofs_nbits);
>          out->zone_src.n_bits = nxm_decode_n_bits(nac->zone_ofs_nbits);
> -        if (mf_vl_mff_invalid(out->zone_src.field, vl_mff_map)) {
> -            return OFPERR_NXFMFC_INVALID_TLV_FIELD;
> -        }
> +        CHECK_VL_MFF(out->zone_src.field, vl_mff_map, tlv_bitmap, error);
>
>          error = mf_check_src(&out->zone_src, NULL);
>          if (error) {
> @@ -5350,13 +5368,14 @@ decode_ct_zone(const struct nx_action_conntrack *nac,
>  static enum ofperr
>  decode_NXAST_RAW_CT(const struct nx_action_conntrack *nac,
>                      enum ofp_version ofp_version,
> -                    const struct vl_mff_map *vl_mff_map, struct ofpbuf *out)
> +                    const struct vl_mff_map *vl_mff_map, uint64_t *tlv_bitmap,
> +                    struct ofpbuf *out)
>  {
>      const size_t ct_offset = ofpacts_pull(out);
>      struct ofpact_conntrack *conntrack = ofpact_put_CT(out);
>      conntrack->flags = ntohs(nac->flags);
>
> -    int error = decode_ct_zone(nac, conntrack, vl_mff_map);
> +    int error = decode_ct_zone(nac, conntrack, vl_mff_map, tlv_bitmap);
>      if (error) {
>          goto out;
>      }
> @@ -5370,7 +5389,8 @@ decode_NXAST_RAW_CT(const struct nx_action_conntrack *nac,
>      error = ofpacts_pull_openflow_actions__(&openflow, openflow.size,
>                                              ofp_version,
>                                              1u << OVSINST_OFPIT11_APPLY_ACTIONS,
> -                                            out, OFPACT_CT, vl_mff_map);
> +                                            out, OFPACT_CT, vl_mff_map,
> +                                            tlv_bitmap);
>      if (error) {
>          goto out;
>      }
> @@ -6256,7 +6276,8 @@ log_bad_action(const struct ofp_action_header *actions, size_t actions_len,
>  static enum ofperr
>  ofpacts_decode(const void *actions, size_t actions_len,
>                 enum ofp_version ofp_version,
> -               const struct vl_mff_map *vl_mff_map, struct ofpbuf *ofpacts)
> +               const struct vl_mff_map *vl_mff_map,
> +               uint64_t *ofpacts_tlv_bitmap, struct ofpbuf *ofpacts)
>  {
>      struct ofpbuf openflow = ofpbuf_const_initializer(actions, actions_len);
>      while (openflow.size) {
> @@ -6268,7 +6289,7 @@ ofpacts_decode(const void *actions, size_t actions_len,
>          error = ofpact_pull_raw(&openflow, ofp_version, &raw, &arg);
>          if (!error) {
>              error = ofpact_decode(action, raw, ofp_version, arg, vl_mff_map,
> -                                  ofpacts);
> +                                  ofpacts_tlv_bitmap, ofpacts);
>          }
>
>          if (error) {
> @@ -6286,7 +6307,8 @@ ofpacts_pull_openflow_actions__(struct ofpbuf *openflow,
>                                  uint32_t allowed_ovsinsts,
>                                  struct ofpbuf *ofpacts,
>                                  enum ofpact_type outer_action,
> -                                const struct vl_mff_map *vl_mff_map)
> +                                const struct vl_mff_map *vl_mff_map,
> +                                uint64_t *ofpacts_tlv_bitmap)
>  {
>      const struct ofp_action_header *actions;
>      size_t orig_size = ofpacts->size;
> @@ -6306,7 +6328,8 @@ ofpacts_pull_openflow_actions__(struct ofpbuf *openflow,
>          return OFPERR_OFPBRC_BAD_LEN;
>      }
>
> -    error = ofpacts_decode(actions, actions_len, version, vl_mff_map, ofpacts);
> +    error = ofpacts_decode(actions, actions_len, version, vl_mff_map,
> +                           ofpacts_tlv_bitmap, ofpacts);
>      if (error) {
>          ofpacts->size = orig_size;
>          return error;
> @@ -6342,11 +6365,13 @@ ofpacts_pull_openflow_actions(struct ofpbuf *openflow,
>                                unsigned int actions_len,
>                                enum ofp_version version,
>                                const struct vl_mff_map *vl_mff_map,
> +                              uint64_t *ofpacts_tlv_bitmap,
>                                struct ofpbuf *ofpacts)
>  {
>      return ofpacts_pull_openflow_actions__(openflow, actions_len, version,
>                                             1u << OVSINST_OFPIT11_APPLY_ACTIONS,
> -                                           ofpacts, 0, vl_mff_map);
> +                                           ofpacts, 0, vl_mff_map,
> +                                           ofpacts_tlv_bitmap);
>  }
>
>  /* OpenFlow 1.1 actions. */
> @@ -6586,13 +6611,15 @@ static enum ofperr
>  ofpacts_decode_for_action_set(const struct ofp_action_header *in,
>                                size_t n_in, enum ofp_version version,
>                                const struct vl_mff_map *vl_mff_map,
> +                              uint64_t *ofpacts_tlv_bitmap,
>                                struct ofpbuf *out)
>  {
>      enum ofperr error;
>      struct ofpact *a;
>      size_t start = out->size;
>
> -    error = ofpacts_decode(in, n_in, version, vl_mff_map, out);
> +    error = ofpacts_decode(in, n_in, version, vl_mff_map, ofpacts_tlv_bitmap,
> +                           out);
>
>      if (error) {
>          return error;
> @@ -6891,6 +6918,7 @@ ofpacts_pull_openflow_instructions(struct ofpbuf *openflow,
>                                     unsigned int instructions_len,
>                                     enum ofp_version version,
>                                     const struct vl_mff_map *vl_mff_map,
> +                                   uint64_t *ofpacts_tlv_bitmap,
>                                     struct ofpbuf *ofpacts)
>  {
>      const struct ofp11_instruction *instructions;
> @@ -6902,7 +6930,8 @@ ofpacts_pull_openflow_instructions(struct ofpbuf *openflow,
>          return ofpacts_pull_openflow_actions__(openflow, instructions_len,
>                                                 version,
>                                                 (1u << N_OVS_INSTRUCTIONS) - 1,
> -                                               ofpacts, 0, vl_mff_map);
> +                                               ofpacts, 0, vl_mff_map,
> +                                               ofpacts_tlv_bitmap);
>      }
>
>      if (instructions_len % OFP11_INSTRUCTION_ALIGN != 0) {
> @@ -6946,7 +6975,7 @@ ofpacts_pull_openflow_instructions(struct ofpbuf *openflow,
>          get_actions_from_instruction(insts[OVSINST_OFPIT11_APPLY_ACTIONS],
>                                       &actions, &actions_len);
>          error = ofpacts_decode(actions, actions_len, version, vl_mff_map,
> -                               ofpacts);
> +                               ofpacts_tlv_bitmap, ofpacts);
>          if (error) {
>              goto exit;
>          }
> @@ -6966,7 +6995,8 @@ ofpacts_pull_openflow_instructions(struct ofpbuf *openflow,
>          get_actions_from_instruction(insts[OVSINST_OFPIT11_WRITE_ACTIONS],
>                                       &actions, &actions_len);
>          error = ofpacts_decode_for_action_set(actions, actions_len,
> -                                              version, vl_mff_map, ofpacts);
> +                                              version, vl_mff_map,
> +                                              ofpacts_tlv_bitmap, ofpacts);
>          if (error) {
>              goto exit;
>          }
> diff --git a/lib/ofp-util.c b/lib/ofp-util.c
> index f7da90b276f9..96b4f8d22333 100644
> --- a/lib/ofp-util.c
> +++ b/lib/ofp-util.c
> @@ -1726,8 +1726,11 @@ ofputil_decode_flow_mod(struct ofputil_flow_mod *fm,
>          return OFPERR_OFPFMFC_BAD_COMMAND;
>      }
>
> +    fm->ofpacts_tlv_bitmap = 0;
>      error = ofpacts_pull_openflow_instructions(&b, b.size, oh->version,
> -                                               vl_mff_map, ofpacts);
> +                                               vl_mff_map,
> +                                               &fm->ofpacts_tlv_bitmap,
> +                                               ofpacts);
>      if (error) {
>          return error;
>      }
> @@ -3009,7 +3012,7 @@ ofputil_decode_flow_stats_reply(struct ofputil_flow_stats *fs,
>      }
>
>      if (ofpacts_pull_openflow_instructions(msg, instructions_len, oh->version,
> -                                           NULL, ofpacts)) {
> +                                           NULL, NULL, ofpacts)) {
>          VLOG_WARN_RL(&bad_ofmsg_rl, "OFPST_FLOW reply bad instructions");
>          return EINVAL;
>      }
> @@ -4034,7 +4037,7 @@ parse_actions_property(struct ofpbuf *property, enum ofp_version version,
>      }
>
>      return ofpacts_pull_openflow_actions(property, property->size,
> -                                         version, NULL, ofpacts);
> +                                         version, NULL, NULL, ofpacts);
>  }
>
>  /* This is like ofputil_decode_packet_in(), except that it decodes the
> @@ -4189,7 +4192,8 @@ ofputil_decode_packet_out(struct ofputil_packet_out *po,
>          }
>
>          error = ofpacts_pull_openflow_actions(&b, ntohs(opo->actions_len),
> -                                              oh->version, NULL, ofpacts);
> +                                              oh->version, NULL, NULL,
> +                                              ofpacts);
>          if (error) {
>              return error;
>          }
> @@ -4201,7 +4205,8 @@ ofputil_decode_packet_out(struct ofputil_packet_out *po,
>          po->in_port = u16_to_ofp(ntohs(opo->in_port));
>
>          error = ofpacts_pull_openflow_actions(&b, ntohs(opo->actions_len),
> -                                              oh->version, NULL, ofpacts);
> +                                              oh->version, NULL, NULL,
> +                                              ofpacts);
>          if (error) {
>              return error;
>          }
> @@ -6743,7 +6748,7 @@ ofputil_decode_flow_update(struct ofputil_flow_update *update,
>
>          actions_len = length - sizeof *nfuf - ROUND_UP(match_len, 8);
>          error = ofpacts_pull_openflow_actions(msg, actions_len, oh->version,
> -                                              NULL, ofpacts);
> +                                              NULL, NULL, ofpacts);
>          if (error) {
>              return error;
>          }
> @@ -8727,7 +8732,7 @@ ofputil_pull_ofp11_buckets(struct ofpbuf *msg, size_t buckets_length,
>
>          ofpbuf_init(&ofpacts, 0);
>          error = ofpacts_pull_openflow_actions(msg, ob_len - sizeof *ob,
> -                                              version, NULL, &ofpacts);
> +                                              version, NULL, NULL, &ofpacts);
>          if (error) {
>              ofpbuf_uninit(&ofpacts);
>              ofputil_bucket_list_destroy(buckets);
> @@ -8801,7 +8806,7 @@ ofputil_pull_ofp15_buckets(struct ofpbuf *msg, size_t buckets_length,
>          buckets_length -= ob_len;
>
>          err = ofpacts_pull_openflow_actions(msg, actions_len, version,
> -                                            NULL, &ofpacts);
> +                                            NULL, NULL, &ofpacts);
>          if (err) {
>              goto err;
>          }
> diff --git a/lib/vl-mff-map.h b/lib/vl-mff-map.h
> index 1c29385782ec..a47b8aa8a6f9 100644
> --- a/lib/vl-mff-map.h
> +++ b/lib/vl-mff-map.h
> @@ -20,6 +20,8 @@
>  #include "cmap.h"
>  #include "openvswitch/thread.h"
>
> +struct rule;
> +
>  /* 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. */
> @@ -29,7 +31,7 @@ struct vl_mff_map {
>  };
>
>  /* Variable length fields. */
> -void mf_vl_mff_map_clear(struct vl_mff_map *vl_mff_map)
> +enum ofperr mf_vl_mff_map_clear(struct vl_mff_map *vl_mff_map, bool)
>      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 *)
> @@ -37,5 +39,9 @@ enum ofperr mf_vl_mff_map_mod_from_tun_metadata(
>  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 *);
> -
> +void mf_vl_mff_set_tlv_bitmap(const struct mf_field *, uint64_t *);
> +enum ofperr mf_check_vl_mff(const struct mf_field *, const struct vl_mff_map *,
> +                            uint64_t *);
> +void mf_vl_mff_add_ref_cnt_from_rule(const struct rule *);
> +void mf_vl_mff_remove_ref_cnt_from_rule(const struct rule *);
>  #endif /* vl-mff-map.h */
> diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
> index 7d3e929f21e3..dab74e3a195f 100644
> --- a/ofproto/ofproto-provider.h
> +++ b/ofproto/ofproto-provider.h
> @@ -423,6 +423,10 @@ struct rule {
>
>      /* Must hold 'mutex' for both read/write, 'ofproto_mutex' not needed. */
>      long long int modified OVS_GUARDED; /* Time of last modification. */
> +
> +    /* 1-bit for each present TLV in flow match / action. */
> +    uint64_t match_tlv_bitmap;
> +    uint64_t ofpacts_tlv_bitmap;
>  };
>
>  void ofproto_rule_ref(struct rule *);
> diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
> index 699c37cd5c3e..cf4db291e12a 100644
> --- a/ofproto/ofproto.c
> +++ b/ofproto/ofproto.c
> @@ -228,6 +228,8 @@ static enum ofperr ofproto_rule_create(struct ofproto *, struct cls_rule *,
>                                         uint16_t importance,
>                                         const struct ofpact *ofpacts,
>                                         size_t ofpacts_len,
> +                                       uint64_t match_tlv_bitmap,
> +                                       uint64_t ofpacts_tlv_bitmap,
>                                         struct rule **new_rule)
>      OVS_NO_THREAD_SAFETY_ANALYSIS;
>
> @@ -1580,12 +1582,6 @@ ofproto_destroy__(struct ofproto *ofproto)
>      tun_metadata_free(ovsrcu_get_protected(struct tun_table *,
>                                             &ofproto->metadata_tab));
>
> -    ovs_mutex_lock(&ofproto->vl_mff_map.mutex);
> -    mf_vl_mff_map_clear(&ofproto->vl_mff_map);
> -    ovs_mutex_unlock(&ofproto->vl_mff_map.mutex);
> -    cmap_destroy(&ofproto->vl_mff_map.cmap);
> -    ovs_mutex_destroy(&ofproto->vl_mff_map.mutex);
> -
>      free(ofproto->name);
>      free(ofproto->type);
>      free(ofproto->mfr_desc);
> @@ -1603,6 +1599,12 @@ ofproto_destroy__(struct ofproto *ofproto)
>      }
>      free(ofproto->tables);
>
> +    ovs_mutex_lock(&ofproto->vl_mff_map.mutex);
> +    mf_vl_mff_map_clear(&ofproto->vl_mff_map, true);
> +    ovs_mutex_unlock(&ofproto->vl_mff_map.mutex);
> +    cmap_destroy(&ofproto->vl_mff_map.cmap);
> +    ovs_mutex_destroy(&ofproto->vl_mff_map.mutex);
> +
>      ovs_assert(hindex_is_empty(&ofproto->cookies));
>      hindex_destroy(&ofproto->cookies);
>

Is there a reason for the above two hunks to shift this logic later?
Should this be in a separate patch to explain why, or could it be
dropped?

> @@ -2829,6 +2831,7 @@ rule_destroy_cb(struct rule *rule)
>          ofproto_rule_send_removed(rule);
>      }
>      rule->ofproto->ofproto_class->rule_destruct(rule);
> +    mf_vl_mff_remove_ref_cnt_from_rule(rule);
>      ofproto_rule_destroy__(rule);
>  }
>
> @@ -4741,7 +4744,9 @@ add_flow_init(struct ofproto *ofproto, struct ofproto_flow_mod *ofm,
>                                      fm->new_cookie, fm->idle_timeout,
>                                      fm->hard_timeout, fm->flags,
>                                      fm->importance, fm->ofpacts,
> -                                    fm->ofpacts_len, &ofm->temp_rule);
> +                                    fm->ofpacts_len,
> +                                    fm->match.flow.tunnel.metadata.present.map,
> +                                    fm->ofpacts_tlv_bitmap, &ofm->temp_rule);
>          if (error) {
>              return error;
>          }
> @@ -4856,6 +4861,7 @@ ofproto_rule_create(struct ofproto *ofproto, struct cls_rule *cr,
>                      uint16_t idle_timeout, uint16_t hard_timeout,
>                      enum ofputil_flow_mod_flags flags, uint16_t importance,
>                      const struct ofpact *ofpacts, size_t ofpacts_len,
> +                    uint64_t match_tlv_bitmap, uint64_t ofpacts_tlv_bitmap,
>                      struct rule **new_rule)
>      OVS_NO_THREAD_SAFETY_ANALYSIS
>  {
> @@ -4906,6 +4912,9 @@ ofproto_rule_create(struct ofproto *ofproto, struct cls_rule *cr,
>      }
>
>      rule->state = RULE_INITIALIZED;
> +    rule->match_tlv_bitmap = match_tlv_bitmap;
> +    rule->ofpacts_tlv_bitmap = ofpacts_tlv_bitmap;
> +    mf_vl_mff_add_ref_cnt_from_rule(rule);
>
>      *new_rule = rule;
>      return 0;
> @@ -5003,6 +5012,8 @@ ofproto_flow_mod_learn_refresh(struct ofproto_flow_mod *ofm)
>                                      rule->importance,
>                                      rule->actions->ofpacts,
>                                      rule->actions->ofpacts_len,
> +                                    rule->match_tlv_bitmap,
> +                                    rule->ofpacts_tlv_bitmap,
>                                      &ofm->temp_rule);
>          ovs_mutex_unlock(&rule->mutex);
>          if (!error) {
> @@ -5263,6 +5274,11 @@ modify_flows_start__(struct ofproto *ofproto, struct ofproto_flow_mod *ofm)
>                  cls_rule_destroy(CONST_CAST(struct cls_rule *, &temp->cr));
>                  cls_rule_clone(CONST_CAST(struct cls_rule *, &temp->cr),
>                                 &old_rule->cr);
> +                if (temp->match_tlv_bitmap != old_rule->match_tlv_bitmap) {
> +                    mf_vl_mff_remove_ref_cnt_from_rule(temp);
> +                    temp->match_tlv_bitmap = old_rule->match_tlv_bitmap;
> +                    mf_vl_mff_add_ref_cnt_from_rule(temp);
> +                }
>                  *CONST_CAST(uint8_t *, &temp->table_id) = old_rule->table_id;
>                  rule_collection_add(new_rules, temp);
>                  first = false;
> @@ -5276,6 +5292,8 @@ modify_flows_start__(struct ofproto *ofproto, struct ofproto_flow_mod *ofm)
>                                              temp->importance,
>                                              temp->actions->ofpacts,
>                                              temp->actions->ofpacts_len,
> +                                            old_rule->match_tlv_bitmap,
> +                                            temp->ofpacts_tlv_bitmap,
>                                              &new_rule);
>                  if (!error) {
>                      rule_collection_add(new_rules, new_rule);
> @@ -7784,14 +7802,15 @@ handle_tlv_table_mod(struct ofconn *ofconn, const struct ofp_header *oh)
>      old_tab = ovsrcu_get_protected(struct tun_table *, &ofproto->metadata_tab);
>      error = tun_metadata_table_mod(&ttm, old_tab, &new_tab);
>      if (!error) {
> -        ovsrcu_set(&ofproto->metadata_tab, new_tab);
> -        tun_metadata_postpone_free(old_tab);
> -
>          ovs_mutex_lock(&ofproto->vl_mff_map.mutex);
>          error = mf_vl_mff_map_mod_from_tun_metadata(&ofproto->vl_mff_map,
>                                                      &ttm);
>          ovs_mutex_unlock(&ofproto->vl_mff_map.mutex);
>      }
> +    if (!error) {
> +        ovsrcu_set(&ofproto->metadata_tab, new_tab);
> +        tun_metadata_postpone_free(old_tab);
> +    }

Maybe I didn't quite dig deep enough, but what's the implications of
moving this? Could it fix another bug, should it be a separate patch?

>
>      ofputil_uninit_tlv_table(&ttm.mappings);
>      return error;
> diff --git a/ovn/controller/pinctrl.c b/ovn/controller/pinctrl.c
> index 0380c8481ecf..890b47fe0d4f 100644
> --- a/ovn/controller/pinctrl.c
> +++ b/ovn/controller/pinctrl.c
> @@ -168,7 +168,8 @@ pinctrl_handle_arp(const struct flow *ip_flow, const struct match *md,
>
>      reload_metadata(&ofpacts, md);
>      enum ofperr error = ofpacts_pull_openflow_actions(userdata, userdata->size,
> -                                                      version, NULL, &ofpacts);
> +                                                      version, NULL, NULL,
> +                                                      &ofpacts);
>      if (error) {
>          static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
>          VLOG_WARN_RL(&rl, "failed to parse arp actions (%s)",
> @@ -1398,7 +1399,8 @@ pinctrl_handle_nd_na(const struct flow *ip_flow, const struct match *md,
>      reload_metadata(&ofpacts, md);
>
>      enum ofperr error = ofpacts_pull_openflow_actions(userdata, userdata->size,
> -                                                      version, NULL, &ofpacts);
> +                                                      version, NULL, NULL,
> +                                                      &ofpacts);
>      if (error) {
>          static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
>          VLOG_WARN_RL(&rl, "failed to parse actions for 'na' (%s)",
> diff --git a/tests/tunnel.at b/tests/tunnel.at
> index 1ba209dd4ce7..b9e9e21bfb3f 100644
> --- a/tests/tunnel.at
> +++ b/tests/tunnel.at
> @@ -535,7 +535,81 @@ AT_CHECK([tail -2 stdout], [0],
>  Datapath actions: 2
>  ])
>
> -AT_CHECK([ovs-ofctl del-tlv-map br0])
> +AT_CHECK([ovs-ofctl dump-flows br0 | ofctl_strip],
> +[0], [dnl
> +NXST_FLOW reply:
> + tun_metadata3=0x1234567890abcdef actions=output:2
> +])
> +
> +dnl A TLV mapping should not be removed if any active flow uses the mapping.
> +AT_CHECK([ovs-ofctl del-tlv-map br0], [1], [], [dnl
> +OFPT_ERROR (xid=0x4): NXTTMFC_INVALID_TLV_DEL
> +NXT_TLV_TABLE_MOD (xid=0x4):
> + CLEAR
> +])
> +
> +AT_CHECK([ovs-ofctl del-flows br0], [0])
> +AT_CHECK([ovs-ofctl del-tlv-map br0], [0])
> +
> +dnl Flow modification
> +AT_CHECK([ovs-ofctl add-tlv-map br0 "{class=0xffff,type=1,len=4}->tun_metadata0"])
> +AT_CHECK([ovs-ofctl add-tlv-map br0 "{class=0xffff,type=2,len=4}->tun_metadata1"])
> +AT_CHECK([ovs-ofctl add-tlv-map br0 "{class=0xffff,type=3,len=4}->tun_metadata2"])
> +
> +AT_CHECK([ovs-ofctl add-flow br0 "in_port=1 actions=multipath(eth_src,50,modulo_n,1,0,tun_metadata0[[0..31]])"])
> +AT_CHECK([ovs-ofctl add-flow br0 "in_port=2 actions=push:tun_metadata1[[0..31]],clone(move:tun_metadata2[[0..31]]->reg0[[0..31]])"])
> +
> +AT_CHECK([ovs-ofctl add-flow br0 "in_port=1 actions=output:4"])
> +AT_CHECK([ovs-ofctl del-tlv-map br0 "{class=0xffff,type=3,len=4}->tun_metadata0"])
> +
> +AT_CHECK([ovs-ofctl add-flow br0 "in_port=2 actions=push:tun_metadata2[[0..31]]"])
> +AT_CHECK([ovs-ofctl del-tlv-map br0 "{class=0xffff,type=2,len=4}->tun_metadata1"])
> +AT_CHECK([ovs-ofctl del-tlv-map br0 "{class=0xffff,type=3,len=4}->tun_metadata2"], [1], [], [dnl
> +OFPT_ERROR (xid=0x4): NXTTMFC_INVALID_TLV_DEL
> +NXT_TLV_TABLE_MOD (xid=0x4):
> + DEL mapping table:
> + class type    length  match field
> + ----- ----    ------  -----------
> + 0xffff        0x3     4       tun_metadata2
> +])
> +
> +AT_CHECK([ovs-ofctl del-flows br0], [0])
> +AT_CHECK([ovs-ofctl del-tlv-map br0], [0])
> +
> +dnl Learn action
> +AT_CHECK([ovs-ofctl add-tlv-map br0 "{class=0xffff,type=1,len=4}->tun_metadata1"])
> +AT_CHECK([ovs-ofctl add-tlv-map br0 "{class=0xffff,type=2,len=4}->tun_metadata2"])
> +AT_CHECK([ovs-ofctl add-flow br0 "in_port=2, eth_src=00:00:00:00:00:01 actions=learn(tun_metadata1[[0..31]]=reg1, output:NXM_OF_IN_PORT[[]])"])
> +AT_CHECK([ovs-ofctl add-flow br0 "in_port=2, eth_src=00:00:00:00:00:02 actions=learn(reg1[[0..31]]=0xFF, load:reg1[[0..31]]->tun_metadata2[[0..31]])"])
> +flow1="in_port(2),eth(src=00:00:00:00:00:01,dst=ff:ff:ff:ff:ff:ff),eth_type(0x0800)"
> +flow2="in_port(2),eth(src=00:00:00:00:00:02,dst=ff:ff:ff:ff:ff:ff),eth_type(0x0800)"
> +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "$flow1" -generate], [0], [stdout])
> +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "$flow2" -generate], [0], [stdout])
> +
> +dnl Delete flows with learn action
> +AT_CHECK([ovs-ofctl del-flows br0 "in_port=2"])
> +
> +AT_CHECK([ovs-ofctl del-tlv-map br0 "{class=0xffff,type=1,len=4}->tun_metadata1"], [1], [], [dnl
> +OFPT_ERROR (xid=0x4): NXTTMFC_INVALID_TLV_DEL
> +NXT_TLV_TABLE_MOD (xid=0x4):
> + DEL mapping table:
> + class type    length  match field
> + ----- ----    ------  -----------
> + 0xffff        0x1     4       tun_metadata1
> +])
> +AT_CHECK([ovs-ofctl del-flows br0 "tun_metadata1"])
> +AT_CHECK([ovs-ofctl del-tlv-map br0 "{class=0xffff,type=1,len=4}->tun_metadata1"])
> +
> +AT_CHECK([ovs-ofctl del-tlv-map br0 "{class=0xffff,type=2,len=4}->tun_metadata2"], [1], [], [dnl
> +OFPT_ERROR (xid=0x4): NXTTMFC_INVALID_TLV_DEL
> +NXT_TLV_TABLE_MOD (xid=0x4):
> + DEL mapping table:
> + class type    length  match field
> + ----- ----    ------  -----------
> + 0xffff        0x2     4       tun_metadata2
> +])
> +AT_CHECK([ovs-ofctl del-flows br0 "reg1=0xFF"])
> +AT_CHECK([ovs-ofctl del-tlv-map br0], [0])
>
>  OVS_VSWITCHD_STOP
>  AT_CLEANUP
> diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c
> index 7b214ebbaacd..6f684d28c9d1 100644
> --- a/utilities/ovs-ofctl.c
> +++ b/utilities/ovs-ofctl.c
> @@ -3876,7 +3876,7 @@ ofctl_parse_actions__(const char *version_s, bool instructions)
>          error = (instructions
>                   ? ofpacts_pull_openflow_instructions
>                   : ofpacts_pull_openflow_actions)(
> -                     &of_in, of_in.size, version, NULL, &ofpacts);
> +                     &of_in, of_in.size, version, NULL, NULL, &ofpacts);
>          if (!error && instructions) {
>              /* Verify actions, enforce consistency. */
>              enum ofputil_protocol protocol;
> --
> 2.7.4
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Thanks for review Joe! Please find my response below.  Patch v2 will be in
the following e-mail.

On Wed, Mar 8, 2017 at 11:40 AM, Joe Stringer <joe@ovn.org> wrote:

> On 7 March 2017 at 09:39, Yi-Hung Wei <yihung.wei@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@ovn.org>
> > Suggested-by: Joe Stringer <joe@ovn.org>
> > Signed-off-by: Yi-Hung Wei <yihung.wei@gmail.com>
> > ---
>
> Thanks for the patch, feedback below.
>
> >  build-aux/extract-ofp-actions     |   9 +-
> >  include/openvswitch/ofp-actions.h |   3 +-
> >  include/openvswitch/ofp-errors.h  |   4 +
> >  include/openvswitch/ofp-util.h    |   1 +
> >  lib/learn.c                       |   6 ++
> >  lib/meta-flow.c                   | 196 ++++++++++++++++++++++++++++++
> +-------
> >  lib/ofp-actions.c                 | 156 ++++++++++++++++++------------
> >  lib/ofp-util.c                    |  21 ++--
> >  lib/vl-mff-map.h                  |  10 +-
> >  ofproto/ofproto-provider.h        |   4 +
> >  ofproto/ofproto.c                 |  39 ++++++--
> >  ovn/controller/pinctrl.c          |   6 +-
> >  tests/tunnel.at                   |  76 ++++++++++++++-
> >  utilities/ovs-ofctl.c             |   2 +-
> >  14 files changed, 404 insertions(+), 129 deletions(-)
> >
> > diff --git a/build-aux/extract-ofp-actions
> b/build-aux/extract-ofp-actions
> > index 184447b99422..0062ab881dd5 100755
> > --- a/build-aux/extract-ofp-actions
> > +++ b/build-aux/extract-ofp-actions
> > @@ -322,7 +322,8 @@ def extract_ofp_actions(fn, definitions):
> >  static enum ofperr
> >  ofpact_decode(const struct ofp_action_header *a, enum
> ofp_raw_action_type raw,
> >                enum ofp_version version, uint64_t arg,
> > -              const struct vl_mff_map *vl_mff_map, struct ofpbuf *out)
> > +              const struct vl_mff_map *vl_mff_map,
> > +              uint64_t *tlv_bitmap, struct ofpbuf *out)
> >  {
> >      switch (raw) {\
> >  """
> > @@ -343,7 +344,7 @@ ofpact_decode(const struct ofp_action_header *a,
> enum ofp_raw_action_type raw,
> >                      else:
> >                          arg = "arg"
> >                  if arg_vl_mff_map:
> > -                    print "        return decode_%s(%s, version,
> vl_mff_map, out);" % (enum, arg)
> > +                    print "        return decode_%s(%s, version,
> vl_mff_map, tlv_bitmap, out);" % (enum, arg)
> >                  else:
> >                      print "        return decode_%s(%s, version, out);"
> % (enum, arg)
> >              print
> > @@ -365,7 +366,7 @@ ofpact_decode(const struct ofp_action_header *a,
> enum ofp_raw_action_type raw,
> >                  else:
> >                      prototype += "%s, enum ofp_version, " % base_argtype
> >                  if arg_vl_mff_map:
> > -                    prototype += 'const struct vl_mff_map *, '
> > +                    prototype += 'const struct vl_mff_map *, uint64_t
> *, '
> >              prototype += "struct ofpbuf *);"
> >              print prototype
> >
> > @@ -374,7 +375,7 @@ static enum ofperr ofpact_decode(const struct
> ofp_action_header *,
> >                                   enum ofp_raw_action_type raw,
> >                                   enum ofp_version version,
> >                                   uint64_t arg, const struct vl_mff_map
> *vl_mff_map,
> > -                                 struct ofpbuf *out);
> > +                                 uint64_t *tlv_bitmap, struct ofpbuf
> *out);
> >  """
> >
> >  if __name__ == '__main__':
> > diff --git a/include/openvswitch/ofp-actions.h
> b/include/openvswitch/ofp-actions.h
> > index 88f573dcd74e..808715e9b1a4 100644
> > --- a/include/openvswitch/ofp-actions.h
> > +++ b/include/openvswitch/ofp-actions.h
> > @@ -946,13 +946,14 @@ enum ofperr ofpacts_pull_openflow_actions(struct
> ofpbuf *openflow,
> >                                            unsigned int actions_len,
> >                                            enum ofp_version version,
> >                                            const struct vl_mff_map *,
> > +                                          uint64_t *,
>
> Please name this variable, this is the public library. Particularly
> since the field is a generic type (uint64_t), there's no way to get
> the context of what it's used for in the header here. Also, please
> update the comment for this function, specifying whether it is a
> required field and what the effect of specifying this is.
>
Thanks, I update that in v2.


>
> >                                            struct ofpbuf *ofpacts);
> >  enum ofperr
> >  ofpacts_pull_openflow_instructions(struct ofpbuf *openflow,
> >                                     unsigned int instructions_len,
> >                                     enum ofp_version version,
> >                                     const struct vl_mff_map *vl_mff_map,
> > -                                   struct ofpbuf *ofpacts);
> > +                                   uint64_t *, struct ofpbuf *ofpacts);
>
> Same here.
>
> >  enum ofperr ofpacts_check(struct ofpact[], size_t ofpacts_len,
> >                            struct flow *, ofp_port_t max_ports,
> >                            uint8_t table_id, uint8_t n_tables,
> > diff --git a/include/openvswitch/ofp-errors.h
> b/include/openvswitch/ofp-errors.h
> > index 81825817e843..a5bba8619bcb 100644
> > --- a/include/openvswitch/ofp-errors.h
> > +++ b/include/openvswitch/ofp-errors.h
> > @@ -772,6 +772,10 @@ enum ofperr {
> >       * to be mapped is the same as one assigned to a different field. */
> >      OFPERR_NXTTMFC_DUP_ENTRY,
> >
> > +    /* NX1.0-1.1(1,537), NX1.2+(38).  Attempted to delete a TLV mapping
> that
> > +     * is used by any active flow. */
> > +    OFPERR_NXTTMFC_INVALID_TLV_DEL,
> > +
> >  /* ## ---------- ## */
> >  /* ## NXT_RESUME ## */
> >  /* ## ---------- ## */
> > diff --git a/include/openvswitch/ofp-util.h
> b/include/openvswitch/ofp-util.h
> > index e73a942a3e15..7cb9e7fd32bd 100644
> > --- a/include/openvswitch/ofp-util.h
> > +++ b/include/openvswitch/ofp-util.h
> > @@ -326,6 +326,7 @@ struct ofputil_flow_mod {
> >      uint16_t importance;     /* Eviction precedence. */
> >      struct ofpact *ofpacts;  /* Series of "struct ofpact"s. */
> >      size_t ofpacts_len;      /* Length of ofpacts, in bytes. */
> > +    uint64_t ofpacts_tlv_bitmap; /* 1-bit for each present TLV in
> 'ofpacts'. */
> >  };
> >
> >  enum ofperr ofputil_decode_flow_mod(struct ofputil_flow_mod *,
> > diff --git a/lib/learn.c b/lib/learn.c
> > index ce52c35f297a..0e86f1629d74 100644
> > --- a/lib/learn.c
> > +++ b/lib/learn.c
> > @@ -29,6 +29,7 @@
> >  #include "openvswitch/ofp-errors.h"
> >  #include "openvswitch/ofp-util.h"
> >  #include "openvswitch/ofpbuf.h"
> > +#include "vl-mff-map.h"
> >  #include "unaligned.h"
> >
> >
> > @@ -107,6 +108,8 @@ learn_execute(const struct ofpact_learn *learn,
> const struct flow *flow,
> >      fm->importance = 0;
> >      fm->buffer_id = UINT32_MAX;
> >      fm->out_port = OFPP_NONE;
> > +    fm->match.flow.tunnel.metadata.present.map = 0;
>
> This line should already be covered by the call to match_init_catchall()
> above.
>
Yes, it is now removed in v2.


>
> > +    fm->ofpacts_tlv_bitmap = 0;
> >      fm->flags = 0;
> >      if (learn->flags & NX_LEARN_F_SEND_FLOW_REM) {
> >          fm->flags |= OFPUTIL_FF_SEND_FLOW_REM;
> > @@ -136,6 +139,8 @@ learn_execute(const struct ofpact_learn *learn,
> const struct flow *flow,
> >          switch (spec->dst_type) {
> >          case NX_LEARN_DST_MATCH:
> >              mf_write_subfield(&spec->dst, &value, &fm->match);
> > +            mf_vl_mff_set_tlv_bitmap(
> > +                spec->dst.field, &fm->match.flow.tunnel.metadat
> a.present.map);
> >              break;
> >
> >          case NX_LEARN_DST_LOAD:
> > @@ -145,6 +150,7 @@ learn_execute(const struct ofpact_learn *learn,
> const struct flow *flow,
> >                           spec->n_bits);
> >              bitwise_one(ofpact_set_field_mask(sf),
> spec->dst.field->n_bytes,
> >                          spec->dst.ofs, spec->n_bits);
> > +            mf_vl_mff_set_tlv_bitmap(spec->dst.field,
> &fm->ofpacts_tlv_bitmap);
> >              break;
> >
> >          case NX_LEARN_DST_OUTPUT:
>
> I think we should also set the tlv bitmap in this LEARN_DST_OUTPUT
> case? IIUC this uses an OXM field to determine the output port. I
> believe it's possible for someone to use a TLV to specify an output
> action using LEARN - so it would set the bitmap in the same way that
> the NX_LEARN_DST_LOAD case does.
>
Yes, for output action in learn, we can specify a TLV to be the SRC field,
but the DST field will eventually be an immediate value that we derive from
the SRC field. So the ref count for a TLV in LEARN_OUTPUT is incresed when
we decode the learn flow indecode_NXAST_RAW_LEARN(), but not when we create
the "learned" flow in learn_execute().


>
> > diff --git a/lib/meta-flow.c b/lib/meta-flow.c
> > index 40704e628aaa..7807b3055f5b 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"
> > +#include "ovs-atomic.h"
> >  #include "ovs-rcu.h"
> >  #include "ovs-thread.h"
> >  #include "packets.h"
> > @@ -2646,6 +2648,7 @@ field_array_set(enum mf_field_id id, const union
> mf_value *value,
> >   * struct vl_mff_map.*/
> >  struct vl_mf_field {
> >      struct mf_field mf;
> > +    struct ovs_refcount ref_cnt;
> >      struct cmap_node cmap_node; /* In ofproto->vl_mff_map->cmap. */
> >  };
> >
> > @@ -2655,17 +2658,25 @@ mf_field_hash(uint32_t key)
> >      return hash_int(key, 0);
> >  }
> >
> > -void
> > -mf_vl_mff_map_clear(struct vl_mff_map *vl_mff_map)
> > +enum ofperr
> > +mf_vl_mff_map_clear(struct vl_mff_map *vl_mff_map, bool clear)
> >      OVS_REQUIRES(vl_mff_map->mutex)
> >  {
> >      struct vl_mf_field *vmf;
> >
> >      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);
> > +        if (clear) {
> > +            cmap_remove(&vl_mff_map->cmap, &vmf->cmap_node,
> > +                        mf_field_hash(vmf->mf.id));
> > +            ovsrcu_postpone(free, vmf);
> > +        } else {
> > +            if (ovs_refcount_read(&vmf->ref_cnt) != 1) {
> > +                return OFPERR_NXTTMFC_INVALID_TLV_DEL;
> > +            }
> > +        }
> >      }
> > +
> > +    return 0;
> >  }
>
> I found this a bit confusing. mf_vl_mff_map_clear() has a boolean,
> also named 'clear' that determines whether it will actually clear or
> not. If you call a function like "clear(clear=false)", what would it
> intuitively do?
>
> I see two calling conventions - first, for TLV clear command from
> controller:
> error = mf_vl_mff_map_clear(vl_mff_map, false);
> if (error)
>     return error;
> error = mf_vl_mff_map_clear(vl_mff_map, true);
>
> Second, from ofproto cleanup code:
> mf_vl_mff_map_clear(vl_mff_map, true);
>
> I suggest that this would be easier to understand if the function were
> structured with the the bool to be 'force':
>
> enum ofperr
> mf_vl_mff_map_clear(struct vl_mff_map *vl_mff_map, bool force)
>    OVS_REQUIRES(vl_mff_map->mutex)
> {
>    struct vl_mf_field *vmf;
>
>     /* First, check whether any flows still refer to the current TLV
> map. */
>    if (!force) {
>        CMAP_FOR_EACH (vmf, cmap_node, &vl_mff_map->cmap) {
>            if (ovs_refcount_read(&vmf->ref_cnt) != 1) {
>                return OFPERR_NXTTMFC_INVALID_TLV_DEL;
>            }
>        }
>    }
>
>    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);
>    }
>
>    return 0;
> }
>
Thanks for suggestion, it is much clear after modification.


>
> You may also consider refactoring the ovsrcu_postpone(free, vmf) into
> a separate, vmf_delete() function which looks something like this:
>
> static void vmf_delete(struct vl_mf_field *vmf)
> {
>    if (ovs_refcount_unref(&vmf->ref_cnt) == 1) {
>         /* Postpone as this function is typically called immediately
> after removing from cmap. */
>        ovsrcu_postpone(free, vmf);
>    } else {
>        VLOG_WARN_RL(&rl, "Attempted to delete VMF %s but refcount is
> nonzero!",
>                  vmf->mf.name);
>    }
> }
>
> This could be reused from the other delete locations as well.
>
I do not factor out the ovsrcu_postpone(), becuase the only case that we
force to clear the vl_mff_map is when we destory ofproto. Otherwise, we are
not going to remove vmf from vl_mff_map if ref_count > 1.


>
> >  static struct vl_mf_field *
> > @@ -2697,53 +2708,100 @@ 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 del)
>
> Similarly, I find it confusing to reason about a function that can be
> called like "delete(delete=false)".
>
Thanks, I update it in v2 as well.


>
> >      OVS_REQUIRES(vl_mff_map->mutex)
> >  {
> >      struct ofputil_tlv_map *tlv_map;
> > -
> > -    if (ttm->command == NXTTMC_CLEAR) {
> > -        mf_vl_mff_map_clear(vl_mff_map);
> > -        return 0;
> > -    }
> > +    struct vl_mf_field *vmf;
> > +    unsigned int idx;
> >
> >      LIST_FOR_EACH (tlv_map, list_node, &ttm->mappings) {
> > -        unsigned int idx = MFF_TUN_METADATA0 + tlv_map->index;
> > -        struct vl_mf_field *vmf;
> > -
> > +        idx = MFF_TUN_METADATA0 + tlv_map->index;
> >          if (idx >= MFF_TUN_METADATA0 + TUN_METADATA_NUM_OPTS) {
> >              return OFPERR_NXTTMFC_BAD_FIELD_IDX;
> >          }
> >
> > -        switch (ttm->command) {
> > -        case NXTTMC_ADD:
> > -            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:
> > -            vmf = mf_get_vl_mff__(idx, vl_mff_map);
> > -            if (vmf) {
> > +        vmf = mf_get_vl_mff__(idx, vl_mff_map);
> > +        if (vmf) {
> > +            if (del == true) {
> >                  cmap_remove(&vl_mff_map->cmap, &vmf->cmap_node,
> >                              mf_field_hash(idx));
> >                  ovsrcu_postpone(free, vmf);
> > +            } else {
> > +                if (ovs_refcount_read(&vmf->ref_cnt) != 1) {
> > +                    return OFPERR_NXTTMFC_INVALID_TLV_DEL;
> > +                }
> >              }
> > -            break;
> > +        }
> > +    }
> > +
> > +    return 0;
> > +}
> > +
> > +static enum ofperr
> > +mf_vl_mff_map_add(struct vl_mff_map *vl_mff_map,
> > +                  const struct ofputil_tlv_table_mod *ttm)
> > +    OVS_REQUIRES(vl_mff_map->mutex)
> > +{
> > +    struct ofputil_tlv_map *tlv_map;
> > +    struct vl_mf_field *vmf;
> > +    unsigned int idx;
> > +
> > +    LIST_FOR_EACH (tlv_map, list_node, &ttm->mappings) {
> > +        idx = MFF_TUN_METADATA0 + tlv_map->index;
> > +        if (idx >= MFF_TUN_METADATA0 + TUN_METADATA_NUM_OPTS) {
> > +            return OFPERR_NXTTMFC_BAD_FIELD_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;
> > +        ovs_refcount_init(&vmf->ref_cnt);
> > +
> > +        cmap_insert(&vl_mff_map->cmap, &vmf->cmap_node,
> > +                    mf_field_hash(idx));
> > +    }
> > +
> > +    return 0;
> > +}
> >
> > -        case NXTTMC_CLEAR:
> > -        default:
> > -            OVS_NOT_REACHED();
> > +/* 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().
>
> I realise the above sentence is from the existing code, but it's
> clearer if this comment is prescriptive, ie:
>
> "This function must be invoked after tun_metadata_table_mod()."
>
Yes.


>
> > + * Returns OFPERR_NXTTMFC_BAD_FIELD_IDX, if the index for the
> vl_mf_field is
> > + * invalid.
> > + * Returns OFPERR_NXTTMFC_INVALID_TLV_DEL, if 'ttm' tries to delete an
> > + * vl_mf_field that is still used by any active flow.*/
> > +enum ofperr
> > +mf_vl_mff_map_mod_from_tun_metadata(struct vl_mff_map *vl_mff_map,
> > +                                    const struct ofputil_tlv_table_mod
> *ttm)
> > +    OVS_REQUIRES(vl_mff_map->mutex)
> > +{
> > +    enum ofperr error;
> > +
> > +    switch (ttm->command) {
> > +    case NXTTMC_ADD:
> > +        return mf_vl_mff_map_add(vl_mff_map, ttm);
> > +
> > +    case NXTTMC_DELETE:
> > +        error = mf_vl_mff_map_del(vl_mff_map, ttm, false);
> > +        if (error) {
> > +            return error;
> >          }
> > +        return mf_vl_mff_map_del(vl_mff_map, ttm, true);
> > +
> > +    case NXTTMC_CLEAR:
> > +        error = mf_vl_mff_map_clear(vl_mff_map, false);
> > +        if (error) {
> > +            return error;
> > +        }
> > +        return mf_vl_mff_map_clear(vl_mff_map, true);
> > +
> > +    default:
> > +        OVS_NOT_REACHED();
> >      }
> >
> >      return 0;
> > @@ -2756,3 +2814,67 @@ mf_vl_mff_invalid(const struct mf_field *mff,
> const struct vl_mff_map *map)
> >  {
> >      return map && mff && mff->variable_len && !mff->mapped;
> >  }
> > +
> > +void
> > +mf_vl_mff_set_tlv_bitmap(const struct mf_field *mff, uint64_t
> *tlv_bitmap)
> > +{
> > +    if (mff && mff->mapped) {
> > +        ULLONG_SET1(*tlv_bitmap, mff->id - MFF_TUN_METADATA0);
>
> I wonder if we should add some runtime check here to ovs_assert() that
> we're not trying to set a bit beyond the length of the field. I think
> that if the second parameter of the ULLONG_SET1() is >64 then some
> tests will fail, but it's not as obvious as an assert right here where
> you're attempting to set the bit.
>
Yes, I add ovs_assert() to make sure it is within the range.


>
> > +    }
> > +}
> > +
> > +/* If the 'mff' is a valid variable length mf_field, udpates the
> 'tlv_bitmap'.
> > + * Returns OFPERR_NXFMFC_INVALID_TLV_FIELD if the variable length
> mf_field
> > + * is invalid. */
> > +enum ofperr
> > +mf_check_vl_mff(const struct mf_field *mff, const struct vl_mff_map
> *map,
> > +                uint64_t *tlv_bitmap)
>
> Could we rename this to mf_set_vl_mff_bitmap()?
>
Sure.


>
> > +{
> > +    if (mf_vl_mff_invalid(mff, map)) {
> > +        return OFPERR_NXFMFC_INVALID_TLV_FIELD;
> > +    }
> > +    mf_vl_mff_set_tlv_bitmap(mff, tlv_bitmap);
> > +
> > +    return 0;
> > +}
> > +
> > +static void
> > +mf_vl_mff_ref_cnt_mod(const struct vl_mff_map *map, uint64_t tlv_bitmap,
> > +                      bool ref)
> > +{
> > +    struct vl_mf_field *vmf;
> > +    int i;
> > +
> > +    if (map) {
> > +        ULLONG_FOR_EACH_1 (i, tlv_bitmap) {
> > +            vmf = mf_get_vl_mff__(i + MFF_TUN_METADATA0, map);
> > +            if (vmf) {
> > +                if (ref) {
> > +                    ovs_refcount_ref(&vmf->ref_cnt);
> > +                } else {
> > +                    ovs_refcount_unref(&vmf->ref_cnt);
>
> We should warn if we're trying to unref and this is the final ref,
> because then we're not freeing the object.
>
It could be the case that no active flow is currently use this tlv, but the
controller is going to use this tlv entry for some new flows. In this case,
it is not necessary to free vmf since it will be useful.

>
> > +                }
> > +            } else {
> > +                VLOG_WARN("Invalid TLV index %d.", i);
> > +            }
> > +        }
> > +    }
> > +}
> > +
> > +void
> > +mf_vl_mff_add_ref_cnt_from_rule(const struct rule *rule)
> > +{
> > +    mf_vl_mff_ref_cnt_mod(&rule->ofproto->vl_mff_map,
> rule->match_tlv_bitmap,
> > +                          true);
> > +    mf_vl_mff_ref_cnt_mod(&rule->ofproto->vl_mff_map,
> rule->ofpacts_tlv_bitmap,
> > +                          true);
> > +}
> > +
> > +void
> > +mf_vl_mff_remove_ref_cnt_from_rule(const struct rule *rule)
> > +{
> > +    mf_vl_mff_ref_cnt_mod(&rule->ofproto->vl_mff_map,
> rule->match_tlv_bitmap,
> > +                          false);
> > +    mf_vl_mff_ref_cnt_mod(&rule->ofproto->vl_mff_map,
> rule->ofpacts_tlv_bitmap,
> > +                          false);
> > +}
>
> These should probably just provide the vl_mff_map and bitmap rather
> than introducing a dependency on ofproto/ofproto-provider.h and struct
> rule.
>
> Usually functions in ovs use the format foo_ref() and foo_unref() to
> add/remove refcounts. Could you rename these to keep with that style?
>
Sure, both concerns are addressed in v2.


>
> > diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
> > index ce80f57e8df3..584e5cd4e847 100644
> > --- a/lib/ofp-actions.c
> > +++ b/lib/ofp-actions.c
> > @@ -405,7 +405,7 @@ static enum ofperr ofpacts_pull_openflow_actions__(
> >      struct ofpbuf *openflow, unsigned int actions_len,
> >      enum ofp_version version, uint32_t allowed_ovsinsts,
> >      struct ofpbuf *ofpacts, enum ofpact_type outer_action,
> > -    const struct vl_mff_map *vl_mff_map);
> > +    const struct vl_mff_map *vl_mff_map, uint64_t *ofpacts_tlv_bitmap);
> >  static char * OVS_WARN_UNUSED_RESULT ofpacts_parse_copy(
> >      const char *s_, struct ofpbuf *ofpacts,
> >      enum ofputil_protocol *usable_protocols,
> > @@ -1117,13 +1117,20 @@ struct nx_action_output_reg2 {
> >  };
> >  OFP_ASSERT(sizeof(struct nx_action_output_reg2) == 24);
> >
> > +#define CHECK_VL_MFF(FIELD, VL_MFF_MAP, BITMAP, ERR)    \
> > +ERR = mf_check_vl_mff(FIELD, VL_MFF_MAP, BITMAP);       \
> > +if (ERR) {                                              \
> > +    return ERR;                                         \
> > +}
> > +
>
> I realise there's a few spots in OVS code that use a style like this,
> and it's (a little) easier to write.. but for reading, it's easy to
> miss the fact that this macro may cause functions to return. For
> instance, we used to do some MPLS checking like this and we ended up
> removing it to improve readability. I think we should just expand this
> out in the 5 or 6 places that this is used in this file.
>
Yes, I will get rid of this in v2.


> >  static enum ofperr
> >  decode_NXAST_RAW_OUTPUT_REG(const struct nx_action_output_reg *naor,
> >                              enum ofp_version ofp_version OVS_UNUSED,
> >                              const struct vl_mff_map *vl_mff_map,
> > -                            struct ofpbuf *out)
> > +                            uint64_t *tlv_bitmap, struct ofpbuf *out)
> >  {
> >      struct ofpact_output_reg *output_reg;
> > +    enum ofperr error;
> >
> >      if (!is_all_zeros(naor->zero, sizeof naor->zero)) {
> >          return OFPERR_OFPBAC_BAD_ARGUMENT;
> > @@ -1136,9 +1143,7 @@ decode_NXAST_RAW_OUTPUT_REG(const struct
> nx_action_output_reg *naor,
> >      output_reg->src.n_bits = nxm_decode_n_bits(naor->ofs_nbits);
> >      output_reg->max_len = ntohs(naor->max_len);
> >
> > -    if (mf_vl_mff_invalid(output_reg->src.field, vl_mff_map)) {
> > -        return OFPERR_NXFMFC_INVALID_TLV_FIELD;
> > -    }
> > +    CHECK_VL_MFF(output_reg->src.field, vl_mff_map, tlv_bitmap, error);
> >
> >      return mf_check_src(&output_reg->src, NULL);
> >  }
> > @@ -1147,7 +1152,7 @@ static enum ofperr
> >  decode_NXAST_RAW_OUTPUT_REG2(const struct nx_action_output_reg2 *naor,
> >                               enum ofp_version ofp_version OVS_UNUSED,
> >                               const struct vl_mff_map *vl_mff_map,
> > -                             struct ofpbuf *out)
> > +                             uint64_t *tlv_bitmap, struct ofpbuf *out)
> >  {
> >      struct ofpact_output_reg *output_reg;
> >      output_reg = ofpact_put_OUTPUT_REG(out);
> > @@ -1164,6 +1169,8 @@ decode_NXAST_RAW_OUTPUT_REG2(const struct
> nx_action_output_reg2 *naor,
> >      if (error) {
> >          return error;
> >      }
> > +    mf_vl_mff_set_tlv_bitmap(output_reg->src.field, tlv_bitmap);
> > +
>
> I wonder if it's cleaner to fold this into nx_pull_header().
>
It's not so obvious why the ofpacts decode functions in this file
> inconsistently need to call the mf_check_vl_mff() and/or
> mf_vl_mff_set_tlv_bitmap(), which makes it a little more difficult to
> verify (and easier to mess up if new actions are added). It'd be nice
> to make this more consistent.
>

Thanks, it is hard to follow as in v1. Actually, we use nx_pull_header(),
nx_pull_entry(), and mf_from_nxm_header() to get a mf_field in ofp-action.c
and those three functions are heavily use in other part of the code base
that may not need to update the bitmap. So instead to fold
mf_vl_mff_set_tlv_bitmap() directly into the three functions, three wrapper
functions mf_vl_mff_nx_pull_header(), mf_vl_mff_nx_pull_entry(), and
mf_vl_mff_mf_from_nxm_header() are created that handle mf_field operation
and set the tlv_bitmap, and it should be more easier to follow now.


> >      if (!is_all_zeros(b.data, b.size)) {
> >          return OFPERR_NXBRC_MUST_BE_ZERO;
> >      }
> > @@ -1286,7 +1293,8 @@ OFP_ASSERT(sizeof(struct nx_action_bundle) == 32);
> >
> >  static enum ofperr
> >  decode_bundle(bool load, const struct nx_action_bundle *nab,
> > -              const struct vl_mff_map *vl_mff_map, struct ofpbuf
> *ofpacts)
> > +              const struct vl_mff_map *vl_mff_map, uint64_t *tlv_bitmap,
> > +              struct ofpbuf *ofpacts)
> >  {
> >      static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
> >      struct ofpact_bundle *bundle;
> > @@ -1326,9 +1334,7 @@ decode_bundle(bool load, const struct
> nx_action_bundle *nab,
> >          bundle->dst.field = mf_from_nxm_header(ntohl(nab->dst),
> vl_mff_map);
> >          bundle->dst.ofs = nxm_decode_ofs(nab->ofs_nbits);
> >          bundle->dst.n_bits = nxm_decode_n_bits(nab->ofs_nbits);
> > -        if (mf_vl_mff_invalid(bundle->dst.field, vl_mff_map)) {
> > -            return OFPERR_NXFMFC_INVALID_TLV_FIELD;
> > -        }
> > +        CHECK_VL_MFF(bundle->dst.field, vl_mff_map, tlv_bitmap, error);
> >
> >          if (bundle->dst.n_bits < 16) {
> >              VLOG_WARN_RL(&rl, "bundle_load action requires at least 16
> bit "
> > @@ -1369,16 +1375,16 @@ decode_NXAST_RAW_BUNDLE(const struct
> nx_action_bundle *nab,
> >                          enum ofp_version ofp_version OVS_UNUSED,
> >                          struct ofpbuf *out)
> >  {
> > -    return decode_bundle(false, nab, NULL, out);
> > +    return decode_bundle(false, nab, NULL, NULL, out);
> >  }
> >
> >  static enum ofperr
> >  decode_NXAST_RAW_BUNDLE_LOAD(const struct nx_action_bundle *nab,
> >                               enum ofp_version ofp_version OVS_UNUSED,
> >                               const struct vl_mff_map *vl_mff_map,
> > -                             struct ofpbuf *out)
> > +                             uint64_t *tlv_bitmap, struct ofpbuf *out)
> >  {
> > -    return decode_bundle(true, nab, vl_mff_map, out);
> > +    return decode_bundle(true, nab, vl_mff_map, tlv_bitmap, out);
> >  }
> >
> >  static void
> > @@ -2282,7 +2288,7 @@ static enum ofperr
> >  decode_copy_field__(ovs_be16 src_offset, ovs_be16 dst_offset, ovs_be16
> n_bits,
> >                      const void *action, ovs_be16 action_len, size_t
> oxm_offset,
> >                      const struct vl_mff_map *vl_mff_map,
> > -                    struct ofpbuf *ofpacts)
> > +                    uint64_t *tlv_bitmap, struct ofpbuf *ofpacts)
> >  {
> >      struct ofpact_reg_move *move = ofpact_put_REG_MOVE(ofpacts);
> >      move->ofpact.raw = ONFACT_RAW13_COPY_FIELD;
> > @@ -2298,10 +2304,13 @@ decode_copy_field__(ovs_be16 src_offset,
> ovs_be16 dst_offset, ovs_be16 n_bits,
> >      if (error) {
> >          return error;
> >      }
> > +    mf_vl_mff_set_tlv_bitmap(move->src.field, tlv_bitmap);
> > +
> >      error = nx_pull_header(&b, vl_mff_map, &move->dst.field, NULL);
> >      if (error) {
> >          return error;
> >      }
> > +    mf_vl_mff_set_tlv_bitmap(move->dst.field, tlv_bitmap);
> >
> >      if (!is_all_zeros(b.data, b.size)) {
> >          return OFPERR_NXBRC_MUST_BE_ZERO;
> > @@ -2314,31 +2323,31 @@ static enum ofperr
> >  decode_OFPAT_RAW15_COPY_FIELD(const struct ofp15_action_copy_field
> *oacf,
> >                                enum ofp_version ofp_version OVS_UNUSED,
> >                                const struct vl_mff_map *vl_mff_map,
> > -                              struct ofpbuf *ofpacts)
> > +                              uint64_t *tlv_bitmap, struct ofpbuf
> *ofpacts)
> >  {
> >      return decode_copy_field__(oacf->src_offset, oacf->dst_offset,
> >                                 oacf->n_bits, oacf, oacf->len,
> >                                 OBJECT_OFFSETOF(oacf, pad2), vl_mff_map,
> > -                               ofpacts);
> > +                               tlv_bitmap, ofpacts);
> >  }
> >
> >  static enum ofperr
> >  decode_ONFACT_RAW13_COPY_FIELD(const struct onf_action_copy_field
> *oacf,
> >                                 enum ofp_version ofp_version OVS_UNUSED,
> >                                 const struct vl_mff_map *vl_mff_map,
> > -                               struct ofpbuf *ofpacts)
> > +                               uint64_t *tlv_bitmap, struct ofpbuf
> *ofpacts)
> >  {
> >      return decode_copy_field__(oacf->src_offset, oacf->dst_offset,
> >                                 oacf->n_bits, oacf, oacf->len,
> >                                 OBJECT_OFFSETOF(oacf, pad3), vl_mff_map,
> > -                               ofpacts);
> > +                               tlv_bitmap, ofpacts);
> >  }
> >
> >  static enum ofperr
> >  decode_NXAST_RAW_REG_MOVE(const struct nx_action_reg_move *narm,
> >                            enum ofp_version ofp_version OVS_UNUSED,
> >                            const struct vl_mff_map *vl_mff_map,
> > -                          struct ofpbuf *ofpacts)
> > +                          uint64_t *tlv_bitmap, struct ofpbuf *ofpacts)
> >  {
> >      struct ofpact_reg_move *move = ofpact_put_REG_MOVE(ofpacts);
> >      move->ofpact.raw = NXAST_RAW_REG_MOVE;
> > @@ -2354,10 +2363,13 @@ decode_NXAST_RAW_REG_MOVE(const struct
> nx_action_reg_move *narm,
> >      if (error) {
> >          return error;
> >      }
> > +    mf_vl_mff_set_tlv_bitmap(move->src.field, tlv_bitmap);
> > +
> >      error = nx_pull_header(&b, vl_mff_map, &move->dst.field, NULL);
> >      if (error) {
> >          return error;
> >      }
> > +    mf_vl_mff_set_tlv_bitmap(move->dst.field, tlv_bitmap);
> >
> >      if (!is_all_zeros(b.data, b.size)) {
> >          return OFPERR_NXBRC_MUST_BE_ZERO;
> > @@ -2481,7 +2493,7 @@ OFP_ASSERT(sizeof(struct nx_action_reg_load) ==
> 24);
> >  static enum ofperr
> >  decode_ofpat_set_field(const struct ofp12_action_set_field *oasf,
> >                         bool may_mask, const struct vl_mff_map
> *vl_mff_map,
> > -                       struct ofpbuf *ofpacts)
> > +                       uint64_t *tlv_bitmap, struct ofpbuf *ofpacts)
> >  {
> >      struct ofpbuf b = ofpbuf_const_initializer(oasf, ntohs(oasf->len));
> >      ofpbuf_pull(&b, OBJECT_OFFSETOF(oasf, pad));
> > @@ -2495,6 +2507,8 @@ decode_ofpat_set_field(const struct
> ofp12_action_set_field *oasf,
> >                  ? OFPERR_OFPBAC_BAD_SET_MASK
> >                  : error);
> >      }
> > +    mf_vl_mff_set_tlv_bitmap(field, tlv_bitmap);
> > +
> >      if (!may_mask) {
> >          memset(&mask, 0xff, field->n_bytes);
> >      }
> > @@ -2540,25 +2554,26 @@ static enum ofperr
> >  decode_OFPAT_RAW12_SET_FIELD(const struct ofp12_action_set_field *oasf,
> >                               enum ofp_version ofp_version OVS_UNUSED,
> >                               const struct vl_mff_map *vl_mff_map,
> > -                             struct ofpbuf *ofpacts)
> > +                             uint64_t *tlv_bitmap, struct ofpbuf
> *ofpacts)
> >  {
> > -    return decode_ofpat_set_field(oasf, false, vl_mff_map, ofpacts);
> > +    return decode_ofpat_set_field(oasf, false, vl_mff_map, tlv_bitmap,
> > +                                  ofpacts);
> >  }
> >
> >  static enum ofperr
> >  decode_OFPAT_RAW15_SET_FIELD(const struct ofp12_action_set_field *oasf,
> >                               enum ofp_version ofp_version OVS_UNUSED,
> >                               const struct vl_mff_map *vl_mff_map,
> > -                             struct ofpbuf *ofpacts)
> > +                             uint64_t *tlv_bitmap, struct ofpbuf
> *ofpacts)
> >  {
> > -    return decode_ofpat_set_field(oasf, true, vl_mff_map, ofpacts);
> > +    return decode_ofpat_set_field(oasf, true, vl_mff_map, tlv_bitmap,
> ofpacts);
> >  }
> >
> >  static enum ofperr
> >  decode_NXAST_RAW_REG_LOAD(const struct nx_action_reg_load *narl,
> >                            enum ofp_version ofp_version OVS_UNUSED,
> >                            const struct vl_mff_map *vl_mff_map,
> > -                          struct ofpbuf *out)
> > +                          uint64_t *tlv_bitmap, struct ofpbuf *out)
> >  {
> >      struct mf_subfield dst;
> >      enum ofperr error;
> > @@ -2566,9 +2581,7 @@ decode_NXAST_RAW_REG_LOAD(const struct
> nx_action_reg_load *narl,
> >      dst.field = mf_from_nxm_header(ntohl(narl->dst), vl_mff_map);
> >      dst.ofs = nxm_decode_ofs(narl->ofs_nbits);
> >      dst.n_bits = nxm_decode_n_bits(narl->ofs_nbits);
> > -    if (mf_vl_mff_invalid(dst.field, vl_mff_map)) {
> > -        return OFPERR_NXFMFC_INVALID_TLV_FIELD;
> > -    }
> > +    CHECK_VL_MFF(dst.field, vl_mff_map, tlv_bitmap, error);
> >
> >      error = mf_check_dst(&dst, NULL);
> >      if (error) {
> > @@ -2596,7 +2609,7 @@ static enum ofperr
> >  decode_NXAST_RAW_REG_LOAD2(const struct ext_action_header *eah,
> >                             enum ofp_version ofp_version OVS_UNUSED,
> >                             const struct vl_mff_map *vl_mff_map,
> > -                           struct ofpbuf *out)
> > +                           uint64_t *tlv_bitmap, struct ofpbuf *out)
> >  {
> >      struct ofpbuf b = ofpbuf_const_initializer(eah, ntohs(eah->len));
> >      ofpbuf_pull(&b, OBJECT_OFFSETOF(eah, pad));
> > @@ -2607,6 +2620,8 @@ decode_NXAST_RAW_REG_LOAD2(const struct
> ext_action_header *eah,
> >      if (error) {
> >          return error;
> >      }
> > +    mf_vl_mff_set_tlv_bitmap(field, tlv_bitmap);
> > +
> >      if (!is_all_zeros(b.data, b.size)) {
> >          return OFPERR_OFPBAC_BAD_SET_ARGUMENT;
> >      }
> > @@ -3109,7 +3124,7 @@ OFP_ASSERT(sizeof(struct nx_action_stack) == 24);
> >
> >  static enum ofperr
> >  decode_stack_action(const struct nx_action_stack *nasp,
> > -                    const struct vl_mff_map *vl_mff_map,
> > +                    const struct vl_mff_map *vl_mff_map, uint64_t
> *tlv_bitmap,
> >                      struct ofpact_stack *stack_action)
> >  {
> >      stack_action->subfield.ofs = ntohs(nasp->offset);
> > @@ -3121,6 +3136,8 @@ decode_stack_action(const struct nx_action_stack
> *nasp,
> >      if (error) {
> >          return error;
> >      }
> > +    mf_vl_mff_set_tlv_bitmap(stack_action->subfield.field, tlv_bitmap);
> > +
> >      stack_action->subfield.n_bits = ntohs(*(const ovs_be16 *) b.data);
> >      ofpbuf_pull(&b, 2);
> >      if (!is_all_zeros(b.data, b.size)) {
> > @@ -3134,10 +3151,11 @@ static enum ofperr
> >  decode_NXAST_RAW_STACK_PUSH(const struct nx_action_stack *nasp,
> >                              enum ofp_version ofp_version OVS_UNUSED,
> >                              const struct vl_mff_map *vl_mff_map,
> > -                            struct ofpbuf *ofpacts)
> > +                            uint64_t *tlv_bitmap, struct ofpbuf
> *ofpacts)
> >  {
> >      struct ofpact_stack *push = ofpact_put_STACK_PUSH(ofpacts);
> > -    enum ofperr error = decode_stack_action(nasp, vl_mff_map, push);
> > +    enum ofperr error = decode_stack_action(nasp, vl_mff_map,
> tlv_bitmap,
> > +                                            push);
> >      return error ? error : nxm_stack_push_check(push, NULL);
> >  }
> >
> > @@ -3145,10 +3163,11 @@ static enum ofperr
> >  decode_NXAST_RAW_STACK_POP(const struct nx_action_stack *nasp,
> >                             enum ofp_version ofp_version OVS_UNUSED,
> >                             const struct vl_mff_map *vl_mff_map,
> > -                           struct ofpbuf *ofpacts)
> > +                           uint64_t *tlv_bitmap, struct ofpbuf *ofpacts)
> >  {
> >      struct ofpact_stack *pop = ofpact_put_STACK_POP(ofpacts);
> > -    enum ofperr error = decode_stack_action(nasp, vl_mff_map, pop);
> > +    enum ofperr error = decode_stack_action(nasp, vl_mff_map,
> tlv_bitmap,
> > +                                            pop);
> >      return error ? error : nxm_stack_pop_check(pop, NULL);
> >  }
> >
> > @@ -4279,14 +4298,14 @@ get_be32(const void **pp)
> >
> >  static enum ofperr
> >  get_subfield(int n_bits, const void **p, struct mf_subfield *sf,
> > -             const struct vl_mff_map *vl_mff_map)
> > +             const struct vl_mff_map *vl_mff_map, uint64_t *tlv_bitmap)
> >  {
> > +    enum ofperr error;
> > +
> >      sf->field = mf_from_nxm_header(ntohl(get_be32(p)), vl_mff_map);
> >      sf->ofs = ntohs(get_be16(p));
> >      sf->n_bits = n_bits;
> > -    if (mf_vl_mff_invalid(sf->field, vl_mff_map)) {
> > -        return OFPERR_NXFMFC_INVALID_TLV_FIELD;
> > -    }
> > +    CHECK_VL_MFF(sf->field, vl_mff_map, tlv_bitmap, error);
> >      return 0;
> >  }
> >
> > @@ -4319,7 +4338,7 @@ static enum ofperr
> >  decode_NXAST_RAW_LEARN(const struct nx_action_learn *nal,
> >                         enum ofp_version ofp_version OVS_UNUSED,
> >                         const struct vl_mff_map *vl_mff_map,
> > -                       struct ofpbuf *ofpacts)
> > +                       uint64_t *tlv_bitmap, struct ofpbuf *ofpacts)
> >  {
> >      struct ofpact_learn *learn;
> >      const void *p, *end;
> > @@ -4384,7 +4403,8 @@ decode_NXAST_RAW_LEARN(const struct
> nx_action_learn *nal,
> >          unsigned int imm_bytes = 0;
> >          enum ofperr error;
> >          if (spec->src_type == NX_LEARN_SRC_FIELD) {
> > -            error = get_subfield(spec->n_bits, &p, &spec->src,
> vl_mff_map);
> > +            error = get_subfield(spec->n_bits, &p, &spec->src,
> vl_mff_map,
> > +                                 tlv_bitmap);
> >              if (error) {
> >                  return error;
> >              }
> > @@ -4399,7 +4419,8 @@ decode_NXAST_RAW_LEARN(const struct
> nx_action_learn *nal,
> >          /* Get the destination. */
> >          if (spec->dst_type == NX_LEARN_DST_MATCH ||
> >              spec->dst_type == NX_LEARN_DST_LOAD) {
> > -            error = get_subfield(spec->n_bits, &p, &spec->dst,
> vl_mff_map);
> > +            error = get_subfield(spec->n_bits, &p, &spec->dst,
> vl_mff_map,
> > +                                 tlv_bitmap);
> >              if (error) {
> >                  return error;
> >              }
> > @@ -4649,11 +4670,12 @@ static enum ofperr
> >  decode_NXAST_RAW_MULTIPATH(const struct nx_action_multipath *nam,
> >                             enum ofp_version ofp_version OVS_UNUSED,
> >                             const struct vl_mff_map *vl_mff_map,
> > -                           struct ofpbuf *out)
> > +                           uint64_t *tlv_bitmap, struct ofpbuf *out)
> >  {
> >      uint32_t n_links = ntohs(nam->max_link) + 1;
> >      size_t min_n_bits = log_2_ceil(n_links);
> >      struct ofpact_multipath *mp;
> > +    enum ofperr error;
> >
> >      mp = ofpact_put_MULTIPATH(out);
> >      mp->fields = ntohs(nam->fields);
> > @@ -4665,9 +4687,7 @@ decode_NXAST_RAW_MULTIPATH(const struct
> nx_action_multipath *nam,
> >      mp->dst.ofs = nxm_decode_ofs(nam->ofs_nbits);
> >      mp->dst.n_bits = nxm_decode_n_bits(nam->ofs_nbits);
> >
> > -    if (mf_vl_mff_invalid(mp->dst.field, vl_mff_map)) {
> > -        return OFPERR_NXFMFC_INVALID_TLV_FIELD;
> > -    }
> > +    CHECK_VL_MFF(mp->dst.field, vl_mff_map, tlv_bitmap, error);
> >
> >      if (!flow_hash_fields_valid(mp->fields)) {
> >          VLOG_WARN_RL(&rl, "unsupported fields %d", (int) mp->fields);
> > @@ -4855,7 +4875,7 @@ static enum ofperr
> >  decode_NXAST_RAW_CLONE(const struct ext_action_header *eah,
> >                         enum ofp_version ofp_version,
> >                         const struct vl_mff_map *vl_mff_map,
> > -                       struct ofpbuf *out)
> > +                       uint64_t *tlv_bitmap, struct ofpbuf *out)
> >  {
> >      int error;
> >      struct ofpbuf openflow;
> > @@ -4869,7 +4889,7 @@ decode_NXAST_RAW_CLONE(const struct
> ext_action_header *eah,
> >      error = ofpacts_pull_openflow_actions__(&openflow, openflow.size,
> >                                              ofp_version,
> >                                              1u <<
> OVSINST_OFPIT11_APPLY_ACTIONS,
> > -                                            out, 0, vl_mff_map);
> > +                                            out, 0, vl_mff_map,
> tlv_bitmap);
> >      clone = ofpbuf_push_uninit(out, sizeof *clone);
> >      out->header = &clone->ofpact;
> >      ofpact_finish_CLONE(out, &clone);
> > @@ -5316,7 +5336,7 @@ OFP_ASSERT(sizeof(struct nx_action_conntrack) ==
> 24);
> >  static enum ofperr
> >  decode_ct_zone(const struct nx_action_conntrack *nac,
> >                 struct ofpact_conntrack *out,
> > -               const struct vl_mff_map *vl_mff_map)
> > +               const struct vl_mff_map *vl_mff_map, uint64_t
> *tlv_bitmap)
> >  {
> >      if (nac->zone_src) {
> >          enum ofperr error;
> > @@ -5325,9 +5345,7 @@ decode_ct_zone(const struct nx_action_conntrack
> *nac,
> >                                                   vl_mff_map);
> >          out->zone_src.ofs = nxm_decode_ofs(nac->zone_ofs_nbits);
> >          out->zone_src.n_bits = nxm_decode_n_bits(nac->zone_ofs_nbits);
> > -        if (mf_vl_mff_invalid(out->zone_src.field, vl_mff_map)) {
> > -            return OFPERR_NXFMFC_INVALID_TLV_FIELD;
> > -        }
> > +        CHECK_VL_MFF(out->zone_src.field, vl_mff_map, tlv_bitmap,
> error);
> >
> >          error = mf_check_src(&out->zone_src, NULL);
> >          if (error) {
> > @@ -5350,13 +5368,14 @@ decode_ct_zone(const struct nx_action_conntrack
> *nac,
> >  static enum ofperr
> >  decode_NXAST_RAW_CT(const struct nx_action_conntrack *nac,
> >                      enum ofp_version ofp_version,
> > -                    const struct vl_mff_map *vl_mff_map, struct ofpbuf
> *out)
> > +                    const struct vl_mff_map *vl_mff_map, uint64_t
> *tlv_bitmap,
> > +                    struct ofpbuf *out)
> >  {
> >      const size_t ct_offset = ofpacts_pull(out);
> >      struct ofpact_conntrack *conntrack = ofpact_put_CT(out);
> >      conntrack->flags = ntohs(nac->flags);
> >
> > -    int error = decode_ct_zone(nac, conntrack, vl_mff_map);
> > +    int error = decode_ct_zone(nac, conntrack, vl_mff_map, tlv_bitmap);
> >      if (error) {
> >          goto out;
> >      }
> > @@ -5370,7 +5389,8 @@ decode_NXAST_RAW_CT(const struct
> nx_action_conntrack *nac,
> >      error = ofpacts_pull_openflow_actions__(&openflow, openflow.size,
> >                                              ofp_version,
> >                                              1u <<
> OVSINST_OFPIT11_APPLY_ACTIONS,
> > -                                            out, OFPACT_CT, vl_mff_map);
> > +                                            out, OFPACT_CT, vl_mff_map,
> > +                                            tlv_bitmap);
> >      if (error) {
> >          goto out;
> >      }
> > @@ -6256,7 +6276,8 @@ log_bad_action(const struct ofp_action_header
> *actions, size_t actions_len,
> >  static enum ofperr
> >  ofpacts_decode(const void *actions, size_t actions_len,
> >                 enum ofp_version ofp_version,
> > -               const struct vl_mff_map *vl_mff_map, struct ofpbuf
> *ofpacts)
> > +               const struct vl_mff_map *vl_mff_map,
> > +               uint64_t *ofpacts_tlv_bitmap, struct ofpbuf *ofpacts)
> >  {
> >      struct ofpbuf openflow = ofpbuf_const_initializer(actions,
> actions_len);
> >      while (openflow.size) {
> > @@ -6268,7 +6289,7 @@ ofpacts_decode(const void *actions, size_t
> actions_len,
> >          error = ofpact_pull_raw(&openflow, ofp_version, &raw, &arg);
> >          if (!error) {
> >              error = ofpact_decode(action, raw, ofp_version, arg,
> vl_mff_map,
> > -                                  ofpacts);
> > +                                  ofpacts_tlv_bitmap, ofpacts);
> >          }
> >
> >          if (error) {
> > @@ -6286,7 +6307,8 @@ ofpacts_pull_openflow_actions__(struct ofpbuf
> *openflow,
> >                                  uint32_t allowed_ovsinsts,
> >                                  struct ofpbuf *ofpacts,
> >                                  enum ofpact_type outer_action,
> > -                                const struct vl_mff_map *vl_mff_map)
> > +                                const struct vl_mff_map *vl_mff_map,
> > +                                uint64_t *ofpacts_tlv_bitmap)
> >  {
> >      const struct ofp_action_header *actions;
> >      size_t orig_size = ofpacts->size;
> > @@ -6306,7 +6328,8 @@ ofpacts_pull_openflow_actions__(struct ofpbuf
> *openflow,
> >          return OFPERR_OFPBRC_BAD_LEN;
> >      }
> >
> > -    error = ofpacts_decode(actions, actions_len, version, vl_mff_map,
> ofpacts);
> > +    error = ofpacts_decode(actions, actions_len, version, vl_mff_map,
> > +                           ofpacts_tlv_bitmap, ofpacts);
> >      if (error) {
> >          ofpacts->size = orig_size;
> >          return error;
> > @@ -6342,11 +6365,13 @@ ofpacts_pull_openflow_actions(struct ofpbuf
> *openflow,
> >                                unsigned int actions_len,
> >                                enum ofp_version version,
> >                                const struct vl_mff_map *vl_mff_map,
> > +                              uint64_t *ofpacts_tlv_bitmap,
> >                                struct ofpbuf *ofpacts)
> >  {
> >      return ofpacts_pull_openflow_actions__(openflow, actions_len,
> version,
> >                                             1u <<
> OVSINST_OFPIT11_APPLY_ACTIONS,
> > -                                           ofpacts, 0, vl_mff_map);
> > +                                           ofpacts, 0, vl_mff_map,
> > +                                           ofpacts_tlv_bitmap);
> >  }
> >
> >  /* OpenFlow 1.1 actions. */
> > @@ -6586,13 +6611,15 @@ static enum ofperr
> >  ofpacts_decode_for_action_set(const struct ofp_action_header *in,
> >                                size_t n_in, enum ofp_version version,
> >                                const struct vl_mff_map *vl_mff_map,
> > +                              uint64_t *ofpacts_tlv_bitmap,
> >                                struct ofpbuf *out)
> >  {
> >      enum ofperr error;
> >      struct ofpact *a;
> >      size_t start = out->size;
> >
> > -    error = ofpacts_decode(in, n_in, version, vl_mff_map, out);
> > +    error = ofpacts_decode(in, n_in, version, vl_mff_map,
> ofpacts_tlv_bitmap,
> > +                           out);
> >
> >      if (error) {
> >          return error;
> > @@ -6891,6 +6918,7 @@ ofpacts_pull_openflow_instructions(struct ofpbuf
> *openflow,
> >                                     unsigned int instructions_len,
> >                                     enum ofp_version version,
> >                                     const struct vl_mff_map *vl_mff_map,
> > +                                   uint64_t *ofpacts_tlv_bitmap,
> >                                     struct ofpbuf *ofpacts)
> >  {
> >      const struct ofp11_instruction *instructions;
> > @@ -6902,7 +6930,8 @@ ofpacts_pull_openflow_instructions(struct ofpbuf
> *openflow,
> >          return ofpacts_pull_openflow_actions__(openflow,
> instructions_len,
> >                                                 version,
> >                                                 (1u <<
> N_OVS_INSTRUCTIONS) - 1,
> > -                                               ofpacts, 0, vl_mff_map);
> > +                                               ofpacts, 0, vl_mff_map,
> > +                                               ofpacts_tlv_bitmap);
> >      }
> >
> >      if (instructions_len % OFP11_INSTRUCTION_ALIGN != 0) {
> > @@ -6946,7 +6975,7 @@ ofpacts_pull_openflow_instructions(struct ofpbuf
> *openflow,
> >          get_actions_from_instruction(insts[OVSINST_OFPIT11_APPLY_ACT
> IONS],
> >                                       &actions, &actions_len);
> >          error = ofpacts_decode(actions, actions_len, version,
> vl_mff_map,
> > -                               ofpacts);
> > +                               ofpacts_tlv_bitmap, ofpacts);
> >          if (error) {
> >              goto exit;
> >          }
> > @@ -6966,7 +6995,8 @@ ofpacts_pull_openflow_instructions(struct ofpbuf
> *openflow,
> >          get_actions_from_instruction(insts[OVSINST_OFPIT11_WRITE_ACT
> IONS],
> >                                       &actions, &actions_len);
> >          error = ofpacts_decode_for_action_set(actions, actions_len,
> > -                                              version, vl_mff_map,
> ofpacts);
> > +                                              version, vl_mff_map,
> > +                                              ofpacts_tlv_bitmap,
> ofpacts);
> >          if (error) {
> >              goto exit;
> >          }
> > diff --git a/lib/ofp-util.c b/lib/ofp-util.c
> > index f7da90b276f9..96b4f8d22333 100644
> > --- a/lib/ofp-util.c
> > +++ b/lib/ofp-util.c
> > @@ -1726,8 +1726,11 @@ ofputil_decode_flow_mod(struct ofputil_flow_mod
> *fm,
> >          return OFPERR_OFPFMFC_BAD_COMMAND;
> >      }
> >
> > +    fm->ofpacts_tlv_bitmap = 0;
> >      error = ofpacts_pull_openflow_instructions(&b, b.size, oh->version,
> > -                                               vl_mff_map, ofpacts);
> > +                                               vl_mff_map,
> > +                                               &fm->ofpacts_tlv_bitmap,
> > +                                               ofpacts);
> >      if (error) {
> >          return error;
> >      }
> > @@ -3009,7 +3012,7 @@ ofputil_decode_flow_stats_reply(struct
> ofputil_flow_stats *fs,
> >      }
> >
> >      if (ofpacts_pull_openflow_instructions(msg, instructions_len,
> oh->version,
> > -                                           NULL, ofpacts)) {
> > +                                           NULL, NULL, ofpacts)) {
> >          VLOG_WARN_RL(&bad_ofmsg_rl, "OFPST_FLOW reply bad
> instructions");
> >          return EINVAL;
> >      }
> > @@ -4034,7 +4037,7 @@ parse_actions_property(struct ofpbuf *property,
> enum ofp_version version,
> >      }
> >
> >      return ofpacts_pull_openflow_actions(property, property->size,
> > -                                         version, NULL, ofpacts);
> > +                                         version, NULL, NULL, ofpacts);
> >  }
> >
> >  /* This is like ofputil_decode_packet_in(), except that it decodes the
> > @@ -4189,7 +4192,8 @@ ofputil_decode_packet_out(struct
> ofputil_packet_out *po,
> >          }
> >
> >          error = ofpacts_pull_openflow_actions(&b,
> ntohs(opo->actions_len),
> > -                                              oh->version, NULL,
> ofpacts);
> > +                                              oh->version, NULL, NULL,
> > +                                              ofpacts);
> >          if (error) {
> >              return error;
> >          }
> > @@ -4201,7 +4205,8 @@ ofputil_decode_packet_out(struct
> ofputil_packet_out *po,
> >          po->in_port = u16_to_ofp(ntohs(opo->in_port));
> >
> >          error = ofpacts_pull_openflow_actions(&b,
> ntohs(opo->actions_len),
> > -                                              oh->version, NULL,
> ofpacts);
> > +                                              oh->version, NULL, NULL,
> > +                                              ofpacts);
> >          if (error) {
> >              return error;
> >          }
> > @@ -6743,7 +6748,7 @@ ofputil_decode_flow_update(struct
> ofputil_flow_update *update,
> >
> >          actions_len = length - sizeof *nfuf - ROUND_UP(match_len, 8);
> >          error = ofpacts_pull_openflow_actions(msg, actions_len,
> oh->version,
> > -                                              NULL, ofpacts);
> > +                                              NULL, NULL, ofpacts);
> >          if (error) {
> >              return error;
> >          }
> > @@ -8727,7 +8732,7 @@ ofputil_pull_ofp11_buckets(struct ofpbuf *msg,
> size_t buckets_length,
> >
> >          ofpbuf_init(&ofpacts, 0);
> >          error = ofpacts_pull_openflow_actions(msg, ob_len - sizeof *ob,
> > -                                              version, NULL, &ofpacts);
> > +                                              version, NULL, NULL,
> &ofpacts);
> >          if (error) {
> >              ofpbuf_uninit(&ofpacts);
> >              ofputil_bucket_list_destroy(buckets);
> > @@ -8801,7 +8806,7 @@ ofputil_pull_ofp15_buckets(struct ofpbuf *msg,
> size_t buckets_length,
> >          buckets_length -= ob_len;
> >
> >          err = ofpacts_pull_openflow_actions(msg, actions_len, version,
> > -                                            NULL, &ofpacts);
> > +                                            NULL, NULL, &ofpacts);
> >          if (err) {
> >              goto err;
> >          }
> > diff --git a/lib/vl-mff-map.h b/lib/vl-mff-map.h
> > index 1c29385782ec..a47b8aa8a6f9 100644
> > --- a/lib/vl-mff-map.h
> > +++ b/lib/vl-mff-map.h
> > @@ -20,6 +20,8 @@
> >  #include "cmap.h"
> >  #include "openvswitch/thread.h"
> >
> > +struct rule;
> > +
> >  /* 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. */
> > @@ -29,7 +31,7 @@ struct vl_mff_map {
> >  };
> >
> >  /* Variable length fields. */
> > -void mf_vl_mff_map_clear(struct vl_mff_map *vl_mff_map)
> > +enum ofperr mf_vl_mff_map_clear(struct vl_mff_map *vl_mff_map, bool)
> >      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 *)
> > @@ -37,5 +39,9 @@ enum ofperr mf_vl_mff_map_mod_from_tun_metadata(
> >  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
> *);
> > -
> > +void mf_vl_mff_set_tlv_bitmap(const struct mf_field *, uint64_t *);
> > +enum ofperr mf_check_vl_mff(const struct mf_field *, const struct
> vl_mff_map *,
> > +                            uint64_t *);
> > +void mf_vl_mff_add_ref_cnt_from_rule(const struct rule *);
> > +void mf_vl_mff_remove_ref_cnt_from_rule(const struct rule *);
> >  #endif /* vl-mff-map.h */
> > diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
> > index 7d3e929f21e3..dab74e3a195f 100644
> > --- a/ofproto/ofproto-provider.h
> > +++ b/ofproto/ofproto-provider.h
> > @@ -423,6 +423,10 @@ struct rule {
> >
> >      /* Must hold 'mutex' for both read/write, 'ofproto_mutex' not
> needed. */
> >      long long int modified OVS_GUARDED; /* Time of last modification. */
> > +
> > +    /* 1-bit for each present TLV in flow match / action. */
> > +    uint64_t match_tlv_bitmap;
> > +    uint64_t ofpacts_tlv_bitmap;
> >  };
> >
> >  void ofproto_rule_ref(struct rule *);
> > diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
> > index 699c37cd5c3e..cf4db291e12a 100644
> > --- a/ofproto/ofproto.c
> > +++ b/ofproto/ofproto.c
> > @@ -228,6 +228,8 @@ static enum ofperr ofproto_rule_create(struct
> ofproto *, struct cls_rule *,
> >                                         uint16_t importance,
> >                                         const struct ofpact *ofpacts,
> >                                         size_t ofpacts_len,
> > +                                       uint64_t match_tlv_bitmap,
> > +                                       uint64_t ofpacts_tlv_bitmap,
> >                                         struct rule **new_rule)
> >      OVS_NO_THREAD_SAFETY_ANALYSIS;
> >
> > @@ -1580,12 +1582,6 @@ ofproto_destroy__(struct ofproto *ofproto)
> >      tun_metadata_free(ovsrcu_get_protected(struct tun_table *,
> >                                             &ofproto->metadata_tab));
> >
> > -    ovs_mutex_lock(&ofproto->vl_mff_map.mutex);
> > -    mf_vl_mff_map_clear(&ofproto->vl_mff_map);
> > -    ovs_mutex_unlock(&ofproto->vl_mff_map.mutex);
> > -    cmap_destroy(&ofproto->vl_mff_map.cmap);
> > -    ovs_mutex_destroy(&ofproto->vl_mff_map.mutex);
> > -
> >      free(ofproto->name);
> >      free(ofproto->type);
> >      free(ofproto->mfr_desc);
> > @@ -1603,6 +1599,12 @@ ofproto_destroy__(struct ofproto *ofproto)
> >      }
> >      free(ofproto->tables);
> >
> > +    ovs_mutex_lock(&ofproto->vl_mff_map.mutex);
> > +    mf_vl_mff_map_clear(&ofproto->vl_mff_map, true);
> > +    ovs_mutex_unlock(&ofproto->vl_mff_map.mutex);
> > +    cmap_destroy(&ofproto->vl_mff_map.cmap);
> > +    ovs_mutex_destroy(&ofproto->vl_mff_map.mutex);
> > +
> >      ovs_assert(hindex_is_empty(&ofproto->cookies));
> >      hindex_destroy(&ofproto->cookies);
> >
>
> Is there a reason for the above two hunks to shift this logic later?
> Should this be in a separate patch to explain why, or could it be
> dropped?
>
> Cause I feel more comfortable to remove the vl_mf_field after all the
rules are deleted. In practice, it is invoked when ofproto is destroyed, so
it probabaly does not matter?


> > @@ -2829,6 +2831,7 @@ rule_destroy_cb(struct rule *rule)
> >          ofproto_rule_send_removed(rule);
> >      }
> >      rule->ofproto->ofproto_class->rule_destruct(rule);
> > +    mf_vl_mff_remove_ref_cnt_from_rule(rule);
> >      ofproto_rule_destroy__(rule);
> >  }
> >
> > @@ -4741,7 +4744,9 @@ add_flow_init(struct ofproto *ofproto, struct
> ofproto_flow_mod *ofm,
> >                                      fm->new_cookie, fm->idle_timeout,
> >                                      fm->hard_timeout, fm->flags,
> >                                      fm->importance, fm->ofpacts,
> > -                                    fm->ofpacts_len, &ofm->temp_rule);
> > +                                    fm->ofpacts_len,
> > +                                    fm->match.flow.tunnel.metadata
> .present.map,
> > +                                    fm->ofpacts_tlv_bitmap,
> &ofm->temp_rule);
> >          if (error) {
> >              return error;
> >          }
> > @@ -4856,6 +4861,7 @@ ofproto_rule_create(struct ofproto *ofproto,
> struct cls_rule *cr,
> >                      uint16_t idle_timeout, uint16_t hard_timeout,
> >                      enum ofputil_flow_mod_flags flags, uint16_t
> importance,
> >                      const struct ofpact *ofpacts, size_t ofpacts_len,
> > +                    uint64_t match_tlv_bitmap, uint64_t
> ofpacts_tlv_bitmap,
> >                      struct rule **new_rule)
> >      OVS_NO_THREAD_SAFETY_ANALYSIS
> >  {
> > @@ -4906,6 +4912,9 @@ ofproto_rule_create(struct ofproto *ofproto,
> struct cls_rule *cr,
> >      }
> >
> >      rule->state = RULE_INITIALIZED;
> > +    rule->match_tlv_bitmap = match_tlv_bitmap;
> > +    rule->ofpacts_tlv_bitmap = ofpacts_tlv_bitmap;
> > +    mf_vl_mff_add_ref_cnt_from_rule(rule);
> >
> >      *new_rule = rule;
> >      return 0;
> > @@ -5003,6 +5012,8 @@ ofproto_flow_mod_learn_refresh(struct
> ofproto_flow_mod *ofm)
> >                                      rule->importance,
> >                                      rule->actions->ofpacts,
> >                                      rule->actions->ofpacts_len,
> > +                                    rule->match_tlv_bitmap,
> > +                                    rule->ofpacts_tlv_bitmap,
> >                                      &ofm->temp_rule);
> >          ovs_mutex_unlock(&rule->mutex);
> >          if (!error) {
> > @@ -5263,6 +5274,11 @@ modify_flows_start__(struct ofproto *ofproto,
> struct ofproto_flow_mod *ofm)
> >                  cls_rule_destroy(CONST_CAST(struct cls_rule *,
> &temp->cr));
> >                  cls_rule_clone(CONST_CAST(struct cls_rule *,
> &temp->cr),
> >                                 &old_rule->cr);
> > +                if (temp->match_tlv_bitmap !=
> old_rule->match_tlv_bitmap) {
> > +                    mf_vl_mff_remove_ref_cnt_from_rule(temp);
> > +                    temp->match_tlv_bitmap = old_rule->match_tlv_bitmap;
> > +                    mf_vl_mff_add_ref_cnt_from_rule(temp);
> > +                }
> >                  *CONST_CAST(uint8_t *, &temp->table_id) =
> old_rule->table_id;
> >                  rule_collection_add(new_rules, temp);
> >                  first = false;
> > @@ -5276,6 +5292,8 @@ modify_flows_start__(struct ofproto *ofproto,
> struct ofproto_flow_mod *ofm)
> >                                              temp->importance,
> >                                              temp->actions->ofpacts,
> >                                              temp->actions->ofpacts_len,
> > +                                            old_rule->match_tlv_bitmap,
> > +                                            temp->ofpacts_tlv_bitmap,
> >                                              &new_rule);
> >                  if (!error) {
> >                      rule_collection_add(new_rules, new_rule);
> > @@ -7784,14 +7802,15 @@ handle_tlv_table_mod(struct ofconn *ofconn,
> const struct ofp_header *oh)
> >      old_tab = ovsrcu_get_protected(struct tun_table *,
> &ofproto->metadata_tab);
> >      error = tun_metadata_table_mod(&ttm, old_tab, &new_tab);
> >      if (!error) {
> > -        ovsrcu_set(&ofproto->metadata_tab, new_tab);
> > -        tun_metadata_postpone_free(old_tab);
> > -
> >          ovs_mutex_lock(&ofproto->vl_mff_map.mutex);
> >          error = mf_vl_mff_map_mod_from_tun_met
> adata(&ofproto->vl_mff_map,
> >                                                      &ttm);
> >          ovs_mutex_unlock(&ofproto->vl_mff_map.mutex);
> >      }
> > +    if (!error) {
> > +        ovsrcu_set(&ofproto->metadata_tab, new_tab);
> > +        tun_metadata_postpone_free(old_tab);
> > +    }
>
> Maybe I didn't quite dig deep enough, but what's the implications of
> moving this? Could it fix another bug, should it be a separate patch?
>

I'm not sure if it be a separate patch? What this change is that we can
only modify update the TLV mapping table if all the table modification
operations are valid. May it makes more sense like this

    if (!error) {
        ovs_mutex_lock(&ofproto->vl_mff_map.mutex);
        error = mf_vl_mff_map_mod_from_tun_metadata(&ofproto->vl_mff_map,
                                                    &ttm);
        ovs_mutex_unlock(&ofproto->vl_mff_map.mutex);
        if (!error) {
            ovsrcu_set(&ofproto->metadata_tab, new_tab);
            tun_metadata_postpone_free(old_tab);
        }
    }


> >
> >      ofputil_uninit_tlv_table(&ttm.mappings);
> >      return error;
> > diff --git a/ovn/controller/pinctrl.c b/ovn/controller/pinctrl.c
> > index 0380c8481ecf..890b47fe0d4f 100644
> > --- a/ovn/controller/pinctrl.c
> > +++ b/ovn/controller/pinctrl.c
> > @@ -168,7 +168,8 @@ pinctrl_handle_arp(const struct flow *ip_flow, const
> struct match *md,
> >
> >      reload_metadata(&ofpacts, md);
> >      enum ofperr error = ofpacts_pull_openflow_actions(userdata,
> userdata->size,
> > -                                                      version, NULL,
> &ofpacts);
> > +                                                      version, NULL,
> NULL,
> > +                                                      &ofpacts);
> >      if (error) {
> >          static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
> >          VLOG_WARN_RL(&rl, "failed to parse arp actions (%s)",
> > @@ -1398,7 +1399,8 @@ pinctrl_handle_nd_na(const struct flow *ip_flow,
> const struct match *md,
> >      reload_metadata(&ofpacts, md);
> >
> >      enum ofperr error = ofpacts_pull_openflow_actions(userdata,
> userdata->size,
> > -                                                      version, NULL,
> &ofpacts);
> > +                                                      version, NULL,
> NULL,
> > +                                                      &ofpacts);
> >      if (error) {
> >          static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
> >          VLOG_WARN_RL(&rl, "failed to parse actions for 'na' (%s)",
> > diff --git a/tests/tunnel.at b/tests/tunnel.at
> > index 1ba209dd4ce7..b9e9e21bfb3f 100644
> > --- a/tests/tunnel.at
> > +++ b/tests/tunnel.at
> > @@ -535,7 +535,81 @@ AT_CHECK([tail -2 stdout], [0],
> >  Datapath actions: 2
> >  ])
> >
> > -AT_CHECK([ovs-ofctl del-tlv-map br0])
> > +AT_CHECK([ovs-ofctl dump-flows br0 | ofctl_strip],
> > +[0], [dnl
> > +NXST_FLOW reply:
> > + tun_metadata3=0x1234567890abcdef actions=output:2
> > +])
> > +
> > +dnl A TLV mapping should not be removed if any active flow uses the
> mapping.
> > +AT_CHECK([ovs-ofctl del-tlv-map br0], [1], [], [dnl
> > +OFPT_ERROR (xid=0x4): NXTTMFC_INVALID_TLV_DEL
> > +NXT_TLV_TABLE_MOD (xid=0x4):
> > + CLEAR
> > +])
> > +
> > +AT_CHECK([ovs-ofctl del-flows br0], [0])
> > +AT_CHECK([ovs-ofctl del-tlv-map br0], [0])
> > +
> > +dnl Flow modification
> > +AT_CHECK([ovs-ofctl add-tlv-map br0 "{class=0xffff,type=1,len=4}->
> tun_metadata0"])
> > +AT_CHECK([ovs-ofctl add-tlv-map br0 "{class=0xffff,type=2,len=4}->
> tun_metadata1"])
> > +AT_CHECK([ovs-ofctl add-tlv-map br0 "{class=0xffff,type=3,len=4}->
> tun_metadata2"])
> > +
> > +AT_CHECK([ovs-ofctl add-flow br0 "in_port=1
> actions=multipath(eth_src,50,modulo_n,1,0,tun_metadata0[[0..31]])"])
> > +AT_CHECK([ovs-ofctl add-flow br0 "in_port=2
> actions=push:tun_metadata1[[0..31]],clone(move:tun_metadata2
> [[0..31]]->reg0[[0..31]])"])
> > +
> > +AT_CHECK([ovs-ofctl add-flow br0 "in_port=1 actions=output:4"])
> > +AT_CHECK([ovs-ofctl del-tlv-map br0 "{class=0xffff,type=3,len=4}->
> tun_metadata0"])
> > +
> > +AT_CHECK([ovs-ofctl add-flow br0 "in_port=2
> actions=push:tun_metadata2[[0..31]]"])
> > +AT_CHECK([ovs-ofctl del-tlv-map br0 "{class=0xffff,type=2,len=4}->
> tun_metadata1"])
> > +AT_CHECK([ovs-ofctl del-tlv-map br0 "{class=0xffff,type=3,len=4}->tun_metadata2"],
> [1], [], [dnl
> > +OFPT_ERROR (xid=0x4): NXTTMFC_INVALID_TLV_DEL
> > +NXT_TLV_TABLE_MOD (xid=0x4):
> > + DEL mapping table:
> > + class type    length  match field
> > + ----- ----    ------  -----------
> > + 0xffff        0x3     4       tun_metadata2
> > +])
> > +
> > +AT_CHECK([ovs-ofctl del-flows br0], [0])
> > +AT_CHECK([ovs-ofctl del-tlv-map br0], [0])
> > +
> > +dnl Learn action
> > +AT_CHECK([ovs-ofctl add-tlv-map br0 "{class=0xffff,type=1,len=4}->
> tun_metadata1"])
> > +AT_CHECK([ovs-ofctl add-tlv-map br0 "{class=0xffff,type=2,len=4}->
> tun_metadata2"])
> > +AT_CHECK([ovs-ofctl add-flow br0 "in_port=2, eth_src=00:00:00:00:00:01
> actions=learn(tun_metadata1[[0..31]]=reg1, output:NXM_OF_IN_PORT[[]])"])
> > +AT_CHECK([ovs-ofctl add-flow br0 "in_port=2, eth_src=00:00:00:00:00:02
> actions=learn(reg1[[0..31]]=0xFF, load:reg1[[0..31]]->tun_metada
> ta2[[0..31]])"])
> > +flow1="in_port(2),eth(src=00:00:00:00:00:01,dst=ff:ff:ff:ff
> :ff:ff),eth_type(0x0800)"
> > +flow2="in_port(2),eth(src=00:00:00:00:00:02,dst=ff:ff:ff:ff
> :ff:ff),eth_type(0x0800)"
> > +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "$flow1" -generate], [0],
> [stdout])
> > +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "$flow2" -generate], [0],
> [stdout])
> > +
> > +dnl Delete flows with learn action
> > +AT_CHECK([ovs-ofctl del-flows br0 "in_port=2"])
> > +
> > +AT_CHECK([ovs-ofctl del-tlv-map br0 "{class=0xffff,type=1,len=4}->tun_metadata1"],
> [1], [], [dnl
> > +OFPT_ERROR (xid=0x4): NXTTMFC_INVALID_TLV_DEL
> > +NXT_TLV_TABLE_MOD (xid=0x4):
> > + DEL mapping table:
> > + class type    length  match field
> > + ----- ----    ------  -----------
> > + 0xffff        0x1     4       tun_metadata1
> > +])
> > +AT_CHECK([ovs-ofctl del-flows br0 "tun_metadata1"])
> > +AT_CHECK([ovs-ofctl del-tlv-map br0 "{class=0xffff,type=1,len=4}->
> tun_metadata1"])
> > +
> > +AT_CHECK([ovs-ofctl del-tlv-map br0 "{class=0xffff,type=2,len=4}->tun_metadata2"],
> [1], [], [dnl
> > +OFPT_ERROR (xid=0x4): NXTTMFC_INVALID_TLV_DEL
> > +NXT_TLV_TABLE_MOD (xid=0x4):
> > + DEL mapping table:
> > + class type    length  match field
> > + ----- ----    ------  -----------
> > + 0xffff        0x2     4       tun_metadata2
> > +])
> > +AT_CHECK([ovs-ofctl del-flows br0 "reg1=0xFF"])
> > +AT_CHECK([ovs-ofctl del-tlv-map br0], [0])
> >
> >  OVS_VSWITCHD_STOP
> >  AT_CLEANUP
> > diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c
> > index 7b214ebbaacd..6f684d28c9d1 100644
> > --- a/utilities/ovs-ofctl.c
> > +++ b/utilities/ovs-ofctl.c
> > @@ -3876,7 +3876,7 @@ ofctl_parse_actions__(const char *version_s, bool
> instructions)
> >          error = (instructions
> >                   ? ofpacts_pull_openflow_instructions
> >                   : ofpacts_pull_openflow_actions)(
> > -                     &of_in, of_in.size, version, NULL, &ofpacts);
> > +                     &of_in, of_in.size, version, NULL, NULL, &ofpacts);
> >          if (!error && instructions) {
> >              /* Verify actions, enforce consistency. */
> >              enum ofputil_protocol protocol;
> > --
> > 2.7.4
> >
> > _______________________________________________
> > dev mailing list
> > dev@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>