On 26 Apr 2023, at 23:54, Ilya Maximets wrote:
> 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 <echau...@redhat.com>
>> ---
>> 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
>
> ?
Sounds like a good plan, will rename the locks and send out a v2.
Cheers,
Eelco
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
