Awesome,
Acked-by: Jarno Rajahalme <ja...@ovn.org>
> On Dec 9, 2015, at 6:27 PM, Daniele Di Proietto <diproiet...@vmware.com>
> wrote:
>
> In the ODP context an empty mask netlink attribute usually means that
> the flow should be an exact match.
>
> odp_flow_key_to_mask{,_udpif}() instead return a struct flow_wildcards
> with matches only on recirc_id and vlan_tci.
>
> A more appropriate behavior is to handle a missing (zero length) netlink
> mask specially (like we do in userspace and Linux datapath) and create
> an exact match flow_wildcards from the original flow.
>
> This fixes a bug in revalidate_ukey(): every flow created with
> megaflows disabled would be revalidated away, because the mask would
> seem too generic. (Another possible fix would be to handle the special
> case of a missing mask in revalidate_ukey(), but this seems a more
> generic solution).
>
> Signed-off-by: Daniele Di Proietto <diproiet...@vmware.com>
> ---
> lib/dpctl.c | 2 +-
> lib/dpif-netdev.c | 46 ++++++++++++++++++++-----------------------
> lib/odp-util.c | 35 ++++++++++++++++++++++++++------
> lib/odp-util.h | 4 ++--
> ofproto/ofproto-dpif-upcall.c | 2 +-
> tests/test-odp.c | 2 +-
> 6 files changed, 55 insertions(+), 36 deletions(-)
>
> diff --git a/lib/dpctl.c b/lib/dpctl.c
> index 438bfd3..26de23f 100644
> --- a/lib/dpctl.c
> +++ b/lib/dpctl.c
> @@ -797,7 +797,7 @@ dpctl_dump_flows(int argc, const char *argv[], struct
> dpctl_params *dpctl_p)
>
> odp_flow_key_to_flow(f.key, f.key_len, &flow);
> odp_flow_key_to_mask(f.mask, f.mask_len, f.key, f.key_len,
> - &wc.masks, &flow);
> + &wc, &flow);
> match_init(&match, &flow, &wc);
>
> match_init(&match_filter, &flow_filter, &wc);
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index 3bf130d..8794ca0 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -1858,33 +1858,29 @@ dpif_netdev_mask_from_nlattrs(const struct nlattr
> *key, uint32_t key_len,
> uint32_t mask_key_len, const struct flow *flow,
> struct flow_wildcards *wc)
> {
> - if (mask_key_len) {
> - enum odp_key_fitness fitness;
> -
> - fitness = odp_flow_key_to_mask_udpif(mask_key, mask_key_len, key,
> - key_len, &wc->masks, flow);
> - if (fitness) {
> - /* This should not happen: it indicates that
> - * odp_flow_key_from_mask() and odp_flow_key_to_mask()
> - * disagree on the acceptable form of a mask. Log the problem
> - * as an error, with enough details to enable debugging. */
> - static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
> -
> - if (!VLOG_DROP_ERR(&rl)) {
> - struct ds s;
> -
> - ds_init(&s);
> - odp_flow_format(key, key_len, mask_key, mask_key_len, NULL,
> &s,
> - true);
> - VLOG_ERR("internal error parsing flow mask %s (%s)",
> - ds_cstr(&s), odp_key_fitness_to_string(fitness));
> - ds_destroy(&s);
> - }
> + enum odp_key_fitness fitness;
> +
> + fitness = odp_flow_key_to_mask_udpif(mask_key, mask_key_len, key,
> + key_len, wc, flow);
> + if (fitness) {
> + /* This should not happen: it indicates that
> + * odp_flow_key_from_mask() and odp_flow_key_to_mask()
> + * disagree on the acceptable form of a mask. Log the problem
> + * as an error, with enough details to enable debugging. */
> + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
> +
> + if (!VLOG_DROP_ERR(&rl)) {
> + struct ds s;
>
> - return EINVAL;
> + ds_init(&s);
> + odp_flow_format(key, key_len, mask_key, mask_key_len, NULL, &s,
> + true);
> + VLOG_ERR("internal error parsing flow mask %s (%s)",
> + ds_cstr(&s), odp_key_fitness_to_string(fitness));
> + ds_destroy(&s);
> }
> - } else {
> - flow_wildcards_init_for_packet(wc, flow);
> +
> + return EINVAL;
> }
>
> return 0;
> diff --git a/lib/odp-util.c b/lib/odp-util.c
> index 4db371d..3717aee 100644
> --- a/lib/odp-util.c
> +++ b/lib/odp-util.c
> @@ -5152,6 +5152,26 @@ odp_flow_key_to_flow(const struct nlattr *key, size_t
> key_len,
> return odp_flow_key_to_flow__(key, key_len, NULL, 0, flow, flow, false);
> }
>
> +static enum odp_key_fitness
> +odp_flow_key_to_mask__(const struct nlattr *mask_key, size_t mask_key_len,
> + const struct nlattr *flow_key, size_t flow_key_len,
> + struct flow_wildcards *mask,
> + const struct flow *src_flow,
> + bool udpif)
> +{
> + if (mask_key_len) {
> + return odp_flow_key_to_flow__(mask_key, mask_key_len,
> + flow_key, flow_key_len,
> + &mask->masks, src_flow, udpif);
> +
> + } else {
> + /* A missing mask means that the flow should be exact matched.
> + * Generate an appropriate exact wildcard for the flow. */
> + flow_wildcards_init_for_packet(mask, src_flow);
> +
> + return ODP_FIT_PERFECT;
> + }
> +}
> /* Converts the 'mask_key_len' bytes of OVS_KEY_ATTR_* attributes in
> 'mask_key'
> * to a mask structure in 'mask'. 'flow' must be a previously translated flow
> * corresponding to 'mask' and similarly flow_key/flow_key_len must be the
> @@ -5160,10 +5180,11 @@ odp_flow_key_to_flow(const struct nlattr *key, size_t
> key_len,
> enum odp_key_fitness
> odp_flow_key_to_mask(const struct nlattr *mask_key, size_t mask_key_len,
> const struct nlattr *flow_key, size_t flow_key_len,
> - struct flow *mask, const struct flow *flow)
> + struct flow_wildcards *mask, const struct flow *flow)
> {
> - return odp_flow_key_to_flow__(mask_key, mask_key_len, flow_key,
> flow_key_len,
> - mask, flow, false);
> + return odp_flow_key_to_mask__(mask_key, mask_key_len,
> + flow_key, flow_key_len,
> + mask, flow, false);
> }
>
> /* These functions are similar to their non-"_udpif" variants but output a
> @@ -5185,10 +5206,12 @@ odp_flow_key_to_flow_udpif(const struct nlattr *key,
> size_t key_len,
> enum odp_key_fitness
> odp_flow_key_to_mask_udpif(const struct nlattr *mask_key, size_t mask_key_len,
> const struct nlattr *flow_key, size_t flow_key_len,
> - struct flow *mask, const struct flow *flow)
> + struct flow_wildcards *mask,
> + const struct flow *flow)
> {
> - return odp_flow_key_to_flow__(mask_key, mask_key_len, flow_key,
> flow_key_len,
> - mask, flow, true);
> + return odp_flow_key_to_mask__(mask_key, mask_key_len,
> + flow_key, flow_key_len,
> + mask, flow, true);
> }
>
> /* Returns 'fitness' as a string, for use in debug messages. */
> diff --git a/lib/odp-util.h b/lib/odp-util.h
> index 35849ae..51cf5c3 100644
> --- a/lib/odp-util.h
> +++ b/lib/odp-util.h
> @@ -240,7 +240,7 @@ enum odp_key_fitness odp_flow_key_to_mask(const struct
> nlattr *mask_key,
> size_t mask_key_len,
> const struct nlattr *flow_key,
> size_t flow_key_len,
> - struct flow *mask,
> + struct flow_wildcards *mask,
> const struct flow *flow);
>
> enum odp_key_fitness odp_flow_key_to_flow_udpif(const struct nlattr *, size_t,
> @@ -249,7 +249,7 @@ enum odp_key_fitness odp_flow_key_to_mask_udpif(const
> struct nlattr *mask_key,
> size_t mask_key_len,
> const struct nlattr *flow_key,
> size_t flow_key_len,
> - struct flow *mask,
> + struct flow_wildcards *mask,
> const struct flow *flow);
>
> const char *odp_key_fitness_to_string(enum odp_key_fitness);
> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
> index c191927..cd8a9f0 100644
> --- a/ofproto/ofproto-dpif-upcall.c
> +++ b/ofproto/ofproto-dpif-upcall.c
> @@ -1878,7 +1878,7 @@ revalidate_ukey(struct udpif *udpif, struct udpif_key
> *ukey,
> }
>
> if (odp_flow_key_to_mask(ukey->mask, ukey->mask_len, ukey->key,
> - ukey->key_len, &dp_mask.masks, &flow)
> + ukey->key_len, &dp_mask, &flow)
> == ODP_FIT_ERROR) {
> goto exit;
> }
> diff --git a/tests/test-odp.c b/tests/test-odp.c
> index cdc761f..55ee97e 100644
> --- a/tests/test-odp.c
> +++ b/tests/test-odp.c
> @@ -197,7 +197,7 @@ parse_filter(char *filter_parse)
>
> odp_flow_key_to_flow(odp_key.data, odp_key.size, &flow);
> odp_flow_key_to_mask(odp_mask.data, odp_mask.size, odp_key.data,
> - odp_key.size, &wc.masks, &flow);
> + odp_key.size, &wc, &flow);
> match_init(&match, &flow, &wc);
>
> match_init(&match_filter, &flow_filter, &wc);
> --
> 2.1.4
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
