ofproto-dpif-xlate: Hash only fields specified for 'hash' selection m…

…ethod. The mask for non-present fields in struct field_array is always zero, so hashing a prerequisite field that was not also specified for the "hash" selection method boiled down to hashing a all-zeroes value and unwildcarding the prerequisite field. Now that mf_are_prereqs_ok() already takes care of unwildcarding, we can simplify the code by hashing only the specified fields. Also change the test case to include fields that have prerequisities. Signed-off-by: Jarno Rajahalme <[email protected]> Acked-by: Ben Pfaff <[email protected]>
zjtheone · Jul 29, 2016 · 362a2eb · 362a2eb
1 parent aff49b8
commit 362a2eb
Show file tree

Hide file tree

Showing 4 changed files with 8 additions and 68 deletions.
diff --git a/include/openvswitch/meta-flow.h b/include/openvswitch/meta-flow.h
@@ -2062,8 +2062,6 @@ void mf_get_mask(const struct mf_field *, const struct flow_wildcards *,
 /* Prerequisites. */
 bool mf_are_prereqs_ok(const struct mf_field *mf, const struct flow *flow,
                        struct flow_wildcards *wc);
-void mf_bitmap_set_field_and_prereqs(const struct mf_field *mf, struct
-                                     mf_bitmap *bm);
 
 static inline bool
 mf_is_l3_or_higher(const struct mf_field *mf)

diff --git a/lib/meta-flow.c b/lib/meta-flow.c
@@ -408,42 +408,6 @@ mf_are_prereqs_ok(const struct mf_field *mf, const struct flow *flow,
     OVS_NOT_REACHED();
 }
 
-/* Set bits of 'bm' corresponding to the field 'mf' and it's prerequisities. */
-void
-mf_bitmap_set_field_and_prereqs(const struct mf_field *mf, struct mf_bitmap *bm)
-{
-    bitmap_set1(bm->bm, mf->id);
-
-    switch (mf->prereqs) {
-    case MFP_ND:
-    case MFP_ND_SOLICIT:
-    case MFP_ND_ADVERT:
-        bitmap_set1(bm->bm, MFF_TCP_SRC);
-        bitmap_set1(bm->bm, MFF_TCP_DST);
-        /* Fall through. */
-    case MFP_TCP:
-    case MFP_UDP:
-    case MFP_SCTP:
-    case MFP_ICMPV4:
-    case MFP_ICMPV6:
-        /* nw_frag always unwildcarded. */
-        bitmap_set1(bm->bm, MFF_IP_PROTO);
-        /* Fall through. */
-    case MFP_ARP:
-    case MFP_IPV4:
-    case MFP_IPV6:
-    case MFP_MPLS:
-    case MFP_IP_ANY:
-        bitmap_set1(bm->bm, MFF_ETH_TYPE);
-        break;
-    case MFP_VLAN_VID:
-        bitmap_set1(bm->bm, MFF_VLAN_TCI);
-        break;
-    case MFP_NONE:
-        break;
-    }
-}
-
 /* Returns true if 'value' may be a valid value *as part of a masked match*,
  * false otherwise.
  *

diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
@@ -3490,7 +3490,6 @@ xlate_default_select_group(struct xlate_ctx *ctx, struct group_dpif *group)
 static void
 xlate_hash_fields_select_group(struct xlate_ctx *ctx, struct group_dpif *group)
 {
-    struct mf_bitmap hash_fields = MF_BITMAP_INITIALIZER;
     const struct field_array *fields;
     struct ofputil_bucket *bucket;
     uint32_t basis;
@@ -3499,44 +3498,23 @@ xlate_hash_fields_select_group(struct xlate_ctx *ctx, struct group_dpif *group)
     fields = group_dpif_get_fields(group);
     basis = hash_uint64(group_dpif_get_selection_method_param(group));
 
-    /* Determine which fields to hash */
     for (i = 0; i < MFF_N_IDS; i++) {
         if (bitmap_is_set(fields->used.bm, i)) {
-            const struct mf_field *mf;
-
-            /* If the field is already present in 'hash_fields' then
-             * this loop has already checked that it and its pre-requisites
-             * are present in the flow and its pre-requisites have
-             * already been added to 'hash_fields'. There is nothing more
-             * to do here and as an optimisation the loop can continue. */
-            if (bitmap_is_set(hash_fields.bm, i)) {
-                continue;
-            }
-
-            mf = mf_from_id(i);
+            const struct mf_field *mf = mf_from_id(i);
 
-            /* Only hash a field if it and its pre-requisites are present
-             * in the flow. */
+            /* Skip fields for which prerequisities are not met. */
             if (!mf_are_prereqs_ok(mf, &ctx->xin->flow, ctx->wc)) {
                 continue;
             }
 
-            /* Hash both the field and its pre-requisites */
-            mf_bitmap_set_field_and_prereqs(mf, &hash_fields);
-        }
-    }
-
-    /* Hash the fields */
-    for (i = 0; i < MFF_N_IDS; i++) {
-        if (bitmap_is_set(hash_fields.bm, i)) {
-            const struct mf_field *mf = mf_from_id(i);
             union mf_value value;
-            int j;
+            union mf_value mask;
 
             mf_get_value(mf, &ctx->xin->flow, &value);
             /* This seems inefficient but so does apply_mask() */
-            for (j = 0; j < mf->n_bytes; j++) {
-                ((uint8_t *) &value)[j] &= ((uint8_t *) &fields->value[i])[j];
+            for (int j = 0; j < mf->n_bytes; j++) {
+                mask.b[j] = fields->value[i].b[j];
+                value.b[j] &= mask.b[j];
             }
             basis = hash_bytes(&value, mf->n_bytes, basis);
 
@@ -3545,7 +3523,7 @@ xlate_hash_fields_select_group(struct xlate_ctx *ctx, struct group_dpif *group)
                 basis = hash_boolean(mf_is_set(mf, &ctx->xin->flow), basis);
             }
 
-            mf_mask_field(mf, ctx->wc);
+            mf_mask_field_masked(mf, &mask, ctx->wc);
         }
     }
 

diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
@@ -418,7 +418,7 @@ AT_CLEANUP
 AT_SETUP([ofproto-dpif - select group with hash selection method])
 OVS_VSWITCHD_START
 add_of_ports br0 1 10 11
-AT_CHECK([ovs-ofctl -O OpenFlow15 add-group br0 'group_id=1234,type=select,selection_method=hash,fields=eth_dst,bucket=output:10,bucket=output:11'])
+AT_CHECK([ovs-ofctl -O OpenFlow15 add-group br0 'group_id=1234,type=select,selection_method=hash,fields(eth_dst,ip_dst,tcp_dst),bucket=output:10,bucket=output:11'])
 AT_CHECK([ovs-ofctl -O OpenFlow15 add-flow br0 'ip actions=write_actions(group:1234)'])
 
 # Try a bunch of different flows and make sure that they get distributed