[dpdk-dev] [ovs-dev] [PATCH] vhost: return -EAGAIN during unregistering vhost if it is busy.

Maxime Coquelin maxime.coquelin at redhat.com
Wed May 6 09:53:25 CEST 2020


Hi Zhike,

On 5/6/20 4:39 AM, 王志克 wrote:
> NO, it is different issue.
> 
> The current  deadlock  mentioned in this patch is caused by some
> blocking function (like ovsrcu_synchronize) in application (like OVS).
> In this patch, the application is needed  to break  the logical deadlock. 

Ok, so we need either a non-blocking rte_vhost_driver_unregister() in
DPDK, or a non-blocking .destroy_device() in OVS. Thanks for the
clarification, I misread your original commit message.

The problem with your patch is that you break the DPDK ABI, as current
unregistering API is supposed to be blocking. Meaning, that could not be
backported to older stable branches.

What we can do on DPDK side is to provide a new non-blocking API for
unregistering, or do

Any chance there is a way for OVS to prevent that?

Thanks,
Maxime

> 
> 
> 
> 
> 
> 
> At 2020-04-27 16:09:31, "Maxime Coquelin" <maxime.coquelin at redhat.com> wrote:
>>
>>
>>On 3/18/20 4:31 AM, 王志克 wrote:
>>> Involve openvswitch group since this fix is highly coupled with OVS.
>>> welcome comment.
>>> At 2020-03-12 17:57:19, "Zhike Wang" <wangzhike at jd.com> wrote:
>>>> The vhost_user_read_cb() and rte_vhost_driver_unregister()
>>>> can be called at the same time by 2 threads, and may lead to deadlock.
>>>> Eg thread1 calls vhost_user_read_cb()->vhost_user_get_vring_base()->destroy_device(),
>>>> then thread2 calls rte_vhost_driver_unregister(), and will retry the fdset_try_del() in loop.
>>>>
>>>> Some application implements destroy_device() as a blocking function, eg
>>>> OVS calls ovsrcu_synchronize() insides destroy_device(). As a result,
>>>> thread1(eg vhost_events) is blocked to wait quiesce of thread2(eg ovs-vswitchd),
>>>> and thread2 is in a loop to wait thread1 to give up the use of the vhost fd,
>>>> then leads to deadlock.
>>>>
>>>> It is better to return -EAGAIN to application, who will decide how to handle
>>>> (eg OVS can call ovsrcu_quiesce() and then retry).
>>>>
>>>> Signed-off-by: Zhike Wang <wangzhike at jd.com>
>>>> ---
>>>> lib/librte_vhost/rte_vhost.h | 4 +++-
>>>> lib/librte_vhost/socket.c    | 8 ++++----
>>>> 2 files changed, 7 insertions(+), 5 deletions(-)
>>
>>
>>Isn't it fixed with below commit that landed into DPDK v20.02?
>>
>>commit 5efb18e85f7fdb436d3e56591656051c16802066
>>Author: Maxime Coquelin <maxime.coquelin at redhat.com>
>>Date:   Tue Jan 14 19:53:57 2020 +0100
>>
>>    vhost: fix deadlock on port deletion
>>
>>    If the vhost-user application (e.g. OVS) deletes the vhost-user
>>    port while Qemu sends a vhost-user request, a deadlock can
>>    happen if the request handler tries to acquire vhost-user's
>>    global mutex, which is also locked by the vhost-user port
>>    deletion API (rte_vhost_driver_unregister).
>>
>>    This patch prevents the deadlock by making
>>    rte_vhost_driver_unregister() to release the mutex and try
>>    again if a request is being handled to give a chance to
>>    the request handler to complete.
>>
>>    Fixes: 8b4b949144b8 ("vhost: fix dead lock on closing in server mode")
>>    Fixes: 5fbb3941da9f ("vhost: introduce driver features related APIs")
>>    Cc: stable at dpdk.org
>>
>>    Signed-off-by: Maxime Coquelin <maxime.coquelin at redhat.com>
>>    Reviewed-by: Tiwei Bie <tiwei.bie at intel.com>
>>    Acked-by: Eelco Chaudron <echaudro at redhat.com>
>>
>>>> diff --git a/lib/librte_vhost/rte_vhost.h b/lib/librte_vhost/rte_vhost.h
>>>> index c7b619a..276db11 100644
>>>> --- a/lib/librte_vhost/rte_vhost.h
>>>> +++ b/lib/librte_vhost/rte_vhost.h
>>>> @@ -389,7 +389,9 @@ void rte_vhost_log_used_vring(int vid, uint16_t vring_idx,
>>>>  */
>>>> int rte_vhost_driver_register(const char *path, uint64_t flags);
>>>>
>>>> -/* Unregister vhost driver. This is only meaningful to vhost user. */
>>>> +/* Unregister vhost driver. This is only meaningful to vhost user.
>>>> + * Return -EAGAIN if device is busy, and leave it to be handled by application.
>>>> + */
>>>> int rte_vhost_driver_unregister(const char *path);
>>>>
>>>> /**
>>>> diff --git a/lib/librte_vhost/socket.c b/lib/librte_vhost/socket.c
>>>> index 7c80121..a75a3f6 100644
>>>> --- a/lib/librte_vhost/socket.c
>>>> +++ b/lib/librte_vhost/socket.c
>>>> @@ -1027,7 +1027,8 @@ struct vhost_user_reconnect_list {
>>>> }
>>>>
>>>> /**
>>>> - * Unregister the specified vhost socket
>>>> + * Unregister the specified vhost socket.
>>>> + * Return -EAGAIN if device is busy, and leave it to be handled by application.
>>>>  */
>>>> int
>>>> rte_vhost_driver_unregister(const char *path)
>>>> @@ -1039,7 +1040,6 @@ struct vhost_user_reconnect_list {
>>>> 	if (path == NULL)
>>>> 		return -1;
>>>>
>>>> -again:
>>>> 	pthread_mutex_lock(&vhost_user.mutex);
>>>>
>>>> 	for (i = 0; i < vhost_user.vsocket_cnt; i++) {
>>>> @@ -1063,7 +1063,7 @@ struct vhost_user_reconnect_list {
>>>> 					pthread_mutex_unlock(
>>>> 							&vsocket->conn_mutex);
>>>> 					pthread_mutex_unlock(&vhost_user.mutex);
>>>> -					goto again;
>>>> +					return -EAGAIN;
>>>> 				}
>>>>
>>>> 				VHOST_LOG_CONFIG(INFO,
>>>> @@ -1085,7 +1085,7 @@ struct vhost_user_reconnect_list {
>>>> 				if (fdset_try_del(&vhost_user.fdset,
>>>> 						vsocket->socket_fd) == -1) {
>>>> 					pthread_mutex_unlock(&vhost_user.mutex);
>>>> -					goto again;
>>>> +					return -EAGAIN;
>>>> 				}
>>>>
>>>> 				close(vsocket->socket_fd);
>>>> -- 
>>>> 1.8.3.1
>>>>
>>> _______________________________________________
>>> dev mailing list
>>> dev at openvswitch.org
>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>> 
> 
> 
> 
>  
> 



More information about the dev mailing list