Skip to content

Commit

Permalink
ofproto-dpif: Use fat_rwlock instead of ovs_rwlock.
Browse files Browse the repository at this point in the history 
This patch fixes a deadlock introduced by commit 6b59b54 (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
84f0f29 (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>
  • Loading branch information
Ansis Atteka committed Apr 7, 2015
1 parent ae2c62e commit d91f11d
Show file tree
Hide file tree
Showing 4 changed files with 20 additions and 18 deletions.
16 changes: 8 additions & 8 deletions ofproto/ofproto-dpif-xlate.c
View file
Expand Up @@ -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. */
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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;
}

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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,
Expand Down
2 changes: 1 addition & 1 deletion ofproto/ofproto-dpif-xlate.h
View file
Expand Up @@ -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,
Expand Down
18 changes: 10 additions & 8 deletions ofproto/ofproto-dpif.c
View file
Expand Up @@ -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;
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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. */
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down
2 changes: 1 addition & 1 deletion ofproto/ofproto-dpif.h
View file
Expand Up @@ -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.
*
Expand Down

0 comments on commit d91f11d

Please sign in to comment.