diff mbox series

[ovs-dev,1/2] dpif: Turn dpif_flow_hash function into generic odp_flow_key_hash.

Message ID 20191208193346.6762-2-i.maximets@ovn.org
State Accepted
Headers show
Series Get rid of broken dpif pointer in dp_netdev structure. | expand

Commit Message

Ilya Maximets Dec. 8, 2019, 7:33 p.m. UTC
Current implementation of dpif_flow_hash() doesn't depend on datapath
interface and only complicates the callers by forcing them to figure
out what is their current 'dpif'.  If we'll need different hashing
for different 'dpif's we'll implement an API for dpif-providers
and each dpif implementation will be able to use their local function
directly without calling it via dpif API.

This change will allow us to not store 'dpif' pointer in the userspace
datapath implementation which is broken and will be removed in next
commits.

This patch moves dpif_flow_hash() to odp-util module and replaces
unused odp_flow_key_hash() by it, along with removing of unused 'dpif'
argument.

Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
---
 lib/dpif-netdev.c      | 10 +++++-----
 lib/dpif-netlink.c     | 27 ++++++++++++---------------
 lib/dpif.c             | 16 ----------------
 lib/dpif.h             |  4 +---
 lib/odp-util.c         | 17 +++++++++++++----
 lib/odp-util.h         |  2 +-
 ofproto/ofproto-dpif.c |  2 +-
 7 files changed, 33 insertions(+), 45 deletions(-)

Comments

Ben Pfaff Jan. 7, 2020, 7:18 p.m. UTC | #1
On Sun, Dec 08, 2019 at 08:33:45PM +0100, Ilya Maximets wrote:
> Current implementation of dpif_flow_hash() doesn't depend on datapath
> interface and only complicates the callers by forcing them to figure
> out what is their current 'dpif'.  If we'll need different hashing
> for different 'dpif's we'll implement an API for dpif-providers
> and each dpif implementation will be able to use their local function
> directly without calling it via dpif API.
> 
> This change will allow us to not store 'dpif' pointer in the userspace
> datapath implementation which is broken and will be removed in next
> commits.
> 
> This patch moves dpif_flow_hash() to odp-util module and replaces
> unused odp_flow_key_hash() by it, along with removing of unused 'dpif'
> argument.
> 
> Signed-off-by: Ilya Maximets <i.maximets@ovn.org>

Much nicer.

Acked-by: Ben Pfaff <blp@ovn.org>
diff mbox series

Patch

diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index 7ebf4ef87..ffb3cd420 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -2984,7 +2984,7 @@  dp_netdev_pmd_find_flow(const struct dp_netdev_pmd_thread *pmd,
     /* If a UFID is not provided, determine one based on the key. */
     if (!ufidp && key && key_len
         && !dpif_netdev_flow_from_nlattrs(key, key_len, &flow, false)) {
-        dpif_flow_hash(pmd->dp->dpif, &flow, sizeof flow, &ufid);
+        odp_flow_key_hash(&flow, sizeof flow, &ufid);
         ufidp = &ufid;
     }
 
@@ -3203,7 +3203,7 @@  dp_netdev_get_mega_ufid(const struct match *match, ovs_u128 *mega_ufid)
         ((uint8_t *)&masked_flow)[i] = ((uint8_t *)&match->flow)[i] &
                                        ((uint8_t *)&match->wc)[i];
     }
-    dpif_flow_hash(NULL, &masked_flow, sizeof(struct flow), mega_ufid);
+    odp_flow_key_hash(&masked_flow, sizeof masked_flow, mega_ufid);
 }
 
 static struct dp_netdev_flow *
@@ -3407,7 +3407,7 @@  dpif_netdev_flow_put(struct dpif *dpif, const struct dpif_flow_put *put)
     if (put->ufid) {
         ufid = *put->ufid;
     } else {
-        dpif_flow_hash(dpif, &match.flow, sizeof match.flow, &ufid);
+        odp_flow_key_hash(&match.flow, sizeof match.flow, &ufid);
     }
 
     /* The Netlink encoding of datapath flow keys cannot express
@@ -6615,7 +6615,7 @@  handle_packet_upcall(struct dp_netdev_pmd_thread *pmd,
     ofpbuf_clear(actions);
     ofpbuf_clear(put_actions);
 
-    dpif_flow_hash(pmd->dp->dpif, &match.flow, sizeof match.flow, &ufid);
+    odp_flow_key_hash(&match.flow, sizeof match.flow, &ufid);
     error = dp_netdev_upcall(pmd, packet, &match.flow, &match.wc,
                              &ufid, DPIF_UC_MISS, NULL, actions,
                              put_actions);
@@ -7152,7 +7152,7 @@  dp_execute_cb(void *aux_, struct dp_packet_batch *packets_,
             struct dp_packet *packet;
             DP_PACKET_BATCH_FOR_EACH (i, packet, packets_) {
                 flow_extract(packet, &flow);
-                dpif_flow_hash(dp->dpif, &flow, sizeof flow, &ufid);
+                odp_flow_key_hash(&flow, sizeof flow, &ufid);
                 dp_execute_userspace_action(pmd, packet, should_steal, &flow,
                                             &ufid, &actions, userdata);
             }
diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
index e9a6887f7..23e513eff 100644
--- a/lib/dpif-netlink.c
+++ b/lib/dpif-netlink.c
@@ -153,7 +153,7 @@  static int dpif_netlink_flow_transact(struct dpif_netlink_flow *request,
                                       struct ofpbuf **bufp);
 static void dpif_netlink_flow_get_stats(const struct dpif_netlink_flow *,
                                         struct dpif_flow_stats *);
-static void dpif_netlink_flow_to_dpif_flow(struct dpif *, struct dpif_flow *,
+static void dpif_netlink_flow_to_dpif_flow(struct dpif_flow *,
                                            const struct dpif_netlink_flow *);
 
 /* One of the dpif channels between the kernel and userspace. */
@@ -1527,7 +1527,7 @@  dpif_netlink_flow_dump_thread_destroy(struct dpif_flow_dump_thread *thread_)
 }
 
 static void
-dpif_netlink_flow_to_dpif_flow(struct dpif *dpif, struct dpif_flow *dpif_flow,
+dpif_netlink_flow_to_dpif_flow(struct dpif_flow *dpif_flow,
                                const struct dpif_netlink_flow *datapath_flow)
 {
     dpif_flow->key = datapath_flow->key;
@@ -1542,8 +1542,8 @@  dpif_netlink_flow_to_dpif_flow(struct dpif *dpif, struct dpif_flow *dpif_flow,
         dpif_flow->ufid = datapath_flow->ufid;
     } else {
         ovs_assert(datapath_flow->key && datapath_flow->key_len);
-        dpif_flow_hash(dpif, datapath_flow->key, datapath_flow->key_len,
-                       &dpif_flow->ufid);
+        odp_flow_key_hash(datapath_flow->key, datapath_flow->key_len,
+                          &dpif_flow->ufid);
     }
     dpif_netlink_flow_get_stats(datapath_flow, &dpif_flow->stats);
     dpif_flow->attrs.offloaded = false;
@@ -1716,8 +1716,7 @@  dpif_netlink_flow_dump_next(struct dpif_flow_dump_thread *thread_,
         if (dump->up.terse || datapath_flow.actions) {
             /* Common case: we don't want actions, or the flow includes
              * actions. */
-            dpif_netlink_flow_to_dpif_flow(&dpif->dpif, &flows[n_flows++],
-                                           &datapath_flow);
+            dpif_netlink_flow_to_dpif_flow(&flows[n_flows++], &datapath_flow);
         } else {
             /* Rare case: the flow does not include actions.  Retrieve this
              * individual flow again to get the actions. */
@@ -1735,8 +1734,7 @@  dpif_netlink_flow_dump_next(struct dpif_flow_dump_thread *thread_,
 
             /* Save this flow.  Then exit, because we only have one buffer to
              * handle this case. */
-            dpif_netlink_flow_to_dpif_flow(&dpif->dpif, &flows[n_flows++],
-                                           &datapath_flow);
+            dpif_netlink_flow_to_dpif_flow(&flows[n_flows++], &datapath_flow);
             break;
         }
     }
@@ -1929,8 +1927,7 @@  dpif_netlink_operate__(struct dpif_netlink *dpif,
 
                 op->error = dpif_netlink_flow_from_ofpbuf(&reply, txn->reply);
                 if (!op->error) {
-                    dpif_netlink_flow_to_dpif_flow(&dpif->dpif, get->flow,
-                                                   &reply);
+                    dpif_netlink_flow_to_dpif_flow(get->flow, &reply);
                 }
             }
             break;
@@ -2452,8 +2449,8 @@  dpif_netlink_queue_to_priority(const struct dpif *dpif OVS_UNUSED,
 }
 
 static int
-parse_odp_packet(const struct dpif_netlink *dpif, struct ofpbuf *buf,
-                 struct dpif_upcall *upcall, int *dp_ifindex)
+parse_odp_packet(struct ofpbuf *buf, struct dpif_upcall *upcall,
+                 int *dp_ifindex)
 {
     static const struct nl_policy ovs_packet_policy[] = {
         /* Always present. */
@@ -2494,7 +2491,7 @@  parse_odp_packet(const struct dpif_netlink *dpif, struct ofpbuf *buf,
     upcall->key = CONST_CAST(struct nlattr *,
                              nl_attr_get(a[OVS_PACKET_ATTR_KEY]));
     upcall->key_len = nl_attr_get_size(a[OVS_PACKET_ATTR_KEY]);
-    dpif_flow_hash(&dpif->dpif, upcall->key, upcall->key_len, &upcall->ufid);
+    odp_flow_key_hash(upcall->key, upcall->key_len, &upcall->ufid);
     upcall->userdata = a[OVS_PACKET_ATTR_USERDATA];
     upcall->out_tun_key = a[OVS_PACKET_ATTR_EGRESS_TUN_KEY];
     upcall->actions = a[OVS_PACKET_ATTR_ACTIONS];
@@ -2588,7 +2585,7 @@  dpif_netlink_recv_windows(struct dpif_netlink *dpif, uint32_t handler_id,
                 return error;
             }
 
-            error = parse_odp_packet(dpif, buf, upcall, &dp_ifindex);
+            error = parse_odp_packet(buf, upcall, &dp_ifindex);
             if (!error && dp_ifindex == dpif->dp_ifindex) {
                 return 0;
             } else if (error) {
@@ -2663,7 +2660,7 @@  dpif_netlink_recv__(struct dpif_netlink *dpif, uint32_t handler_id,
                 return error;
             }
 
-            error = parse_odp_packet(dpif, buf, upcall, &dp_ifindex);
+            error = parse_odp_packet(buf, upcall, &dp_ifindex);
             if (!error && dp_ifindex == dpif->dp_ifindex) {
                 return 0;
             } else if (error) {
diff --git a/lib/dpif.c b/lib/dpif.c
index c88b2106f..0eeb18686 100644
--- a/lib/dpif.c
+++ b/lib/dpif.c
@@ -904,22 +904,6 @@  dpif_flow_stats_format(const struct dpif_flow_stats *stats, struct ds *s)
     }
 }
 
-/* Places the hash of the 'key_len' bytes starting at 'key' into '*hash'. */
-void
-dpif_flow_hash(const struct dpif *dpif OVS_UNUSED,
-               const void *key, size_t key_len, ovs_u128 *hash)
-{
-    static struct ovsthread_once once = OVSTHREAD_ONCE_INITIALIZER;
-    static uint32_t secret;
-
-    if (ovsthread_once_start(&once)) {
-        secret = random_uint32();
-        ovsthread_once_done(&once);
-    }
-    hash_bytes128(key, key_len, secret, hash);
-    uuid_set_bits_v4((struct uuid *)hash);
-}
-
 /* Deletes all flows from 'dpif'.  Returns 0 if successful, otherwise a
  * positive errno value.  */
 int
diff --git a/lib/dpif.h b/lib/dpif.h
index d96f854a3..ef1b7745d 100644
--- a/lib/dpif.h
+++ b/lib/dpif.h
@@ -532,8 +532,6 @@  enum dpif_flow_put_flags {
 bool dpif_probe_feature(struct dpif *, const char *name,
                         const struct ofpbuf *key, const struct ofpbuf *actions,
                         const ovs_u128 *ufid);
-void dpif_flow_hash(const struct dpif *, const void *key, size_t key_len,
-                    ovs_u128 *hash);
 int dpif_flow_flush(struct dpif *);
 int dpif_flow_put(struct dpif *, enum dpif_flow_put_flags,
                   const struct nlattr *key, size_t key_len,
@@ -840,7 +838,7 @@  void dpif_register_dp_purge_cb(struct dpif *, dp_purge_callback *, void *aux);
 /* A callback to process an upcall, currently implemented only by dpif-netdev.
  *
  * The caller provides the 'packet' and 'flow' to process, the corresponding
- * 'ufid' as generated by dpif_flow_hash(), the polling thread id 'pmd_id',
+ * 'ufid' as generated by odp_flow_key_hash(), the polling thread id 'pmd_id',
  * the 'type' of the upcall, and if 'type' is DPIF_UC_ACTION then the
  * 'userdata' attached to the action.
  *
diff --git a/lib/odp-util.c b/lib/odp-util.c
index c3be2a596..14fa1ca03 100644
--- a/lib/odp-util.c
+++ b/lib/odp-util.c
@@ -6430,11 +6430,20 @@  odp_key_to_dp_packet(const struct nlattr *key, size_t key_len,
     }
 }
 
-uint32_t
-odp_flow_key_hash(const struct nlattr *key, size_t key_len)
+/* Places the hash of the 'key_len' bytes starting at 'key' into '*hash'.
+ * Generated value has format of random UUID. */
+void
+odp_flow_key_hash(const void *key, size_t key_len, ovs_u128 *hash)
 {
-    BUILD_ASSERT_DECL(!(NLA_ALIGNTO % sizeof(uint32_t)));
-    return hash_bytes32(ALIGNED_CAST(const uint32_t *, key), key_len, 0);
+    static struct ovsthread_once once = OVSTHREAD_ONCE_INITIALIZER;
+    static uint32_t secret;
+
+    if (ovsthread_once_start(&once)) {
+        secret = random_uint32();
+        ovsthread_once_done(&once);
+    }
+    hash_bytes128(key, key_len, secret, hash);
+    uuid_set_bits_v4((struct uuid *)hash);
 }
 
 static void
diff --git a/lib/odp-util.h b/lib/odp-util.h
index f15e258e6..4ecce1aac 100644
--- a/lib/odp-util.h
+++ b/lib/odp-util.h
@@ -243,7 +243,7 @@  struct odp_flow_key_parms {
 void odp_flow_key_from_flow(const struct odp_flow_key_parms *, struct ofpbuf *);
 void odp_flow_key_from_mask(const struct odp_flow_key_parms *, struct ofpbuf *);
 
-uint32_t odp_flow_key_hash(const struct nlattr *, size_t);
+void odp_flow_key_hash(const void *key, size_t key_len, ovs_u128 *hash);
 
 /* Estimated space needed for metadata. */
 enum { ODP_KEY_METADATA_SIZE = 9 * 8 };
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index b17c6cdca..9bb535bdf 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -923,7 +923,7 @@  check_ufid(struct dpif_backer *backer)
 
     ofpbuf_use_stack(&key, &keybuf, sizeof keybuf);
     odp_flow_key_from_flow(&odp_parms, &key);
-    dpif_flow_hash(backer->dpif, key.data, key.size, &ufid);
+    odp_flow_key_hash(key.data, key.size, &ufid);
 
     enable_ufid = dpif_probe_feature(backer->dpif, "UFID", &key, NULL, &ufid);