ofproto-dpif: Use fat_rwlock instead of ovs_rwlock.

This patch fixes a deadlock introduced by commit 6b59b543 (ovs-thread:
Use fair (but nonrecursive) rwlocks on glibc.)

If STP is enabled, then a handler thread could have already had
acquired "xlate_rwlock" in xlate_actions() and then might have
attempt to acquire it again in xlate_send_packet() leading to
a deadlock:

pthread_rwlock_rdlock () from /lib/x86_64-linux-gnu/libpthread.so.0
ovs_rwlock_rdlock_at (l_=0x769cc0, where=0x4f4568 "../ofproto/ofproto-dpif-xlate.c:3600") at ../lib/ovs-thread.c:71
xlate_send_packet (ofport=0x23b6400, packet=0x7f980400a8d0) at ../ofproto/ofproto-dpif-xlate.c:3600
ofproto_dpif_send_packet (ofport=<optimized out>, packet=0x7f980400a8d0) at ../ofproto/ofproto-dpif.c:3684
send_bpdu_cb (pkt=0x7f980400a8d0, port_num=0, ofproto_=0x229a410) at ../ofproto/ofproto-dpif.c:1927
stp_send_bpdu (p=0x2400c00, bpdu=0x7f980f7e3080, bpdu_size=35) at ../lib/stp.c:1558
stp_transmit_config (p=0x2400c00) at ../lib/stp.c:1052
stp_acknowledge_topology_change (p=<optimized out>) at ../lib/stp.c:1301
stp_received_tcn_bpdu (p=<optimized out>, stp=<optimized out>) at ../lib/stp.c:1353
stp_received_bpdu (p=0x2400c00, bpdu=0x7f980f7f81e9, bpdu_size=<optimized out>) at ../lib/stp.c:771
stp_process_packet (packet=0x7f980f7f80f8, xport=0x24594b0) at ../ofproto/ofproto-dpif-xlate.c:840
process_special (flow=<optimized out>, xport=0x24594b0, packet=0x7f980f7f80f8, ctx=<optimized out>) at ../ofproto/ofproto-dpif-xlate.c:1832
compose_output_action__ (ctx=0x7f980f7e3730, ofp_port=<optimized out>, check_stp=true) at ../ofproto/ofproto-dpif-xlate.c:1894
compose_output_action (ofp_port=<optimized out>, ctx=0x7f980f7e3730) at ../ofproto/ofproto-dpif-xlate.c:2031
output_normal (ctx=0x7f980f7e3730, out_xbundle=0x23d13a0, vlan=<optimized out>) at ../ofproto/ofproto-dpif-xlate.c:1316
xlate_normal (ctx=0x7f980f7e3730) at ../ofproto/ofproto-dpif-xlate.c:1625
xlate_output_action (ctx=0x7f980f7e3730, port=<optimized out>, max_len=<optimized out>, may_packet_in=<optimized out>) at ../ofproto/ofproto-dpif-xlate.c:2540
do_xlate_actions (ofpacts=<optimized out>, ofpacts_len=<optimized out>, ctx=0x7f980f7e3730) at ../ofproto/ofproto-dpif-xlate.c:2833
xlate_actions__ (xin=0x7f980f7fda40, xout=0x7f980f7e41f0) at ../ofproto/ofproto-dpif-xlate.c:3485
xlate_actions (xin=0x7f980f7fda40, xout=0x7f980f7e41f0) at ../ofproto/ofproto-dpif-xlate.c:3223
xlate_actions_for_side_effects (xin=<optimized out>) at ../ofproto/ofproto-dpif-xlate.c:3136
handle_upcalls (n_upcalls=50, upcalls=0x7f980f7f3080, misses=0x7f980f7fd890, handler=<optimized out>) at ../ofproto/ofproto-dpif-upcall.c:973
udpif_upcall_handler (arg=0x23e91e0) at ../ofproto/ofproto-dpif-upcall.c:541
ovsthread_wrapper (aux_=<optimized out>) at ../lib/ovs-thread.c:322
start_thread () from /lib/x86_64-linux-gnu/libpthread.so.0
clone () from /lib/x86_64-linux-gnu/libc.so.6
?? ()

The patch fixes this deadlock by using fat_rwlock that still allows
to acquire read lock in a recursive manner.

This bug is not present in master branch because commit
84f0f298 (ofproto-dpif-xlate: Implement RCU locking in
ofproto-dpif-xlate) removed xlate_rwlock.

VMware-BZ: #1425671
Reported-by: Scott Hendricks <shendricks@nicira.com>
Signed-off-by: Ansis Atteka <aatteka@nicira.com>
Acked-by: Ben Pfaff <blp@nicira.com>
Acked-by: Alex Wang <alexw@nicira.com>

diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index b0f3f89..c5dc9e8 100644 (file)
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -66,7 +66,7 @@ VLOG_DEFINE_THIS_MODULE(ofproto_dpif_xlate);
  * recursive or not. */
 #define MAX_RESUBMITS (MAX_RESUBMIT_RECURSION * MAX_RESUBMIT_RECURSION)
 
-struct ovs_rwlock xlate_rwlock = OVS_RWLOCK_INITIALIZER;
+struct fat_rwlock xlate_rwlock;
 
 struct xbridge {
     struct hmap_node hmap_node;   /* Node in global 'xbridges' map. */
@@ -639,7 +639,7 @@ xlate_receive(const struct dpif_backer *backer, struct ofpbuf *packet,
     const struct xport *xport;
     int error = ENODEV;
 
-    ovs_rwlock_rdlock(&xlate_rwlock);
+    fat_rwlock_rdlock(&xlate_rwlock);
     if (odp_flow_key_to_flow(key, key_len, flow) == ODP_FIT_ERROR) {
         error = EINVAL;
         goto exit;
@@ -721,7 +721,7 @@ xlate_receive(const struct dpif_backer *backer, struct ofpbuf *packet,
     }
 
 exit:
-    ovs_rwlock_unlock(&xlate_rwlock);
+    fat_rwlock_unlock(&xlate_rwlock);
     return error;
 }
 
@@ -3231,9 +3231,9 @@ void
 xlate_actions(struct xlate_in *xin, struct xlate_out *xout)
     OVS_EXCLUDED(xlate_rwlock)
 {
-    ovs_rwlock_rdlock(&xlate_rwlock);
+    fat_rwlock_rdlock(&xlate_rwlock);
     xlate_actions__(xin, xout);
-    ovs_rwlock_unlock(&xlate_rwlock);
+    fat_rwlock_unlock(&xlate_rwlock);
 }
 
 /* Returns the maximum number of packets that the Linux kernel is willing to
@@ -3609,15 +3609,15 @@ xlate_send_packet(const struct ofport_dpif *ofport, struct ofpbuf *packet)
     flow_extract(packet, NULL, &flow);
     flow.in_port.ofp_port = OFPP_NONE;
 
-    ovs_rwlock_rdlock(&xlate_rwlock);
+    fat_rwlock_rdlock(&xlate_rwlock);
     xport = xport_lookup(ofport);
     if (!xport) {
-        ovs_rwlock_unlock(&xlate_rwlock);
+        fat_rwlock_unlock(&xlate_rwlock);
         return EINVAL;
     }
     output.port = xport->ofp_port;
     output.max_len = 0;
-    ovs_rwlock_unlock(&xlate_rwlock);
+    fat_rwlock_unlock(&xlate_rwlock);
 
     return ofproto_dpif_execute_actions(xport->xbridge->ofproto, &flow, NULL,
                                         &output.ofpact, sizeof output,

diff --git a/ofproto/ofproto-dpif-xlate.h b/ofproto/ofproto-dpif-xlate.h
index 760736a..2fdc5f1 100644 (file)
--- a/ofproto/ofproto-dpif-xlate.h
+++ b/ofproto/ofproto-dpif-xlate.h
@@ -136,7 +136,7 @@ struct xlate_in {
     struct xlate_cache *xcache;
 };
 
-extern struct ovs_rwlock xlate_rwlock;
+extern struct fat_rwlock xlate_rwlock;
 
 void xlate_ofproto_set(struct ofproto_dpif *, const char *name,
                        struct dpif *, struct rule_dpif *miss_rule,

diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 2c150b2..9b87248 100644 (file)
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -410,6 +410,8 @@ init(const struct shash *iface_hints)
 {
     struct shash_node *node;
 
+    fat_rwlock_init(&xlate_rwlock);
+
     /* Make a local copy, since we don't own 'iface_hints' elements. */
     SHASH_FOR_EACH(node, iface_hints) {
         const struct iface_hint *orig_hint = node->data;
@@ -598,7 +600,7 @@ type_run(const char *type)
                 continue;
             }
 
-            ovs_rwlock_wrlock(&xlate_rwlock);
+            fat_rwlock_wrlock(&xlate_rwlock);
             xlate_ofproto_set(ofproto, ofproto->up.name,
                               ofproto->backer->dpif, ofproto->miss_rule,
                               ofproto->no_packet_in_rule, ofproto->ml,
@@ -631,7 +633,7 @@ type_run(const char *type)
                                  ofport->up.pp.config, ofport->up.pp.state,
                                  ofport->is_tunnel, ofport->may_enable);
             }
-            ovs_rwlock_unlock(&xlate_rwlock);
+            fat_rwlock_unlock(&xlate_rwlock);
         }
 
         udpif_revalidate(backer->udpif);
@@ -1311,9 +1313,9 @@ destruct(struct ofproto *ofproto_)
     struct list pins;
 
     ofproto->backer->need_revalidate = REV_RECONFIGURE;
-    ovs_rwlock_wrlock(&xlate_rwlock);
+    fat_rwlock_wrlock(&xlate_rwlock);
     xlate_remove_ofproto(ofproto);
-    ovs_rwlock_unlock(&xlate_rwlock);
+    fat_rwlock_unlock(&xlate_rwlock);
 
     /* Ensure that the upcall processing threads have no remaining references
      * to the ofproto or anything in it. */
@@ -1666,9 +1668,9 @@ port_destruct(struct ofport *port_)
     const char *dp_port_name;
 
     ofproto->backer->need_revalidate = REV_RECONFIGURE;
-    ovs_rwlock_wrlock(&xlate_rwlock);
+    fat_rwlock_wrlock(&xlate_rwlock);
     xlate_ofport_remove(port);
-    ovs_rwlock_unlock(&xlate_rwlock);
+    fat_rwlock_unlock(&xlate_rwlock);
 
     dp_port_name = netdev_vport_get_dpif_port(port->up.netdev, namebuf,
                                               sizeof namebuf);
@@ -2315,9 +2317,9 @@ bundle_destroy(struct ofbundle *bundle)
     ofproto = bundle->ofproto;
     mbridge_unregister_bundle(ofproto->mbridge, bundle);
 
-    ovs_rwlock_wrlock(&xlate_rwlock);
+    fat_rwlock_wrlock(&xlate_rwlock);
     xlate_bundle_remove(bundle);
-    ovs_rwlock_unlock(&xlate_rwlock);
+    fat_rwlock_unlock(&xlate_rwlock);
 
     LIST_FOR_EACH_SAFE (port, next_port, bundle_node, &bundle->ports) {
         bundle_del_port(port);

diff --git a/ofproto/ofproto-dpif.h b/ofproto/ofproto-dpif.h
index cee4723..5b0a1a8 100644 (file)
--- a/ofproto/ofproto-dpif.h
+++ b/ofproto/ofproto-dpif.h
@@ -59,7 +59,7 @@ enum { TBL_INTERNAL = N_TABLES - 1 };    /* Used for internal hidden rules. */
 BUILD_ASSERT_DECL(N_TABLES >= 2 && N_TABLES <= 255);
 
 /* For lock annotation below only. */
-extern struct ovs_rwlock xlate_rwlock;
+extern struct fat_rwlock xlate_rwlock;
 
 /* Ofproto-dpif -- DPIF based ofproto implementation.
  *