On 4/20/23 09:39, Eelco Chaudron wrote: > When doing performance testing with OVS v3.1 we ran into a deadlock > situation with the netdev_hmap_rwlock read/write lock. After some > debugging, it was discovered that the netdev_hmap_rwlock read lock > was taken recursively. And well in the folowing sequence of events: > > netdev_ports_flow_get() > It takes the read lock, while it walks all the ports > in the port_to_netdev hmap and calls: > - netdev_flow_get() which will call: > - netdev_tc_flow_get() which will call: > - netdev_ifindex_to_odp_port() > This function also takes the same read lock to > walk the ifindex_to_port hmap. > > In OVS a read/write lock does not support recursive readers. For details > see the comments in ovs-thread.h. If you do this, it will lock up, > mainly due to OVS setting the PTHREAD_RWLOCK_PREFER_WRITER_NONRECURSIVE_NP > attribute to the lock. > > The solution with this patch is to use two separate read/write > locks, with an order guarantee to avoid another potential deadlock. > > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2182541 > Fixes: 9fe21a4fc12a ("netdev-offload: replace netdev_hmap_mutex to > netdev_hmap_rwlock") > Signed-off-by: Eelco Chaudron <[email protected]> > --- > lib/netdev-offload.c | 70 > +++++++++++++++++++++++++++----------------------- > 1 file changed, 38 insertions(+), 32 deletions(-) > > diff --git a/lib/netdev-offload.c b/lib/netdev-offload.c > index 4592262bd..f34981053 100644 > --- a/lib/netdev-offload.c > +++ b/lib/netdev-offload.c > @@ -485,11 +485,13 @@ netdev_set_hw_info(struct netdev *netdev, int type, int > val) > } > > /* Protects below port hashmaps. */ > -static struct ovs_rwlock netdev_hmap_rwlock = OVS_RWLOCK_INITIALIZER; > +static struct ovs_rwlock netdev_ifindex_hmap_rwlock = OVS_RWLOCK_INITIALIZER; > +static struct ovs_rwlock netdev_port_hmap_rwlock \ > + OVS_ACQ_BEFORE(netdev_ifindex_hmap_rwlock) = OVS_RWLOCK_INITIALIZER; > > -static struct hmap port_to_netdev OVS_GUARDED_BY(netdev_hmap_rwlock) > +static struct hmap port_to_netdev OVS_GUARDED_BY(netdev_port_hmap_rwlock) > = HMAP_INITIALIZER(&port_to_netdev); > -static struct hmap ifindex_to_port OVS_GUARDED_BY(netdev_hmap_rwlock) > +static struct hmap ifindex_to_port OVS_GUARDED_BY(netdev_ifindex_hmap_rwlock) > = HMAP_INITIALIZER(&ifindex_to_port);
Hi, Eelco. Thanks for the fix! Looks good in general. One nit: maybe it's better to rename these locks to something more descriptive? Otherwise, they are a bit hard to tell from each other on a quick read. What do you think about something like: netdev_port_hmap_rwlock --> port_to_netdev_rwlock netdev_ifindex_hmap_rwlock --> ifindex_to_port_rwlock ? Best regards, Ilya Maximets. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
