[ovs-dev,v2,1/3] userspace datapath: Add OVS_HASH_L4_SYMMETRIC dp_hash algorithm

Message ID 1523888788-20288-2-git-send-email-jan.scheurich@ericsson.com
State Changes Requested
Headers show
Series
  • Use improved dp_hash select group by default
Related show

Commit Message

Jan Scheurich April 16, 2018, 2:26 p.m.
This commit implements a new dp_hash algorithm OVS_HASH_L4_SYMMETRIC in
the netdev datapath. It will be used as default hash algorithm for the
dp_hash-based select groups in a subsequent commit to maintain
compatibility with the symmetry property of the current default hash
selection method.

Signed-off-by: Jan Scheurich <jan.scheurich@ericsson.com>
Signed-off-by: Nitin Katiyar <nitin.katiyar@ericsson.com>
Co-authored-by: Nitin Katiyar <nitin.katiyar@ericsson.com>
---
 datapath/linux/compat/include/linux/openvswitch.h |  4 +++
 lib/flow.c                                        | 42 +++++++++++++++++++++--
 lib/flow.h                                        |  1 +
 lib/odp-execute.c                                 | 23 +++++++++++--
 4 files changed, 65 insertions(+), 5 deletions(-)

Comments

Ben Pfaff April 17, 2018, 7:56 p.m. | #1
On Mon, Apr 16, 2018 at 04:26:26PM +0200, Jan Scheurich wrote:
> This commit implements a new dp_hash algorithm OVS_HASH_L4_SYMMETRIC in
> the netdev datapath. It will be used as default hash algorithm for the
> dp_hash-based select groups in a subsequent commit to maintain
> compatibility with the symmetry property of the current default hash
> selection method.
> 
> Signed-off-by: Jan Scheurich <jan.scheurich@ericsson.com>
> Signed-off-by: Nitin Katiyar <nitin.katiyar@ericsson.com>
> Co-authored-by: Nitin Katiyar <nitin.katiyar@ericsson.com>

"sparse" says:
    ../lib/flow.c:2128:13: error: restricted ovs_be32 degrades to integer
    ../lib/flow.c:2128:35: error: restricted ovs_be16 degrades to integer

I think that it is saying that htons should be htonl here:

    if (flow->packet_type != htons(PT_ETH)) {
        /* Cannot hash non-Ethernet flows */
        return 0;
    }

GCC says:

    ../lib/flow.c: In function 'flow_hash_symmetric_l2.part.20':
    ../lib/flow.c:2138:25: error: 'fields.<Ue630>.vlan_tci' is used uninitialized in this function [-Werror=uninitialized]
             fields.vlan_tci ^= flow->vlans[i].tci & htons(VLAN_VID_MASK);
                             ^~

I think that it is right.

Thanks,

Ben.

Patch

diff --git a/datapath/linux/compat/include/linux/openvswitch.h b/datapath/linux/compat/include/linux/openvswitch.h
index 84ebcaf..2bb3cb2 100644
--- a/datapath/linux/compat/include/linux/openvswitch.h
+++ b/datapath/linux/compat/include/linux/openvswitch.h
@@ -720,6 +720,10 @@  struct ovs_action_push_vlan {
  */
 enum ovs_hash_alg {
 	OVS_HASH_ALG_L4,
+#ifndef __KERNEL__
+	OVS_HASH_ALG_SYM_L4,
+#endif
+	__OVS_HASH_MAX
 };
 
 /*
diff --git a/lib/flow.c b/lib/flow.c
index 09b66b8..9d8c1ca 100644
--- a/lib/flow.c
+++ b/lib/flow.c
@@ -2108,6 +2108,44 @@  flow_hash_symmetric_l4(const struct flow *flow, uint32_t basis)
     return jhash_bytes(&fields, sizeof fields, basis);
 }
 
+/* Symmetrically Hashes non-IP 'flow' based on its L2 headers. */
+uint32_t
+flow_hash_symmetric_l2(const struct flow *flow, uint32_t basis)
+{
+    union {
+        struct {
+            ovs_be16 eth_type;
+            ovs_be16 vlan_tci;
+            struct eth_addr eth_addr;
+            ovs_be16 pad;
+        };
+        uint32_t word[3];
+    } fields;
+
+    uint32_t hash = basis;
+    int i;
+
+    if (flow->packet_type != htons(PT_ETH)) {
+        /* Cannot hash non-Ethernet flows */
+        return 0;
+    }
+
+    for (i = 0; i < ARRAY_SIZE(fields.eth_addr.be16); i++) {
+        fields.eth_addr.be16[i] =
+                flow->dl_src.be16[i] ^ flow->dl_dst.be16[i];
+    }
+    for (i = 0; i < FLOW_MAX_VLAN_HEADERS; i++) {
+        fields.vlan_tci ^= flow->vlans[i].tci & htons(VLAN_VID_MASK);
+    }
+    fields.eth_type = flow->dl_type;
+    fields.pad = 0;
+
+    hash = hash_add(hash, fields.word[0]);
+    hash = hash_add(hash, fields.word[1]);
+    hash = hash_add(hash, fields.word[2]);
+    return hash_finish(hash, basis);
+}
+
 /* Hashes 'flow' based on its L3 through L4 protocol information */
 uint32_t
 flow_hash_symmetric_l3l4(const struct flow *flow, uint32_t basis,
@@ -2128,8 +2166,8 @@  flow_hash_symmetric_l3l4(const struct flow *flow, uint32_t basis,
             hash = hash_add64(hash, a[i] ^ b[i]);
         }
     } else {
-        /* Cannot hash non-IP flows */
-        return 0;
+        /* Revert to hashing L2 headers */
+        return flow_hash_symmetric_l2(flow, basis);
     }
 
     hash = hash_add(hash, flow->nw_proto);
diff --git a/lib/flow.h b/lib/flow.h
index af82931..900e8f8 100644
--- a/lib/flow.h
+++ b/lib/flow.h
@@ -236,6 +236,7 @@  hash_odp_port(odp_port_t odp_port)
 
 uint32_t flow_hash_5tuple(const struct flow *flow, uint32_t basis);
 uint32_t flow_hash_symmetric_l4(const struct flow *flow, uint32_t basis);
+uint32_t flow_hash_symmetric_l2(const struct flow *flow, uint32_t basis);
 uint32_t flow_hash_symmetric_l3l4(const struct flow *flow, uint32_t basis,
                          bool inc_udp_ports );
 
diff --git a/lib/odp-execute.c b/lib/odp-execute.c
index 1969f02..c716c41 100644
--- a/lib/odp-execute.c
+++ b/lib/odp-execute.c
@@ -726,14 +726,16 @@  odp_execute_actions(void *dp, struct dp_packet_batch *batch, bool steal,
         }
 
         switch ((enum ovs_action_attr) type) {
+
         case OVS_ACTION_ATTR_HASH: {
             const struct ovs_action_hash *hash_act = nl_attr_get(a);
 
-            /* Calculate a hash value directly.  This might not match the
+            /* Calculate a hash value directly. This might not match the
              * value computed by the datapath, but it is much less expensive,
              * and the current use case (bonding) does not require a strict
              * match to work properly. */
-            if (hash_act->hash_alg == OVS_HASH_ALG_L4) {
+            switch (hash_act->hash_alg) {
+            case OVS_HASH_ALG_L4: {
                 struct flow flow;
                 uint32_t hash;
 
@@ -749,7 +751,22 @@  odp_execute_actions(void *dp, struct dp_packet_batch *batch, bool steal,
                     }
                     packet->md.dp_hash = hash;
                 }
-            } else {
+                break;
+            }
+            case OVS_HASH_ALG_SYM_L4: {
+                struct flow flow;
+                uint32_t hash;
+
+                DP_PACKET_BATCH_FOR_EACH (i, packet, batch) {
+                    flow_extract(packet, &flow);
+                    hash = flow_hash_symmetric_l3l4(&flow,
+                                                    hash_act->hash_basis,
+                                                    false);
+                    packet->md.dp_hash = hash;
+                }
+                break;
+            }
+            default:
                 /* Assert on unknown hash algorithm.  */
                 OVS_NOT_REACHED();
             }