It's highly unlikely that two flows will hash to the same UID. However,
we could handle multiple upcalls from the same flow. If we hash the flow
with an additional hash function and the hashes are the same, then the
upcalls are most likely from the same flow (assuming that the hash
collisions are different for each functions). If the hashes 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 <joestrin...@nicira.com>
---
RFC. This is really unlikely, but we don't have any way of detecting
this case currently. This makes it likely that we'll detect such cases,
but it's still theoretically possible we'll miss them. Not sure whether
the extra code is warranted or not. During testing I've only triggered
the handler_duplicate_upcall case and never the VLOG_WARN case.
---
ofproto/ofproto-dpif-upcall.c | 48 +++++++++++++++++++++++++++++++++++------
1 file changed, 42 insertions(+), 6 deletions(-)
diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
index ad50f79..e663f78 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);
@@ -99,6 +99,8 @@ struct udpif {
struct dpif *dpif; /* Datapath handle. */
struct dpif_backer *backer; /* Opaque dpif_backer pointer. */
+ uint32_t secret; /* Random seed for udpif_key 'check'. */
+
struct handler *handlers; /* Upcall handlers. */
size_t n_handlers;
@@ -207,6 +209,8 @@ struct udpif_key {
struct ofpbuf *actions; /* Datapath flow actions as nlattrs. */
ovs_u128 uid; /* Unique flow identifier. */
uint32_t hash; /* Pre-computed hash for 'key'. */
+ uint32_t check; /* Additional hash for detecting duplicate
+ upcalls. */
atomic_bool installed; /* True if the datapath flow has been
installed and handlers are finished with
this ukey. Used to reduce contention. */
@@ -268,7 +272,7 @@ static void upcall_unixctl_set_flow_limit(struct
unixctl_conn *conn, int argc,
static void upcall_unixctl_dump_wait(struct unixctl_conn *conn, int argc,
const char *argv[], void *aux);
-static struct udpif_key *ukey_new(struct upcall *);
+static struct udpif_key *ukey_new(struct udpif *udpif, struct upcall *);
static bool ukey_install_start(struct udpif *, struct udpif_key *ukey);
static void ukey_install_finish(struct udpif_key *ukey, int error);
static bool ukey_install(struct udpif *udpif, struct udpif_key *ukey);
@@ -316,6 +320,7 @@ udpif_create(struct dpif_backer *backer, struct dpif *dpif)
udpif->dpif = dpif;
udpif->backer = backer;
atomic_init(&udpif->flow_limit, MIN(ofproto_flow_limit, 10000));
+ udpif->secret = random_uint32();
udpif->reval_seq = seq_create();
udpif->dump_seq = seq_create();
latch_init(&udpif->exit_latch);
@@ -958,7 +963,7 @@ upcall_xlate(struct udpif *udpif, struct upcall *upcall,
&upcall->put_actions);
}
- upcall->ukey = ukey_new(upcall);
+ upcall->ukey = ukey_new(udpif, upcall);
}
static void
@@ -1236,7 +1241,23 @@ ukey_lookup(struct udpif *udpif, const ovs_u128 *uid)
}
static struct udpif_key *
-ukey_new(struct upcall *upcall)
+ukey_lookup_by_uid_check(struct udpif *udpif, const ovs_u128 *uid,
+ const uint32_t check)
+{
+ struct udpif_key *ukey;
+ int idx = get_uid_hash(uid) % N_UMAPS;
+ struct cmap *cmap = &udpif->ukeys[idx].cmap;
+
+ CMAP_FOR_EACH_WITH_HASH (ukey, cmap_node, get_uid_hash(uid), cmap) {
+ if (!memcmp(&ukey->check, &check, sizeof check)) {
+ return ukey;
+ }
+ }
+ return NULL;
+}
+
+static struct udpif_key *
+ukey_new(struct udpif *udpif, struct upcall *upcall)
OVS_NO_THREAD_SAFETY_ANALYSIS
{
struct udpif_key *ukey = xzalloc(sizeof *ukey);
@@ -1268,6 +1289,8 @@ ukey_new(struct upcall *upcall)
ukey->mask_len = ofpbuf_size(&mask);
memcpy(&ukey->uid, upcall->uid, sizeof ukey->uid);
ukey->hash = get_uid_hash(&ukey->uid);
+ ukey->check = hash_words(ALIGNED_CAST(uint32_t*,ukey->key), ukey->key_len,
+ udpif->secret);
ukey->actions = ofpbuf_clone(&upcall->put_actions);
atomic_init(&ukey->installed, false);
@@ -1300,8 +1323,21 @@ ukey_install_start(struct udpif *udpif, struct udpif_key
*ukey)
cmap_insert(&umap->cmap, &ukey->cmap_node, ukey->hash);
locked = true;
} else {
- COVERAGE_INC(handler_install_conflict);
- VLOG_DBG("Failed to ukey_install_start(): 0x%"PRIx32, ukey->hash);
+ /* It's highly unlikely that two flows will hash to the same UID.
+ * However, we could be handling multiple upcalls from the same flow.
+ * If we hash the flow with a different hash function and the hashes
+ * are the same, then the upcalls are most likely from the same flow.
+ * If the hashes are different, then we have generated the same UID
+ * hash for two different flows. */
+ if (ukey_lookup_by_uid_check(udpif, &ukey->uid, ukey->check)) {
+ COVERAGE_INC(handler_duplicate_upcall);
+ } else {
+ struct ds ds = DS_EMPTY_INITIALIZER;
+
+ odp_format_uid(&ukey->uid, &ds);
+ VLOG_WARN("Conflicting ukey with %s", ds_cstr(&ds));
+ ds_destroy(&ds);
+ }
}
ovs_mutex_unlock(&umap->mutex);
--
1.7.10.4
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
