| Message ID | 1537879012-20859-2-git-send-email-xiangxia.m.yue@gmail.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | net: vhost: improve performance when enable busyloop | expand |
Hi, On 25/09/2018 13:36, xiangxia.m.yue@gmail.com wrote: > From: Tonghao Zhang <xiangxia.m.yue@gmail.com> > > This patch changes the way that lock all vqs > at the same, to lock them one by one. It will > be used for next patch to avoid the deadlock. > > Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com> > Acked-by: Jason Wang <jasowang@redhat.com> > Signed-off-by: Jason Wang <jasowang@redhat.com> > --- > drivers/vhost/vhost.c | 24 +++++++----------------- > 1 file changed, 7 insertions(+), 17 deletions(-) > > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c > index b13c6b4..f52008b 100644 > --- a/drivers/vhost/vhost.c > +++ b/drivers/vhost/vhost.c > @@ -294,8 +294,11 @@ static void vhost_vq_meta_reset(struct vhost_dev *d) > { > int i; > > - for (i = 0; i < d->nvqs; ++i) > + for (i = 0; i < d->nvqs; ++i) { > + mutex_lock(&d->vqs[i]->mutex); > __vhost_vq_meta_reset(d->vqs[i]); > + mutex_unlock(&d->vqs[i]->mutex); > + } > } > > static void vhost_vq_reset(struct vhost_dev *dev, > @@ -891,20 +894,6 @@ static inline void __user *__vhost_get_user(struct vhost_virtqueue *vq, > #define vhost_get_used(vq, x, ptr) \ > vhost_get_user(vq, x, ptr, VHOST_ADDR_USED) > > -static void vhost_dev_lock_vqs(struct vhost_dev *d) > -{ > - int i = 0; > - for (i = 0; i < d->nvqs; ++i) > - mutex_lock_nested(&d->vqs[i]->mutex, i); > -} > - > -static void vhost_dev_unlock_vqs(struct vhost_dev *d) > -{ > - int i = 0; > - for (i = 0; i < d->nvqs; ++i) > - mutex_unlock(&d->vqs[i]->mutex); > -} > - > static int vhost_new_umem_range(struct vhost_umem *umem, > u64 start, u64 size, u64 end, > u64 userspace_addr, int perm) > @@ -954,7 +943,10 @@ static void vhost_iotlb_notify_vq(struct vhost_dev *d, > if (msg->iova <= vq_msg->iova && > msg->iova + msg->size - 1 >= vq_msg->iova && > vq_msg->type == VHOST_IOTLB_MISS) { > + mutex_lock(&node->vq->mutex); This seems to introduce a deadlock (and sleep-in-atomic): the vq->mutex is taken while the IOTLB spinlock is held (taken earlier in vhost_iotlb_notify_vq()). On the vhost_iotlb_miss() path, the IOTLB spinlock is taken while the vq->mutex is held. I'm not sure how to fix it. Given that we're holding dev->mutex, that vq->poll only seems to be modified under dev->mutex, and assuming that vhost_poll_queue(vq->poll) can be called concurrently, is it safe to simply not take vq->mutex here? Thanks, Jean > vhost_poll_queue(&node->vq->poll); > + mutex_unlock(&node->vq->mutex); > + > list_del(&node->node); > kfree(node); > } > @@ -986,7 +978,6 @@ static int vhost_process_iotlb_msg(struct vhost_dev *dev, > int ret = 0; > > mutex_lock(&dev->mutex); > - vhost_dev_lock_vqs(dev); > switch (msg->type) { > case VHOST_IOTLB_UPDATE: > if (!dev->iotlb) { > @@ -1020,7 +1011,6 @@ static int vhost_process_iotlb_msg(struct vhost_dev *dev, > break; > } > > - vhost_dev_unlock_vqs(dev); > mutex_unlock(&dev->mutex); > > return ret; >
On 2018/11/30 上午3:28, Jean-Philippe Brucker wrote: > Hi, > > On 25/09/2018 13:36,xiangxia.m.yue@gmail.com wrote: >> From: Tonghao Zhang<xiangxia.m.yue@gmail.com> >> >> This patch changes the way that lock all vqs >> at the same, to lock them one by one. It will >> be used for next patch to avoid the deadlock. >> >> Signed-off-by: Tonghao Zhang<xiangxia.m.yue@gmail.com> >> Acked-by: Jason Wang<jasowang@redhat.com> >> Signed-off-by: Jason Wang<jasowang@redhat.com> >> --- >> drivers/vhost/vhost.c | 24 +++++++----------------- >> 1 file changed, 7 insertions(+), 17 deletions(-) >> >> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c >> index b13c6b4..f52008b 100644 >> --- a/drivers/vhost/vhost.c >> +++ b/drivers/vhost/vhost.c >> @@ -294,8 +294,11 @@ static void vhost_vq_meta_reset(struct vhost_dev *d) >> { >> int i; >> >> - for (i = 0; i < d->nvqs; ++i) >> + for (i = 0; i < d->nvqs; ++i) { >> + mutex_lock(&d->vqs[i]->mutex); >> __vhost_vq_meta_reset(d->vqs[i]); >> + mutex_unlock(&d->vqs[i]->mutex); >> + } >> } >> >> static void vhost_vq_reset(struct vhost_dev *dev, >> @@ -891,20 +894,6 @@ static inline void __user *__vhost_get_user(struct vhost_virtqueue *vq, >> #define vhost_get_used(vq, x, ptr) \ >> vhost_get_user(vq, x, ptr, VHOST_ADDR_USED) >> >> -static void vhost_dev_lock_vqs(struct vhost_dev *d) >> -{ >> - int i = 0; >> - for (i = 0; i < d->nvqs; ++i) >> - mutex_lock_nested(&d->vqs[i]->mutex, i); >> -} >> - >> -static void vhost_dev_unlock_vqs(struct vhost_dev *d) >> -{ >> - int i = 0; >> - for (i = 0; i < d->nvqs; ++i) >> - mutex_unlock(&d->vqs[i]->mutex); >> -} >> - >> static int vhost_new_umem_range(struct vhost_umem *umem, >> u64 start, u64 size, u64 end, >> u64 userspace_addr, int perm) >> @@ -954,7 +943,10 @@ static void vhost_iotlb_notify_vq(struct vhost_dev *d, >> if (msg->iova <= vq_msg->iova && >> msg->iova + msg->size - 1 >= vq_msg->iova && >> vq_msg->type == VHOST_IOTLB_MISS) { >> + mutex_lock(&node->vq->mutex); > This seems to introduce a deadlock (and sleep-in-atomic): the vq->mutex > is taken while the IOTLB spinlock is held (taken earlier in > vhost_iotlb_notify_vq()). On the vhost_iotlb_miss() path, the IOTLB > spinlock is taken while the vq->mutex is held. Good catch. > I'm not sure how to fix it. Given that we're holding dev->mutex, that > vq->poll only seems to be modified under dev->mutex, and assuming that > vhost_poll_queue(vq->poll) can be called concurrently, is it safe to > simply not take vq->mutex here? Yes, I think it can be removed here. Want to post a patch for this? Thanks > Thanks, > Jean > >
On 30/11/2018 02:34, Jason Wang wrote: > > On 2018/11/30 上午3:28, Jean-Philippe Brucker wrote: >> Hi, >> >> On 25/09/2018 13:36,xiangxia.m.yue@gmail.com wrote: >>> From: Tonghao Zhang<xiangxia.m.yue@gmail.com> >>> >>> This patch changes the way that lock all vqs >>> at the same, to lock them one by one. It will >>> be used for next patch to avoid the deadlock. >>> >>> Signed-off-by: Tonghao Zhang<xiangxia.m.yue@gmail.com> >>> Acked-by: Jason Wang<jasowang@redhat.com> >>> Signed-off-by: Jason Wang<jasowang@redhat.com> >>> --- >>> drivers/vhost/vhost.c | 24 +++++++----------------- >>> 1 file changed, 7 insertions(+), 17 deletions(-) >>> >>> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c >>> index b13c6b4..f52008b 100644 >>> --- a/drivers/vhost/vhost.c >>> +++ b/drivers/vhost/vhost.c >>> @@ -294,8 +294,11 @@ static void vhost_vq_meta_reset(struct vhost_dev *d) >>> { >>> int i; >>> >>> - for (i = 0; i < d->nvqs; ++i) >>> + for (i = 0; i < d->nvqs; ++i) { >>> + mutex_lock(&d->vqs[i]->mutex); >>> __vhost_vq_meta_reset(d->vqs[i]); >>> + mutex_unlock(&d->vqs[i]->mutex); >>> + } >>> } >>> >>> static void vhost_vq_reset(struct vhost_dev *dev, >>> @@ -891,20 +894,6 @@ static inline void __user *__vhost_get_user(struct vhost_virtqueue *vq, >>> #define vhost_get_used(vq, x, ptr) \ >>> vhost_get_user(vq, x, ptr, VHOST_ADDR_USED) >>> >>> -static void vhost_dev_lock_vqs(struct vhost_dev *d) >>> -{ >>> - int i = 0; >>> - for (i = 0; i < d->nvqs; ++i) >>> - mutex_lock_nested(&d->vqs[i]->mutex, i); >>> -} >>> - >>> -static void vhost_dev_unlock_vqs(struct vhost_dev *d) >>> -{ >>> - int i = 0; >>> - for (i = 0; i < d->nvqs; ++i) >>> - mutex_unlock(&d->vqs[i]->mutex); >>> -} >>> - >>> static int vhost_new_umem_range(struct vhost_umem *umem, >>> u64 start, u64 size, u64 end, >>> u64 userspace_addr, int perm) >>> @@ -954,7 +943,10 @@ static void vhost_iotlb_notify_vq(struct vhost_dev *d, >>> if (msg->iova <= vq_msg->iova && >>> msg->iova + msg->size - 1 >= vq_msg->iova && >>> vq_msg->type == VHOST_IOTLB_MISS) { >>> + mutex_lock(&node->vq->mutex); >> This seems to introduce a deadlock (and sleep-in-atomic): the vq->mutex >> is taken while the IOTLB spinlock is held (taken earlier in >> vhost_iotlb_notify_vq()). On the vhost_iotlb_miss() path, the IOTLB >> spinlock is taken while the vq->mutex is held. > > > Good catch. > > >> I'm not sure how to fix it. Given that we're holding dev->mutex, that >> vq->poll only seems to be modified under dev->mutex, and assuming that >> vhost_poll_queue(vq->poll) can be called concurrently, is it safe to >> simply not take vq->mutex here? > > > Yes, I think it can be removed here. > > Want to post a patch for this? Yes, I'll post it shortly Thanks, Jean
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c index b13c6b4..f52008b 100644 --- a/drivers/vhost/vhost.c +++ b/drivers/vhost/vhost.c @@ -294,8 +294,11 @@ static void vhost_vq_meta_reset(struct vhost_dev *d) { int i; - for (i = 0; i < d->nvqs; ++i) + for (i = 0; i < d->nvqs; ++i) { + mutex_lock(&d->vqs[i]->mutex); __vhost_vq_meta_reset(d->vqs[i]); + mutex_unlock(&d->vqs[i]->mutex); + } } static void vhost_vq_reset(struct vhost_dev *dev, @@ -891,20 +894,6 @@ static inline void __user *__vhost_get_user(struct vhost_virtqueue *vq, #define vhost_get_used(vq, x, ptr) \ vhost_get_user(vq, x, ptr, VHOST_ADDR_USED) -static void vhost_dev_lock_vqs(struct vhost_dev *d) -{ - int i = 0; - for (i = 0; i < d->nvqs; ++i) - mutex_lock_nested(&d->vqs[i]->mutex, i); -} - -static void vhost_dev_unlock_vqs(struct vhost_dev *d) -{ - int i = 0; - for (i = 0; i < d->nvqs; ++i) - mutex_unlock(&d->vqs[i]->mutex); -} - static int vhost_new_umem_range(struct vhost_umem *umem, u64 start, u64 size, u64 end, u64 userspace_addr, int perm) @@ -954,7 +943,10 @@ static void vhost_iotlb_notify_vq(struct vhost_dev *d, if (msg->iova <= vq_msg->iova && msg->iova + msg->size - 1 >= vq_msg->iova && vq_msg->type == VHOST_IOTLB_MISS) { + mutex_lock(&node->vq->mutex); vhost_poll_queue(&node->vq->poll); + mutex_unlock(&node->vq->mutex); + list_del(&node->node); kfree(node); } @@ -986,7 +978,6 @@ static int vhost_process_iotlb_msg(struct vhost_dev *dev, int ret = 0; mutex_lock(&dev->mutex); - vhost_dev_lock_vqs(dev); switch (msg->type) { case VHOST_IOTLB_UPDATE: if (!dev->iotlb) { @@ -1020,7 +1011,6 @@ static int vhost_process_iotlb_msg(struct vhost_dev *dev, break; } - vhost_dev_unlock_vqs(dev); mutex_unlock(&dev->mutex); return ret;