Re: [dpdk-dev] [PATCH 3/4] vhost: avoid deadlock on async register

["Hu, Jiayu" <jiayu.hu@intel.com>]

Hi Maxime,

> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Monday, March 29, 2021 11:19 PM
> To: Hu, Jiayu <jiayu.hu@intel.com>; dev@dpdk.org
> Cc: Xia, Chenbo <chenbo.xia@intel.com>; Wang, Yinan
> <yinan.wang@intel.com>; Jiang, Cheng1 <cheng1.jiang@intel.com>; Pai G,
> Sunil <sunil.pai.g@intel.com>
> Subject: Re: [PATCH 3/4] vhost: avoid deadlock on async register
> 
> 
> 
> On 3/17/21 1:56 PM, Jiayu Hu wrote:
> > 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 <jiayu.hu@intel.com>
> > ---
> >  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,
> >  	 */
> >  	if (!(dev->features & (1ULL <<
> VHOST_USER_F_PROTOCOL_FEATURES))) {
> >  		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
> 
> >  	}
> >
> >  	if (vq->ready) {
> >