Re: [ovs-dev] [PATCH v2 17/26] ofproto-dpif-xlate: Hash only fields specified for 'hash' selection method.

[Ben Pfaff]

On Thu, Jul 28, 2016 at 05:56:09PM -0700, Jarno Rajahalme wrote:
> 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 

Much nicer, thanks.

Acked-by: Ben Pfaff 
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH v2 17/26] ofproto-dpif-xlate: Hash only fields specified for 'hash' selection method.

[Jarno Rajahalme]

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 
---
 include/openvswitch/meta-flow.h |  2 --
 lib/meta-flow.c | 36 
 ofproto/ofproto-dpif-xlate.c| 36 +++-
 tests/ofproto-dpif.at   |  2 +-
 4 files changed, 8 insertions(+), 68 deletions(-)

diff --git a/include/openvswitch/meta-flow.h b/include/openvswitch/meta-flow.h
index 10e1f77..d8c5dd8 100644
--- 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
index 1701520..c44dd2a 100644
--- 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
index 1fc738d..3efba3a 100644
--- 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, >xin->flow, ctx->wc)) {
 continue;
 }
 
-/* Hash both the field and its pre-requisites */
-mf_bitmap_set_field_and_prereqs(mf, _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, >xin->flow, );
 /* This seems inefficient but so does apply_mask() */
-for (j = 0; j <