Jesse,
Some comments and a possible bug below, otherwise looks good.
Acked-by: Jarno Rajahalme <jrajaha...@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 <je...@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 <je...@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)
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
