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 <et...@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 <et...@nicira.com>
>
> On Thu, Jun 27, 2013 at 6:16 PM, Justin Pettit <jpet...@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 <jpet...@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@openvswitch.org
>> http://openvswitch.org/mailman/listinfo/dev
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
