[ovs-dev] [PATCH 4/5] flow: Only un-wildcard relevant IP headers.

[Ethan Jackson]

Out of curiosity, we don't we simply not emit the wildcards for those
fields of the packet which are irrelevant?  I.E. if an ethernet packet
says to exact match the IP src, simply don't when we translate from a
flow to an odp key?

Acked-by: Ethan Jackson <ethan at nicira.com>

On Thu, Jun 27, 2013 at 6:16 PM, Justin Pettit <jpettit at nicira.com> wrote:
> When determining the fields to un-wildcard, we need to be careful
> about only un-wildcarding fields that are relevant.  Also, we
> didn't properly handle IPv6 addresses.
>
> Signed-off-by: Justin Pettit <jpettit at nicira.com>
> ---
>  lib/bond.c      |    4 ++--
>  lib/bundle.c    |    2 +-
>  lib/flow.c      |   20 ++++++++++++++------
>  lib/flow.h      |    3 ++-
>  lib/multipath.c |    2 +-
>  5 files changed, 20 insertions(+), 11 deletions(-)
>
> diff --git a/lib/bond.c b/lib/bond.c
> index 68ac068..21922a0 100644
> --- a/lib/bond.c
> +++ b/lib/bond.c
> @@ -1375,12 +1375,12 @@ choose_output_slave(const struct bond *bond, const struct flow *flow,
>              return NULL;
>          }
>          if (wc) {
> -            flow_mask_hash_fields(wc, NX_HASH_FIELDS_SYMMETRIC_L4);
> +            flow_mask_hash_fields(flow, wc, NX_HASH_FIELDS_SYMMETRIC_L4);
>          }
>          /* Fall Through. */
>      case BM_SLB:
>          if (wc) {
> -            flow_mask_hash_fields(wc, NX_HASH_FIELDS_ETH_SRC);
> +            flow_mask_hash_fields(flow, wc, NX_HASH_FIELDS_ETH_SRC);
>          }
>          e = lookup_bond_entry(bond, flow, vlan);
>          if (!e->slave || !e->slave->enabled) {
> diff --git a/lib/bundle.c b/lib/bundle.c
> index d1834d0..78a297a 100644
> --- a/lib/bundle.c
> +++ b/lib/bundle.c
> @@ -60,7 +60,7 @@ execute_hrw(const struct ofpact_bundle *bundle,
>      int best, i;
>
>      if (bundle->n_slaves > 1) {
> -        flow_mask_hash_fields(wc, bundle->fields);
> +        flow_mask_hash_fields(flow, wc, bundle->fields);
>      }
>
>      flow_hash = flow_hash_fields(flow, bundle->fields, bundle->basis);
> diff --git a/lib/flow.c b/lib/flow.c
> index a42fea1..1a5084b 100644
> --- a/lib/flow.c
> +++ b/lib/flow.c
> @@ -781,7 +781,8 @@ flow_hash_symmetric_l4(const struct flow *flow, uint32_t basis)
>
>  /* Masks the fields in 'wc' that are used by the flow hash 'fields'. */
>  void
> -flow_mask_hash_fields(struct flow_wildcards *wc, enum nx_hash_fields fields)
> +flow_mask_hash_fields(const struct flow *flow, struct flow_wildcards *wc,
> +                      enum nx_hash_fields fields)
>  {
>      switch (fields) {
>      case NX_HASH_FIELDS_ETH_SRC:
> @@ -791,11 +792,18 @@ flow_mask_hash_fields(struct flow_wildcards *wc, enum nx_hash_fields fields)
>      case NX_HASH_FIELDS_SYMMETRIC_L4:
>          memset(&wc->masks.dl_src, 0xff, sizeof wc->masks.dl_src);
>          memset(&wc->masks.dl_dst, 0xff, sizeof wc->masks.dl_dst);
> -        memset(&wc->masks.nw_proto, 0xff, sizeof wc->masks.nw_proto);
> -        memset(&wc->masks.nw_src, 0xff, sizeof wc->masks.nw_src);
> -        memset(&wc->masks.nw_dst, 0xff, sizeof wc->masks.nw_dst);
> -        memset(&wc->masks.tp_src, 0xff, sizeof wc->masks.tp_src);
> -        memset(&wc->masks.tp_dst, 0xff, sizeof wc->masks.tp_dst);
> +        if (flow->dl_type == htons(ETH_TYPE_IP)) {
> +            memset(&wc->masks.nw_src, 0xff, sizeof wc->masks.nw_src);
> +            memset(&wc->masks.nw_dst, 0xff, sizeof wc->masks.nw_dst);
> +        } else {
> +            memset(&wc->masks.ipv6_src, 0xff, sizeof wc->masks.ipv6_src);
> +            memset(&wc->masks.ipv6_dst, 0xff, sizeof wc->masks.ipv6_dst);
> +        }
> +        if (is_ip_any(flow)) {
> +            memset(&wc->masks.nw_proto, 0xff, sizeof wc->masks.nw_proto);
> +            memset(&wc->masks.tp_src, 0xff, sizeof wc->masks.tp_src);
> +            memset(&wc->masks.tp_dst, 0xff, sizeof wc->masks.tp_dst);
> +        }
>          wc->masks.vlan_tci |= htons(VLAN_VID_MASK | VLAN_CFI);
>          break;
>
> diff --git a/lib/flow.h b/lib/flow.h
> index 9a6e937..7c3654b 100644
> --- a/lib/flow.h
> +++ b/lib/flow.h
> @@ -262,7 +262,8 @@ bool flow_wildcards_equal(const struct flow_wildcards *,
>                            const struct flow_wildcards *);
>  uint32_t flow_hash_symmetric_l4(const struct flow *flow, uint32_t basis);
>
> -void flow_mask_hash_fields(struct flow_wildcards *, enum nx_hash_fields);
> +void flow_mask_hash_fields(const struct flow *, struct flow_wildcards *,
> +                           enum nx_hash_fields);
>  uint32_t flow_hash_fields(const struct flow *, enum nx_hash_fields,
>                            uint16_t basis);
>  const char *flow_hash_fields_to_str(enum nx_hash_fields);
> diff --git a/lib/multipath.c b/lib/multipath.c
> index dbd5704..1be6964 100644
> --- a/lib/multipath.c
> +++ b/lib/multipath.c
> @@ -113,7 +113,7 @@ multipath_execute(const struct ofpact_multipath *mp, struct flow *flow,
>      uint16_t link = multipath_algorithm(hash, mp->algorithm,
>                                          mp->max_link + 1, mp->arg);
>
> -    flow_mask_hash_fields(wc, mp->fields);
> +    flow_mask_hash_fields(flow, wc, mp->fields);
>      nxm_reg_load(&mp->dst, link, flow);
>  }
>
> --
> 1.7.5.4
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
X-CudaMail-Whitelist-To: dev at openvswitch.org