thr3ads.net - Linux Virtualization - [PATCH net-next v6 1/4] net: vhost: lock the vqs one by one [Jul 2018]

If this information is useful, please help other people find it:
Share via:

Michael S. Tsirkin

2018-Jul-22 15:26 UTC

[PATCH net-next v6 1/4] net: vhost: lock the vqs one by one

On Sat, Jul 21, 2018 at 11:03:59AM -0700, xiangxia.m.yue at gmail.com
wrote:> From: Tonghao Zhang <xiangxia.m.yue at 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 at gmail.com>
> Acked-by: Jason Wang <jasowang at redhat.com>
> Signed-off-by: Jason Wang <jasowang at 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 a502f1a..a1c06e7 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,
> @@ -890,20 +893,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)
> @@ -953,7 +942,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);
>  		}
> @@ -985,7 +977,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) {
> @@ -1019,7 +1010,6 @@ static int vhost_process_iotlb_msg(struct vhost_dev
*dev,
>  		break;
>  	}
>  
> -	vhost_dev_unlock_vqs(dev);
>  	mutex_unlock(&dev->mutex);
>  
>  	return ret;
I do prefer the finer-grained locking but I remember we
discussed something like this in the past and Jason saw issues
with such a locking.

Jason?
> -- 
> 1.8.3.1

Tonghao Zhang

2018-Jul-25 12:05 UTC

head link

[PATCH net-next v6 1/4] net: vhost: lock the vqs one by one

On Sun, Jul 22, 2018 at 11:26 PM Michael S. Tsirkin <mst at redhat.com>
wrote:>
> On Sat, Jul 21, 2018 at 11:03:59AM -0700, xiangxia.m.yue at gmail.com
wrote:
> > From: Tonghao Zhang <xiangxia.m.yue at 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 at gmail.com>
> > Acked-by: Jason Wang <jasowang at redhat.com>
> > Signed-off-by: Jason Wang <jasowang at 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 a502f1a..a1c06e7 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,
> > @@ -890,20 +893,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)
> > @@ -953,7 +942,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);
> >               }
> > @@ -985,7 +977,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) {
> > @@ -1019,7 +1010,6 @@ static int vhost_process_iotlb_msg(struct
vhost_dev *dev,
> >               break;
> >       }
> >
> > -     vhost_dev_unlock_vqs(dev);
> >       mutex_unlock(&dev->mutex);
> >
> >       return ret;
>
> I do prefer the finer-grained locking but I remember we
> discussed something like this in the past and Jason saw issues
> with such a locking.This change is suggested by Jason. Should I send new version because
the patch 3 is changed.
> Jason?
>
> > --
> > 1.8.3.1

Jason Wang

2018-Jul-30 02:54 UTC

head link

[PATCH net-next v6 1/4] net: vhost: lock the vqs one by one

On 2018?07?25? 20:05, Tonghao Zhang wrote:> On Sun, Jul 22, 2018 at 11:26 PM Michael S. Tsirkin <mst at
redhat.com> wrote:
>> On Sat, Jul 21, 2018 at 11:03:59AM -0700, xiangxia.m.yue at gmail.com
wrote:
>>> From: Tonghao Zhang <xiangxia.m.yue at 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 at gmail.com>
>>> Acked-by: Jason Wang <jasowang at redhat.com>
>>> Signed-off-by: Jason Wang <jasowang at 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 a502f1a..a1c06e7 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,
>>> @@ -890,20 +893,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)
>>> @@ -953,7 +942,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);
>>>                }
>>> @@ -985,7 +977,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) {
>>> @@ -1019,7 +1010,6 @@ static int vhost_process_iotlb_msg(struct
vhost_dev *dev,
>>>                break;
>>>        }
>>>
>>> -     vhost_dev_unlock_vqs(dev);
>>>        mutex_unlock(&dev->mutex);
>>>
>>>        return ret;
>> I do prefer the finer-grained locking but I remember we
>> discussed something like this in the past and Jason saw issues
>> with such a locking.
> This change is suggested by Jason. Should I send new version because
> the patch 3 is changed.
>
>> Jason?
Actually, the code was a little bit tricky here. Since it assumes 
handle_tx() and handle_rx() run on a single thread. Though the lock 
ordering is different, it was still safe.

Maybe we can add some comments to explain this.

Thanks
>>
>>> --
>>> 1.8.3.1

Possibly Parallel Threads

Search for more seemingly similar threads

Linux Virtualization - Jul 2018 - [PATCH net-next v6 1/4] net: vhost: lock the vqs one by one

[PATCH net-next v6 1/4] net: vhost: lock the vqs one by one

[PATCH net-next v6 1/4] net: vhost: lock the vqs one by one

[PATCH net-next v6 1/4] net: vhost: lock the vqs one by one

Possibly Parallel Threads