RE: [RFC PATCH v2 4/9] vhost: fix async access

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

> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Thursday, March 31, 2022 4:00 PM
> To: David Marchand <david.marchand@redhat.com>; dev@dpdk.org
> Cc: Xia, Chenbo <chenbo.xia@intel.com>; Hu, Jiayu <jiayu.hu@intel.com>;
> Wang, YuanX <yuanx.wang@intel.com>; Ding, Xuan <xuan.ding@intel.com>;
> stable@dpdk.org; Patrick Fu <patrick.fu@intel.com>
> Subject: Re: [RFC PATCH v2 4/9] vhost: fix async access
> 
> Hi Jiayu,
> 
> On 3/30/22 15:49, David Marchand wrote:
> > vq->async accesses must be protected with vq->access_lock.
> >
> > Fixes: eb666d24085f ("vhost: fix async unregister deadlock")
> > Fixes: 0c0935c5f794 ("vhost: allow to check in-flight packets for
> > async vhost")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: David Marchand <david.marchand@redhat.com>
> > ---
> >   lib/vhost/vhost.c | 25 ++++++++++---------------
> >   1 file changed, 10 insertions(+), 15 deletions(-)
> 
> Could you please test and review below patch?
> We may want to apply it early, before the annotation series is applied.

Sure, I will review them.

Thanks,
Jiayu
> 
> Thanks!
> Maxime
> 
> > diff --git a/lib/vhost/vhost.c b/lib/vhost/vhost.c index
> > 2f96a28dac..a93e41f314 100644
> > --- a/lib/vhost/vhost.c
> > +++ b/lib/vhost/vhost.c
> > @@ -1753,27 +1753,23 @@ rte_vhost_async_channel_unregister(int vid,
> uint16_t queue_id)
> >   	if (vq == NULL)
> >   		return ret;
> >
> > -	ret = 0;
> > -
> > -	if (!vq->async)
> > -		return ret;
> > -
> >   	if (!rte_spinlock_trylock(&vq->access_lock)) {
> >   		VHOST_LOG_CONFIG(ERR, "(%s) failed to unregister async
> channel, virtqueue busy.\n",
> >   				dev->ifname);
> > -		return -1;
> > +		return ret;
> >   	}
> >
> > -	if (vq->async->pkts_inflight_n) {
> > +	if (!vq->async) {
> > +		ret = 0;
> > +	} else if (vq->async->pkts_inflight_n) {
> >   		VHOST_LOG_CONFIG(ERR, "(%s) failed to unregister async
> channel.\n", dev->ifname);
> >   		VHOST_LOG_CONFIG(ERR, "(%s) inflight packets must be
> completed before unregistration.\n",
> >   			dev->ifname);
> > -		ret = -1;
> > -		goto out;
> > +	} else {
> > +		vhost_free_async_mem(vq);
> > +		ret = 0;
> >   	}
> >
> > -	vhost_free_async_mem(vq);
> > -out:
> >   	rte_spinlock_unlock(&vq->access_lock);
> >
> >   	return ret;
> > @@ -1891,9 +1887,6 @@ rte_vhost_async_get_inflight(int vid, uint16_t
> queue_id)
> >   	if (vq == NULL)
> >   		return ret;
> >
> > -	if (!vq->async)
> > -		return ret;
> > -
> >   	if (!rte_spinlock_trylock(&vq->access_lock)) {
> >   		VHOST_LOG_CONFIG(DEBUG,
> >   			"(%s) failed to check in-flight packets. virtqueue
> busy.\n", @@
> > -1901,7 +1894,9 @@ rte_vhost_async_get_inflight(int vid, uint16_t
> queue_id)
> >   		return ret;
> >   	}
> >
> > -	ret = vq->async->pkts_inflight_n;
> > +	if (vq->async)
> > +		ret = vq->async->pkts_inflight_n;
> > +
> >   	rte_spinlock_unlock(&vq->access_lock);
> >
> >   	return ret;