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

Justin Pettit jpettit at nicira.com
Fri Jun 28 02:57:09 UTC 2013


That's a good idea as a sanity check--especially since the consequences are bad (the flow is rejected by the kernel).  I'll look at adding that.

--Justin


On Jun 27, 2013, at 6:37 PM, Ethan Jackson <ethan at nicira.com> wrote:

> 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




More information about the dev mailing list