[ovs-dev] [RFCv3 12/13] revalidator: Reduce ukey contention.

[Joe Stringer]

When handler threads are installing ukeys, they hold the ukey mutex for
the duration of flow setup. If a revalidator thread dumps this flow
while the handler holds the lock, it will attempt to lock it using
ovs_mutex_trylock(), then if it cannot grab the lock, skip the flow.

Attempting to lock on a ukey that is currently locked is a common
occurrence when:
 - There are many handler threads running, and
 - Large numbers of small flows are passing through OVS.

In these cases, the act of attempting to lock the ukey is quite
expensive. This patch adds a flag 'installed' to the ukey, which is
raised when flow setup is complete. By checking this flag before
attempting to perform a trylock(), we can reduce lock contention between
handler and revalidator threads.

Signed-off-by: Joe Stringer <joestringer at nicira.com>
---
v3: No change.
v2: No change.
---
 ofproto/ofproto-dpif-upcall.c |   29 +++++++++++++++++++++++++----
 1 file changed, 25 insertions(+), 4 deletions(-)

diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
index 81ea1c0..93c13ab 100644
--- a/ofproto/ofproto-dpif-upcall.c
+++ b/ofproto/ofproto-dpif-upcall.c
@@ -208,14 +208,17 @@ 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'. */
+    atomic_bool installed;         /* True if the datapath flow has been
+                                      installed and handlers are finished with
+                                      this ukey. Used to reduce contention. */
 
     struct ovs_mutex mutex;                   /* Guards the following. */
     struct dpif_flow_stats stats OVS_GUARDED; /* Last known stats.*/
     long long int created OVS_GUARDED;        /* Estimate of creation time. */
     uint64_t dump_seq OVS_GUARDED;            /* Tracks udpif->dump_seq. */
     uint64_t reval_seq OVS_GUARDED;           /* Tracks udpif->reval_seq. */
-    bool flow_exists OVS_GUARDED;             /* Ensures flows are only deleted
-                                                 once. */
+    bool flow_exists OVS_GUARDED;             /* The flow is currently in the
+                                                 datapath. */
 
     struct xlate_cache *xcache OVS_GUARDED;   /* Cache for xlate entries that
                                                * are affected by this ukey.
@@ -1312,6 +1315,7 @@ ukey_new(struct udpif *udpif, struct upcall *upcall)
     ukey->hash = get_uid_hash(&ukey->uid);
     ukey->actions = ofpbuf_clone(&upcall->put_actions);
 
+    atomic_init(&ukey->installed, false);
     ovs_mutex_init(&ukey->mutex);
     ukey->dump_seq = upcall->dump_seq;
     ukey->reval_seq = upcall->reval_seq;
@@ -1355,6 +1359,7 @@ ukey_install_finish(struct udpif_key *ukey, int error)
 {
     if (!error) {
         ukey->flow_exists = true;
+        atomic_store(&ukey->installed, true);
     }
     ovs_mutex_unlock(&ukey->mutex);
 }
@@ -1370,6 +1375,22 @@ ukey_install(struct udpif *udpif, struct udpif_key *ukey)
     return true;
 }
 
+/* Wrapper for ovs_mutex_trylock() which reduces contention between handler
+ * threads and revalidator threads by not bothering to try locking if the
+ * flow is still being processed. */
+static int
+ukey_trylock(struct udpif_key *ukey)
+    OVS_TRY_LOCK(0, ukey->mutex)
+{
+    bool installed;
+
+    atomic_read(&ukey->installed, &installed);
+    if (!installed) {
+        return EBUSY;
+    }
+    return ovs_mutex_trylock(&ukey->mutex);
+}
+
 /* Searches for a ukey in 'udpif->ukeys' that matches 'flow' and attempts to
  * lock the ukey.
  *
@@ -1391,7 +1412,7 @@ ukey_acquire(struct udpif *udpif, const struct dpif_flow *flow,
     ukey = ukey_lookup(udpif, flow->uid, flow->uid_len,
                        flow->key, flow->key_len);
     if (ukey) {
-        retval = ovs_mutex_trylock(&ukey->mutex);
+        retval = ukey_trylock(ukey);
     } else {
         struct ds ds = DS_EMPTY_INITIALIZER;
         ovs_u128 uid;
@@ -1848,7 +1869,7 @@ revalidator_sweep__(struct revalidator *revalidator, bool purge)
 
             /* Handler threads could be holding a ukey lock while it installs a
              * new flow, so don't hang around waiting for access to it. */
-            if (ovs_mutex_trylock(&ukey->mutex)) {
+            if (ukey_trylock(ukey)) {
                 continue;
             }
             flow_exists = ukey->flow_exists;
-- 
1.7.10.4