[ovs-dev] [PATCH] tunnel: Support matching on the presence of Geneve options.

Jarno Rajahalme jrajahalme at nicira.com
Fri Aug 28 21:50:24 UTC 2015


Jesse,

Some comments and a possible bug below, otherwise looks good.

Acked-by: Jarno Rajahalme <jrajahalme at nicira.com>

I recall Ben has reviewed the earlier patches in this domain, so maybe it would be good to get his Ack as well.

  Jarno

> On Aug 28, 2015, at 9:39 AM, Jesse Gross <jesse at nicira.com> wrote:
> 
> Sometimes it is useful to match only on whether a Geneve option
> is present even if the specific value is unimportant. A special
> case of this is zero length options where there is no value at all
> and the only information conveyed is whether the option was included
> in the packet.
> 
> This operation was partially supported before but it was not consistent -
> in particular, options were never serialized through NXM/OXM unless
> they had a non-zero mask. Furthermore, zero length options were rejected
> altogether when they were installed through the Geneve map OpenFlow
> command.
> 
> This adds support for these types of matches by making any NXM/OXM for
> tunnel metadata force a match on that field. In the case of a zero length
> option, both the value and mask of the NXM are ignored.
> 
> Signed-off-by: Jesse Gross <jesse at nicira.com>
> ---
> lib/bitmap.h                 |  2 ++
> lib/meta-flow.c              | 46 ++++++++++++++++++++--------
> lib/meta-flow.h              |  3 +-
> lib/nx-match.c               | 43 +++++++++++++-------------
> lib/nx-match.h               |  6 ++--
> lib/odp-util.c               |  3 +-
> lib/ofp-parse.c              | 21 ++++++++++---
> lib/ofp-util.c               |  3 +-
> lib/tun-metadata.c           | 72 ++++++++++++++++++++++++++++++--------------
> lib/tun-metadata.h           | 11 ++++++-
> ofproto/ofproto-dpif-xlate.c |  5 +++
> tests/ovs-ofctl.at           |  1 +
> tests/tunnel.at              | 42 +++++++++++++++++++++++++-
> utilities/ovs-ofctl.8.in     |  7 ++++-
> 14 files changed, 193 insertions(+), 72 deletions(-)
> 
> diff --git a/lib/bitmap.h b/lib/bitmap.h
> index cf5d3f0..35f033e 100644
> --- a/lib/bitmap.h
> +++ b/lib/bitmap.h
> @@ -278,4 +278,6 @@ bitmap_is_all_zeros(const unsigned long *bitmap, size_t n)
> #define ULLONG_SET0(MAP, OFFSET) ((MAP) &= ~(1ULL << (OFFSET)))
> #define ULLONG_SET1(MAP, OFFSET) ((MAP) |= 1ULL << (OFFSET))
> 
> +#define ULLONG_GET(MAP, OFFSET) ((MAP) & (1ULL << (OFFSET)))
> +

It seems a bit unclear if this is intended to return the bit as ULLONG or a boolean for the presence of the bit.

> #endif /* bitmap.h */
> diff --git a/lib/meta-flow.c b/lib/meta-flow.c
> index 5a46ce4..971565d 100644
> --- a/lib/meta-flow.c
> +++ b/lib/meta-flow.c
> @@ -202,12 +202,9 @@ mf_is_all_wild(const struct mf_field *mf, const struct flow_wildcards *wc)
>         return !wc->masks.tunnel.gbp_id;
>     case MFF_TUN_GBP_FLAGS:
>         return !wc->masks.tunnel.gbp_flags;
> -    CASE_MFF_TUN_METADATA: {
> -        union mf_value value;
> -
> -        tun_metadata_read(&wc->masks.tunnel, mf, &value);
> -        return is_all_zeros(&value.tun_metadata, mf->n_bytes);
> -    }
> +    CASE_MFF_TUN_METADATA:
> +        return !ULLONG_GET(wc->masks.tunnel.metadata.present.map,
> +                           mf->id - MFF_TUN_METADATA0);
>     case MFF_METADATA:
>         return !wc->masks.metadata;
>     case MFF_IN_PORT:
> @@ -1063,19 +1060,28 @@ field_len(const struct mf_field *mf, const union mf_value *value_)
> /* Returns the effective length of the field. For fixed length fields,
>  * this is just the defined length. For variable length fields, it is
>  * the minimum size encoding that retains the same meaning (i.e.
> - * discarding leading zeros). */
> + * discarding leading zeros).
> + *
> + * 'is_masked' returns (if non-NULL) whether the original contained
> + * a mask. Otherwise, a mask that is the same length as the value
> + * might be misinterpreted as an exact match. */
> int
> mf_field_len(const struct mf_field *mf, const union mf_value *value,
> -             const union mf_value *mask)
> +             const union mf_value *mask, bool *is_masked_)
> {
>     int len, mask_len;
> +    bool is_masked = mask && !is_all_ones(mask, mf->n_bytes);
> 
>     len = field_len(mf, value);
> -    if (mask && !is_all_ones(mask, mf->n_bytes)) {
> +    if (is_masked) {
>         mask_len = field_len(mf, mask);
>         len = MAX(len, mask_len);
>     }
> 
> +    if (is_masked_) {
> +        *is_masked_ = is_masked;
> +    }
> +
>     return len;
> }
> 
> @@ -1329,6 +1335,13 @@ mf_set_flow_value_masked(const struct mf_field *field,
>     mf_set_flow_value(field, &tmp, flow);
> }
> 
> +bool
> +mf_is_tun_metadata(const struct mf_field *mf)
> +{
> +    return mf->id >= MFF_TUN_METADATA0 &&
> +           mf->id < MFF_TUN_METADATA0 + TUN_METADATA_NUM_OPTS;
> +}
> +
> /* Returns true if 'mf' has a zero value in 'flow', false if it is nonzero.
>  *
>  * The caller is responsible for ensuring that 'flow' meets 'mf''s
> @@ -1336,10 +1349,15 @@ mf_set_flow_value_masked(const struct mf_field *field,
> bool
> mf_is_zero(const struct mf_field *mf, const struct flow *flow)
> {
> -    union mf_value value;
> +    if (!mf_is_tun_metadata(mf)) {
> +        union mf_value value;
> 
> -    mf_get_value(mf, flow, &value);
> -    return is_all_zeros(&value, mf->n_bytes);
> +        mf_get_value(mf, flow, &value);
> +        return is_all_zeros(&value, mf->n_bytes);
> +    } else {
> +        return !ULLONG_GET(flow->tunnel.metadata.present.map,
> +                           mf->id - MFF_TUN_METADATA0);

What if the option is present, but the value is zero? For masks that is not allowed, but how about the flow itself?

> +    }
> }
> 
> /* Makes 'match' wildcard field 'mf’.

(snip)

> diff --git a/lib/tun-metadata.c b/lib/tun-metadata.c
> index acaacff..bef4641 100644
> --- a/lib/tun-metadata.c
> +++ b/lib/tun-metadata.c
> @@ -280,7 +280,7 @@ tun_metadata_write(struct flow_tnl *tnl,
> 
> static const struct tun_metadata_loc *
> metadata_loc_from_match(struct tun_table *map, struct match *match,
> -                        unsigned int idx, unsigned int field_len)
> +                        unsigned int idx, unsigned int field_len, bool masked)
> {
>     ovs_assert(idx < TUN_METADATA_NUM_OPTS);
> 
> @@ -293,18 +293,19 @@ metadata_loc_from_match(struct tun_table *map, struct match *match,
>     }
> 
>     if (match->tun_md.alloc_offset + field_len >= TUN_METADATA_TOT_OPT_SIZE ||
> -        match->tun_md.loc[idx].len) {
> +        match->wc.masks.tunnel.metadata.present.map & idx) {

Is ‘idx’ an index or a mask?

>         return NULL;
>     }
> 
> -    match->tun_md.loc[idx].len = field_len;
> -    match->tun_md.loc[idx].c.offset = match->tun_md.alloc_offset;
> -    match->tun_md.loc[idx].c.len = field_len;
> -    match->tun_md.loc[idx].c.next = NULL;
> +    match->tun_md.entry[idx].loc.len = field_len;
> +    match->tun_md.entry[idx].loc.c.offset = match->tun_md.alloc_offset;
> +    match->tun_md.entry[idx].loc.c.len = field_len;
> +    match->tun_md.entry[idx].loc.c.next = NULL;
> +    match->tun_md.entry[idx].masked = masked;
>     match->tun_md.alloc_offset += field_len;
>     match->tun_md.valid = true;
> 
> -    return &match->tun_md.loc[idx];
> +    return &match->tun_md.entry[idx].loc;
> }
> 
> /* Makes 'match' match 'value'/'mask' on field 'mf’.

(snip)

> 




More information about the dev mailing list