Hi Maxime,

-----Original Message-----
From: Maxime Coquelin <redacted>
Sent: Monday, March 29, 2021 11:19 PM
To: Hu, Jiayu <redacted>; dev@dpdk.org
Cc: Xia, Chenbo <redacted>; Wang, Yinan
[off-list ref]; Jiang, Cheng1 [off-list ref]; Pai G,
Sunil [off-list ref]
Subject: Re: [PATCH 3/4] vhost: avoid deadlock on async register



On 3/17/21 1:56 PM, Jiayu Hu wrote:

quoted

Users register async copy device when vhost queue is enabled.
However, if VHOST_USER_F_PROTOCOL_FEATURES is not supported,
a deadlock occurs inside rte_vhost_async_channel_register(),
as vhost_user_msg_handler() already takes vq->access_lock
before processing VHOST_USER_SET_VRING_KICK message.

This patch removes calling vring_state_changed() in
vhost_user_set_vring_kick() to avoid deadlock on async register.

Signed-off-by: Jiayu Hu <redacted>
---
 lib/librte_vhost/vhost_user.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
index 399675c..a319c1c 100644
--- a/lib/librte_vhost/vhost_user.c
+++ b/lib/librte_vhost/vhost_user.c

@@ -1919,9 +1919,6 @@ vhost_user_set_vring_kick(struct virtio_net

**pdev, struct VhostUserMsg *msg,

quoted

 	 */
 	if (!(dev->features & (1ULL <<

VHOST_USER_F_PROTOCOL_FEATURES))) {

quoted

 		vq->enabled = 1;
-		if (dev->notify_ops->vring_state_changed)
-			dev->notify_ops->vring_state_changed(
-				dev->vid, file.index, 1);

That looks very wrong, as:
1. The apps want to receive this notification. It looks like breaking
existing apps in order to support the experimental async datapath. E.g.
OVS needs it to start polling the queues when protocol features is not
negotiated.

IMHO, if protocol feature is not negotiated, vring_state_chaned will also
be called in vhost_user_msg_handler. In the case you mentioned,
vq->enabled is set to true in set_vring_kick, and in vhost_user_msg_handler,
"cur_ready != (vq && vq->ready)" is true, as vq->ready is false when init. So
vhost_user_msg_handler will call vhost_user_notify_queue_state, which
calls set_vring_kick inside.

In addition, calling vring_state_changed in set_vring_kick is protected by lock,
but it's not in in vhost_user_msg_handler. It looks confusing to me. Is there
any special reason for this design?

2. The fix in your case seems to indicate that your app's
vring_state_changed callback called rte_vhost_async_channel_register.
And your fix consists in no more calling the callback, and so no more
calling rte_vhost_async_channel_register?

rte_vhost_async_channel_register is recommended to call in
vring_state_changed, and vring_state_changed will be called
by vhost_user_msg_handler.

Thanks,
Jiayu

quoted

 	}

 	if (vq->ready) {