[ovs-dev] [PATCHv5 12/12] upcall: Distinguish duplicate upcalls from conflicts.

[Joe Stringer]

It's highly unlikely that two flows will hash to the same UID. However,
we could handle multiple upcalls from the same flow. If the flow keys
are the same as a previous flow, then the current upcalls belongs to a
flow that already has a datapath flow installed. If the keys are
different, then we have generated the same UID hash for two different
flows, and we should log a warning.

Signed-off-by: Joe Stringer <joestringer at nicira.com>
---
v5: Compare flow keys rather than hashes of flow keys.
v4: Initial post.
---
 ofproto/ofproto-dpif-upcall.c |   38 ++++++++++++++++++++++++++++----------
 1 file changed, 28 insertions(+), 10 deletions(-)

diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
index dea3304..45c22a5 100644
--- a/ofproto/ofproto-dpif-upcall.c
+++ b/ofproto/ofproto-dpif-upcall.c
@@ -46,7 +46,7 @@
 
 VLOG_DEFINE_THIS_MODULE(ofproto_dpif_upcall);
 
-COVERAGE_DEFINE(handler_install_conflict);
+COVERAGE_DEFINE(handler_duplicate_upcall);
 COVERAGE_DEFINE(upcall_ukey_contention);
 COVERAGE_DEFINE(dumped_duplicate_flow);
 COVERAGE_DEFINE(dumped_new_flow);
@@ -1286,23 +1286,41 @@ ukey_new(struct upcall *upcall)
  * On success, returns true, installs the ukey and returns it in a locked
  * state. Otherwise, returns false. */
 static bool
-ukey_install_start(struct udpif *udpif, struct udpif_key *ukey)
-    OVS_TRY_LOCK(true, ukey->mutex)
+ukey_install_start(struct udpif *udpif, struct udpif_key *new_ukey)
+    OVS_TRY_LOCK(true, new_ukey->mutex)
 {
     struct umap *umap;
+    struct udpif_key *old_ukey;
     uint32_t idx;
     bool locked = false;
 
-    idx = ukey->hash % N_UMAPS;
+    idx = new_ukey->hash % N_UMAPS;
     umap = &udpif->ukeys[idx];
     ovs_mutex_lock(&umap->mutex);
-    if (!ukey_lookup(udpif, &ukey->uid)) {
-        ovs_mutex_lock(&ukey->mutex);
-        cmap_insert(&umap->cmap, &ukey->cmap_node, ukey->hash);
-        locked = true;
+    old_ukey = ukey_lookup(udpif, &new_ukey->uid);
+    if (old_ukey) {
+        /* Uncommon case: A ukey is already installed with the same UID. */
+        if (old_ukey->key_len == new_ukey->key_len
+            && !memcmp(old_ukey->key, new_ukey->key, new_ukey->key_len)) {
+            COVERAGE_INC(handler_duplicate_upcall);
+        } else {
+            struct ds ds = DS_EMPTY_INITIALIZER;
+
+            odp_format_uid(&old_ukey->uid, &ds);
+            ds_put_cstr(&ds, " ");
+            odp_flow_key_format(old_ukey->key, old_ukey->key_len, &ds);
+            ds_put_cstr(&ds, "\n");
+            odp_format_uid(&new_ukey->uid, &ds);
+            ds_put_cstr(&ds, " ");
+            odp_flow_key_format(new_ukey->key, new_ukey->key_len, &ds);
+
+            VLOG_WARN("Conflicting ukey for flows:\n%s", ds_cstr(&ds));
+            ds_destroy(&ds);
+        }
     } else {
-        COVERAGE_INC(handler_install_conflict);
-        VLOG_DBG("Failed to ukey_install_start(): 0x%"PRIx32, ukey->hash);
+        ovs_mutex_lock(&new_ukey->mutex);
+        cmap_insert(&umap->cmap, &new_ukey->cmap_node, new_ukey->hash);
+        locked = true;
     }
     ovs_mutex_unlock(&umap->mutex);
 
-- 
1.7.10.4