[ovs-dev] [PATCH] ofproto-dpif-upcall:Fix for OVS route cache re-add

[Cheekatamarla Eswara Venkata Pavan Kumar]

Problem Statement:
OVS flushes and subsequently repopulates its route
cache whenever it receives a netlink notification
about kernel interface change. At the same time the
port addition triggers a revalidation of all
datapath flow cache entries. The revalidation of
egress tunnel flows depends on correct routing
information and can fail during the rebuild of
the route cache, which leads to temporary insertion
of drop flows.

Solution:
There is already a route_table_mutex that OVS seizes
when it rebuilds the route cache.
To ensure that flow revalidation cannot collide with
route cache rebuild, seize that route_table_mutex also
during revalidation. Since revalidation is multi-threaded,
we seize the lock only on the leader thread. As a
revalidation run can last hundreds of milliseconds,
replace the lock() with trylock() in the route table code
to avoid blocking the main OVS thread during revalidation.

Signed-off-by: Cheekatamarla Eswara Venkata Pavan Kumar <pavankumar.e at acldigital.com>
---
 lib/route-table.c             | 34 ++++++++++++++++++++++++++++------
 lib/route-table.h             |  2 ++
 ofproto/ofproto-dpif-upcall.c | 14 ++++++++++----
 3 files changed, 40 insertions(+), 10 deletions(-)

diff --git a/lib/route-table.c b/lib/route-table.c
index ac82cf262..e1b17f1b8 100644
--- a/lib/route-table.c
+++ b/lib/route-table.c
@@ -88,6 +88,21 @@ static void route_map_clear(void);
 static void name_table_init(void);
 static void name_table_change(const struct rtnetlink_change *, void *);
 
+inline void route_table_lock(void)
+{
+   ovs_mutex_lock(&route_table_mutex);
+}
+
+inline void route_table_unlock(void)
+{
+   ovs_mutex_unlock(&route_table_mutex);
+}
+
+inline int route_table_trylock(void)
+{
+   return ovs_mutex_trylock(&route_table_mutex);
+}
+
 uint64_t
 route_table_get_change_seq(void)
 {
@@ -100,7 +115,7 @@ void
 route_table_init(void)
     OVS_EXCLUDED(route_table_mutex)
 {
-    ovs_mutex_lock(&route_table_mutex);
+    route_table_lock();
     ovs_assert(!nln);
     ovs_assert(!route_notifier);
     ovs_assert(!route6_notifier);
@@ -119,7 +134,7 @@ route_table_init(void)
     route_table_reset();
     name_table_init();
 
-    ovs_mutex_unlock(&route_table_mutex);
+    route_table_unlock();
 }
 
 /* Run periodically to update the locally maintained routing table. */
@@ -127,7 +142,11 @@ void
 route_table_run(void)
     OVS_EXCLUDED(route_table_mutex)
 {
-    ovs_mutex_lock(&route_table_mutex);
+        /* Skip route table updates when the table is locked. */
+    if (route_table_trylock()) {
+        return;
+    }
+
     if (nln) {
         rtnetlink_run();
         nln_run(nln);
@@ -136,7 +155,7 @@ route_table_run(void)
             route_table_reset();
         }
     }
-    ovs_mutex_unlock(&route_table_mutex);
+    route_table_unlock();
 }
 
 /* Causes poll_block() to wake up when route_table updates are required. */
@@ -144,12 +163,15 @@ void
 route_table_wait(void)
     OVS_EXCLUDED(route_table_mutex)
 {
-    ovs_mutex_lock(&route_table_mutex);
+    if (route_table_trylock()) {
+        return;
+    }
+
     if (nln) {
         rtnetlink_wait();
         nln_wait(nln);
     }
-    ovs_mutex_unlock(&route_table_mutex);
+    route_table_unlock();
 }
 
 static int
diff --git a/lib/route-table.h b/lib/route-table.h
index 3a02d737a..58418bd3f 100644
--- a/lib/route-table.h
+++ b/lib/route-table.h
@@ -33,4 +33,6 @@ void route_table_wait(void);
 bool route_table_fallback_lookup(const struct in6_addr *ip6_dst,
                                  char name[],
                                  struct in6_addr *gw6);
+void route_table_lock(void);
+void route_table_unlock(void);
 #endif /* route-table.h */
diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
index 1c9c720f0..ab2d20833 100644
--- a/ofproto/ofproto-dpif-upcall.c
+++ b/ofproto/ofproto-dpif-upcall.c
@@ -23,6 +23,7 @@
 #include "coverage.h"
 #include "cmap.h"
 #include "lib/dpif-provider.h"
+#include "lib/route-table.h"
 #include "dpif.h"
 #include "openvswitch/dynamic-string.h"
 #include "fail-open.h"
@@ -587,7 +588,8 @@ udpif_start_threads(struct udpif *udpif, uint32_t n_handlers_,
         dpif_enable_upcall(udpif->dpif);
 
         ovs_barrier_init(&udpif->reval_barrier, udpif->n_revalidators);
-        ovs_barrier_init(&udpif->pause_barrier, udpif->n_revalidators + 1);
+        /* For main thread and leader*/
+        ovs_barrier_init(&udpif->pause_barrier, 2);
         udpif->reval_exit = false;
         udpif->pause = false;
         udpif->offload_rebalance_time = time_msec();
@@ -953,6 +955,10 @@ udpif_revalidator(void *arg)
              * on the pause_barrier */
             udpif->pause = latch_is_set(&udpif->pause_latch);
 
+            if (udpif->pause) {
+                revalidator_pause(revalidator);
+            }
+
             /* Only the leader checks the exit latch to prevent a race where
              * some threads think it's true and exit and others think it's
              * false and block indefinitely on the reval_barrier */
@@ -965,14 +971,13 @@ udpif_revalidator(void *arg)
                 terse_dump = udpif_use_ufid(udpif);
                 udpif->dump = dpif_flow_dump_create(udpif->dpif, terse_dump,
                                                     NULL);
+                /* Prevent reset of the routing table during revalidation */
+                route_table_lock();
             }
         }
 
         /* Wait for the leader to start the flow dump. */
         ovs_barrier_block(&udpif->reval_barrier);
-        if (udpif->pause) {
-            revalidator_pause(revalidator);
-        }
 
         if (udpif->reval_exit) {
             break;
@@ -990,6 +995,7 @@ udpif_revalidator(void *arg)
             unsigned int flow_limit;
             long long int duration;
 
+            route_table_unlock();
             atomic_read_relaxed(&udpif->flow_limit, &flow_limit);
 
             dpif_flow_dump_destroy(udpif->dump);
-- 
2.25.1