| Message ID | 20220330134956.18927-5-david.marchand@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | David Marchand |
| Headers | show |
| Series | vhost lock annotations | expand |
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. 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;
> -----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;
> -----Original Message----- > From: David Marchand <david.marchand@redhat.com> > Sent: Wednesday, March 30, 2022 7:20 PM > To: dev@dpdk.org > Cc: maxime.coquelin@redhat.com; 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: [RFC PATCH v2 4/9] vhost: fix async access > > 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(-) Thanks for the fix, David. Tested the changes for rte_vhost_async_get_inflight, works as expected. Although I couldn't test, the changes for rte_vhost_async_channel_unregister looks good to me . Based on that, Acked-by: Sunil Pai G <sunil.pai.g@intel.com>
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;
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(-)