>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 ><[email protected]> >--- > 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(); As you mentioned, this locks the whole revalidation process. Is it possible to narrow down the lock scope? maybe on tnl_port_map_lookup? Besides revalidation, I think upcall xlation may also need the 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 > >_______________________________________________ >dev mailing list >[email protected] >https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
_______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
