[PATCH net-next v6 0/4] net: vhost: improve performance when enable busyloop

[PATCH net-next v6 0/4] net: vhost: improve performance when enable busyloop

[xiangxia.m.yue]

From: Tonghao Zhang <xiangxia.m.yue@gmail.com>

This patches improve the guest receive performance.
On the handle_tx side, we poll the sock receive queue
at the same time. handle_rx do that in the same way.

For more performance report, see patch 4.

v5->v6:
rebase the codes.

Tonghao Zhang (4):
  net: vhost: lock the vqs one by one
  net: vhost: replace magic number of lock annotation
  net: vhost: factor out busy polling logic to vhost_net_busy_poll()
  net: vhost: add rx busy polling in tx path

 drivers/vhost/net.c   | 148 +++++++++++++++++++++++++++++---------------------
 drivers/vhost/vhost.c |  24 +++-----
 2 files changed, 94 insertions(+), 78 deletions(-)

-- 
1.8.3.1

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

[xiangxia.m.yue]

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 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;
-- 
1.8.3.1

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

[Michael S. Tsirkin]

On Sat, Jul 21, 2018 at 11:03:59AM -0700, 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 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

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

[Tonghao Zhang]

On Sun, Jul 22, 2018 at 11:26 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Sat, Jul 21, 2018 at 11:03:59AM -0700, 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 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

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

[Jason Wang]

On 2018年07月25日 20:05, Tonghao Zhang wrote:
> On Sun, Jul 22, 2018 at 11:26 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>> On Sat, Jul 21, 2018 at 11:03:59AM -0700, 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 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

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

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

[Tonghao Zhang]

On Sun, Jul 22, 2018 at 11:26 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Sat, Jul 21, 2018 at 11:03:59AM -0700, 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 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

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

[Michael S. Tsirkin]

On Sat, Jul 21, 2018 at 11:03:59AM -0700, 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 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

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

[xiangxia.m.yue]

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 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;
-- 
1.8.3.1

[PATCH net-next v6 2/4] net: vhost: replace magic number of lock annotation

[xiangxia.m.yue]

From: Tonghao Zhang <xiangxia.m.yue@gmail.com>

Use the VHOST_NET_VQ_XXX as a subclass for mutex_lock_nested.

Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
Acked-by: Jason Wang <jasowang@redhat.com>
---
 drivers/vhost/net.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
index b224036..321264c 100644
--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c
@@ -490,7 +490,7 @@ static void handle_tx(struct vhost_net *net)
 	bool zcopy, zcopy_used;
 	int sent_pkts = 0;
 
-	mutex_lock(&vq->mutex);
+	mutex_lock_nested(&vq->mutex, VHOST_NET_VQ_TX);
 	sock = vq->private_data;
 	if (!sock)
 		goto out;
@@ -667,7 +667,7 @@ static int vhost_net_rx_peek_head_len(struct vhost_net *net, struct sock *sk,
 		/* Flush batched heads first */
 		vhost_rx_signal_used(rnvq);
 		/* Both tx vq and rx socket were polled here */
-		mutex_lock_nested(&tvq->mutex, 1);
+		mutex_lock_nested(&tvq->mutex, VHOST_NET_VQ_TX);
 		vhost_disable_notify(&net->dev, tvq);
 
 		preempt_disable();
@@ -809,7 +809,7 @@ static void handle_rx(struct vhost_net *net)
 	__virtio16 num_buffers;
 	int recv_pkts = 0;
 
-	mutex_lock_nested(&vq->mutex, 0);
+	mutex_lock_nested(&vq->mutex, VHOST_NET_VQ_RX);
 	sock = vq->private_data;
 	if (!sock)
 		goto out;
-- 
1.8.3.1

[PATCH net-next v6 2/4] net: vhost: replace magic number of lock annotation

[xiangxia.m.yue]

From: Tonghao Zhang <xiangxia.m.yue@gmail.com>

Use the VHOST_NET_VQ_XXX as a subclass for mutex_lock_nested.

Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
Acked-by: Jason Wang <jasowang@redhat.com>
---
 drivers/vhost/net.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
index b224036..321264c 100644
--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c
@@ -490,7 +490,7 @@ static void handle_tx(struct vhost_net *net)
 	bool zcopy, zcopy_used;
 	int sent_pkts = 0;
 
-	mutex_lock(&vq->mutex);
+	mutex_lock_nested(&vq->mutex, VHOST_NET_VQ_TX);
 	sock = vq->private_data;
 	if (!sock)
 		goto out;
@@ -667,7 +667,7 @@ static int vhost_net_rx_peek_head_len(struct vhost_net *net, struct sock *sk,
 		/* Flush batched heads first */
 		vhost_rx_signal_used(rnvq);
 		/* Both tx vq and rx socket were polled here */
-		mutex_lock_nested(&tvq->mutex, 1);
+		mutex_lock_nested(&tvq->mutex, VHOST_NET_VQ_TX);
 		vhost_disable_notify(&net->dev, tvq);
 
 		preempt_disable();
@@ -809,7 +809,7 @@ static void handle_rx(struct vhost_net *net)
 	__virtio16 num_buffers;
 	int recv_pkts = 0;
 
-	mutex_lock_nested(&vq->mutex, 0);
+	mutex_lock_nested(&vq->mutex, VHOST_NET_VQ_RX);
 	sock = vq->private_data;
 	if (!sock)
 		goto out;
-- 
1.8.3.1

[PATCH net-next v6 3/4] net: vhost: factor out busy polling logic to vhost_net_busy_poll()

[xiangxia.m.yue]

From: Tonghao Zhang <xiangxia.m.yue@gmail.com>

Factor out generic busy polling logic and will be
used for in tx path in the next patch. And with the patch,
qemu can set differently the busyloop_timeout for rx queue.

Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
---
 drivers/vhost/net.c | 114 ++++++++++++++++++++++++++++++++++------------------
 1 file changed, 74 insertions(+), 40 deletions(-)

diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
index 321264c..2dc937e 100644
--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c
@@ -428,6 +428,78 @@ static int vhost_net_enable_vq(struct vhost_net *n,
 	return vhost_poll_start(poll, sock->file);
 }
 
+static int sk_has_rx_data(struct sock *sk)
+{
+	struct socket *sock = sk->sk_socket;
+
+	if (sock->ops->peek_len)
+		return sock->ops->peek_len(sock);
+
+	return skb_queue_empty(&sk->sk_receive_queue);
+}
+
+static void vhost_net_busy_poll_vq_check(struct vhost_net *net,
+					 struct vhost_virtqueue *rvq,
+					 struct vhost_virtqueue *tvq,
+					 bool rx)
+{
+	struct socket *sock = rvq->private_data;
+
+	if (rx) {
+		if (!vhost_vq_avail_empty(&net->dev, tvq)) {
+			vhost_poll_queue(&tvq->poll);
+		} else if (unlikely(vhost_enable_notify(&net->dev, tvq))) {
+			vhost_disable_notify(&net->dev, tvq);
+			vhost_poll_queue(&tvq->poll);
+		}
+	} else if ((sock && sk_has_rx_data(sock->sk)) &&
+		    !vhost_vq_avail_empty(&net->dev, rvq)) {
+		vhost_poll_queue(&rvq->poll);
+	}
+}
+
+static void vhost_net_busy_poll(struct vhost_net *net,
+				struct vhost_virtqueue *rvq,
+				struct vhost_virtqueue *tvq,
+				bool *busyloop_intr,
+				bool rx)
+{
+	unsigned long busyloop_timeout;
+	unsigned long endtime;
+	struct socket *sock;
+	struct vhost_virtqueue *vq = rx ? tvq : rvq;
+
+	mutex_lock_nested(&vq->mutex, rx ? VHOST_NET_VQ_TX: VHOST_NET_VQ_RX);
+	vhost_disable_notify(&net->dev, vq);
+	sock = rvq->private_data;
+
+	busyloop_timeout = rx ? rvq->busyloop_timeout:
+				tvq->busyloop_timeout;
+
+	preempt_disable();
+	endtime = busy_clock() + busyloop_timeout;
+
+	while (vhost_can_busy_poll(endtime)) {
+		if (vhost_has_work(&net->dev)) {
+			*busyloop_intr = true;
+			break;
+		}
+
+		if ((sock && sk_has_rx_data(sock->sk) &&
+		     !vhost_vq_avail_empty(&net->dev, rvq)) ||
+		    !vhost_vq_avail_empty(&net->dev, tvq))
+			break;
+
+		cpu_relax();
+	}
+
+	preempt_enable();
+
+	vhost_net_busy_poll_vq_check(net, rvq, tvq, rx);
+
+	mutex_unlock(&vq->mutex);
+}
+
 static int vhost_net_tx_get_vq_desc(struct vhost_net *net,
 				    struct vhost_virtqueue *vq,
 				    struct iovec iov[], unsigned int iov_size,
@@ -631,16 +703,6 @@ static int peek_head_len(struct vhost_net_virtqueue *rvq, struct sock *sk)
 	return len;
 }
 
-static int sk_has_rx_data(struct sock *sk)
-{
-	struct socket *sock = sk->sk_socket;
-
-	if (sock->ops->peek_len)
-		return sock->ops->peek_len(sock);
-
-	return skb_queue_empty(&sk->sk_receive_queue);
-}
-
 static void vhost_rx_signal_used(struct vhost_net_virtqueue *nvq)
 {
 	struct vhost_virtqueue *vq = &nvq->vq;
@@ -660,41 +722,13 @@ static int vhost_net_rx_peek_head_len(struct vhost_net *net, struct sock *sk,
 	struct vhost_net_virtqueue *tnvq = &net->vqs[VHOST_NET_VQ_TX];
 	struct vhost_virtqueue *rvq = &rnvq->vq;
 	struct vhost_virtqueue *tvq = &tnvq->vq;
-	unsigned long uninitialized_var(endtime);
 	int len = peek_head_len(rnvq, sk);
 
-	if (!len && tvq->busyloop_timeout) {
+	if (!len && rvq->busyloop_timeout) {
 		/* Flush batched heads first */
 		vhost_rx_signal_used(rnvq);
 		/* Both tx vq and rx socket were polled here */
-		mutex_lock_nested(&tvq->mutex, VHOST_NET_VQ_TX);
-		vhost_disable_notify(&net->dev, tvq);
-
-		preempt_disable();
-		endtime = busy_clock() + tvq->busyloop_timeout;
-
-		while (vhost_can_busy_poll(endtime)) {
-			if (vhost_has_work(&net->dev)) {
-				*busyloop_intr = true;
-				break;
-			}
-			if ((sk_has_rx_data(sk) &&
-			     !vhost_vq_avail_empty(&net->dev, rvq)) ||
-			    !vhost_vq_avail_empty(&net->dev, tvq))
-				break;
-			cpu_relax();
-		}
-
-		preempt_enable();
-
-		if (!vhost_vq_avail_empty(&net->dev, tvq)) {
-			vhost_poll_queue(&tvq->poll);
-		} else if (unlikely(vhost_enable_notify(&net->dev, tvq))) {
-			vhost_disable_notify(&net->dev, tvq);
-			vhost_poll_queue(&tvq->poll);
-		}
-
-		mutex_unlock(&tvq->mutex);
+		vhost_net_busy_poll(net, rvq, tvq, busyloop_intr, true);
 
 		len = peek_head_len(rnvq, sk);
 	}
-- 
1.8.3.1

Re: [PATCH net-next v6 3/4] net: vhost: factor out busy polling logic to vhost_net_busy_poll()

[Toshiaki Makita]

On 2018/07/22 3:04, xiangxia.m.yue@gmail.com wrote:
> From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> 
> Factor out generic busy polling logic and will be
> used for in tx path in the next patch. And with the patch,
> qemu can set differently the busyloop_timeout for rx queue.
> 
> Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> ---
...
> +static void vhost_net_busy_poll_vq_check(struct vhost_net *net,
> +					 struct vhost_virtqueue *rvq,
> +					 struct vhost_virtqueue *tvq,
> +					 bool rx)
> +{
> +	struct socket *sock = rvq->private_data;
> +
> +	if (rx) {
> +		if (!vhost_vq_avail_empty(&net->dev, tvq)) {
> +			vhost_poll_queue(&tvq->poll);
> +		} else if (unlikely(vhost_enable_notify(&net->dev, tvq))) {
> +			vhost_disable_notify(&net->dev, tvq);
> +			vhost_poll_queue(&tvq->poll);
> +		}
> +	} else if ((sock && sk_has_rx_data(sock->sk)) &&
> +		    !vhost_vq_avail_empty(&net->dev, rvq)) {
> +		vhost_poll_queue(&rvq->poll);

Now we wait for vq_avail for rx as well, I think you cannot skip
vhost_enable_notify() on tx. Probably you might want to do:

} else if (sock && sk_has_rx_data(sock->sk)) {
	if (!vhost_vq_avail_empty(&net->dev, rvq)) {
		vhost_poll_queue(&rvq->poll);
	} else if (unlikely(vhost_enable_notify(&net->dev, rvq))) {
		vhost_disable_notify(&net->dev, rvq);
		vhost_poll_queue(&rvq->poll);
	}
}

Also it's better to care vhost_net_disable_vq()/vhost_net_enable_vq() on tx?

-- 
Toshiaki Makita

Re: [PATCH net-next v6 3/4] net: vhost: factor out busy polling logic to vhost_net_busy_poll()

[Tonghao Zhang]

On Mon, Jul 23, 2018 at 5:58 PM Toshiaki Makita
<makita.toshiaki@lab.ntt.co.jp> wrote:
>
> On 2018/07/22 3:04, xiangxia.m.yue@gmail.com wrote:
> > From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> >
> > Factor out generic busy polling logic and will be
> > used for in tx path in the next patch. And with the patch,
> > qemu can set differently the busyloop_timeout for rx queue.
> >
> > Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> > ---
> ...
> > +static void vhost_net_busy_poll_vq_check(struct vhost_net *net,
> > +                                      struct vhost_virtqueue *rvq,
> > +                                      struct vhost_virtqueue *tvq,
> > +                                      bool rx)
> > +{
> > +     struct socket *sock = rvq->private_data;
> > +
> > +     if (rx) {
> > +             if (!vhost_vq_avail_empty(&net->dev, tvq)) {
> > +                     vhost_poll_queue(&tvq->poll);
> > +             } else if (unlikely(vhost_enable_notify(&net->dev, tvq))) {
> > +                     vhost_disable_notify(&net->dev, tvq);
> > +                     vhost_poll_queue(&tvq->poll);
> > +             }
> > +     } else if ((sock && sk_has_rx_data(sock->sk)) &&
> > +                 !vhost_vq_avail_empty(&net->dev, rvq)) {
> > +             vhost_poll_queue(&rvq->poll);
>
> Now we wait for vq_avail for rx as well, I think you cannot skip
> vhost_enable_notify() on tx. Probably you might want to do:
I think vhost_enable_notify is needed.

> } else if (sock && sk_has_rx_data(sock->sk)) {
>         if (!vhost_vq_avail_empty(&net->dev, rvq)) {
>                 vhost_poll_queue(&rvq->poll);
>         } else if (unlikely(vhost_enable_notify(&net->dev, rvq))) {
>                 vhost_disable_notify(&net->dev, rvq);
>                 vhost_poll_queue(&rvq->poll);
>         }
> }
As Jason review as before, we only want rx kick when packet is pending at
socket but we're out of available buffers. So we just enable notify,
but not poll it ?

        } else if ((sock && sk_has_rx_data(sock->sk)) &&
                    !vhost_vq_avail_empty(&net->dev, rvq)) {
                vhost_poll_queue(&rvq->poll);
        else {
                vhost_enable_notify(&net->dev, rvq);
        }
> Also it's better to care vhost_net_disable_vq()/vhost_net_enable_vq() on tx?
I cant find why it is better, if necessary, we can do it.
> --
> Toshiaki Makita
>

Re: [PATCH net-next v6 3/4] net: vhost: factor out busy polling logic to vhost_net_busy_poll()

[Toshiaki Makita]

On 18/07/23 (月) 21:43, Tonghao Zhang wrote:
> On Mon, Jul 23, 2018 at 5:58 PM Toshiaki Makita
> <makita.toshiaki@lab.ntt.co.jp> wrote:
>>
>> On 2018/07/22 3:04, xiangxia.m.yue@gmail.com wrote:
>>> From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
>>>
>>> Factor out generic busy polling logic and will be
>>> used for in tx path in the next patch. And with the patch,
>>> qemu can set differently the busyloop_timeout for rx queue.
>>>
>>> Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
>>> ---
>> ...
>>> +static void vhost_net_busy_poll_vq_check(struct vhost_net *net,
>>> +                                      struct vhost_virtqueue *rvq,
>>> +                                      struct vhost_virtqueue *tvq,
>>> +                                      bool rx)
>>> +{
>>> +     struct socket *sock = rvq->private_data;
>>> +
>>> +     if (rx) {
>>> +             if (!vhost_vq_avail_empty(&net->dev, tvq)) {
>>> +                     vhost_poll_queue(&tvq->poll);
>>> +             } else if (unlikely(vhost_enable_notify(&net->dev, tvq))) {
>>> +                     vhost_disable_notify(&net->dev, tvq);
>>> +                     vhost_poll_queue(&tvq->poll);
>>> +             }
>>> +     } else if ((sock && sk_has_rx_data(sock->sk)) &&
>>> +                 !vhost_vq_avail_empty(&net->dev, rvq)) {
>>> +             vhost_poll_queue(&rvq->poll);
>>
>> Now we wait for vq_avail for rx as well, I think you cannot skip
>> vhost_enable_notify() on tx. Probably you might want to do:
> I think vhost_enable_notify is needed.
> 
>> } else if (sock && sk_has_rx_data(sock->sk)) {
>>          if (!vhost_vq_avail_empty(&net->dev, rvq)) {
>>                  vhost_poll_queue(&rvq->poll);
>>          } else if (unlikely(vhost_enable_notify(&net->dev, rvq))) {
>>                  vhost_disable_notify(&net->dev, rvq);
>>                  vhost_poll_queue(&rvq->poll);
>>          }
>> }
> As Jason review as before, we only want rx kick when packet is pending at
> socket but we're out of available buffers. So we just enable notify,
> but not poll it ?
> 
>          } else if ((sock && sk_has_rx_data(sock->sk)) &&
>                      !vhost_vq_avail_empty(&net->dev, rvq)) {
>                  vhost_poll_queue(&rvq->poll);
>          else {
>                  vhost_enable_notify(&net->dev, rvq);
>          }

When vhost_enable_notify() returns true the avail becomes non-empty 
while we are enabling notify. We may delay the rx process if we don't 
check the return value of vhost_enable_notify().

>> Also it's better to care vhost_net_disable_vq()/vhost_net_enable_vq() on tx?
> I cant find why it is better, if necessary, we can do it.

The reason is pretty simple... we are busypolling the socket so we don't 
need rx wakeups during it?

Re: [PATCH net-next v6 3/4] net: vhost: factor out busy polling logic to vhost_net_busy_poll()

[Tonghao Zhang]

On Mon, Jul 23, 2018 at 10:20 PM Toshiaki Makita
<toshiaki.makita1@gmail.com> wrote:
>
> On 18/07/23 (月) 21:43, Tonghao Zhang wrote:
> > On Mon, Jul 23, 2018 at 5:58 PM Toshiaki Makita
> > <makita.toshiaki@lab.ntt.co.jp> wrote:
> >>
> >> On 2018/07/22 3:04, xiangxia.m.yue@gmail.com wrote:
> >>> From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> >>>
> >>> Factor out generic busy polling logic and will be
> >>> used for in tx path in the next patch. And with the patch,
> >>> qemu can set differently the busyloop_timeout for rx queue.
> >>>
> >>> Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> >>> ---
> >> ...
> >>> +static void vhost_net_busy_poll_vq_check(struct vhost_net *net,
> >>> +                                      struct vhost_virtqueue *rvq,
> >>> +                                      struct vhost_virtqueue *tvq,
> >>> +                                      bool rx)
> >>> +{
> >>> +     struct socket *sock = rvq->private_data;
> >>> +
> >>> +     if (rx) {
> >>> +             if (!vhost_vq_avail_empty(&net->dev, tvq)) {
> >>> +                     vhost_poll_queue(&tvq->poll);
> >>> +             } else if (unlikely(vhost_enable_notify(&net->dev, tvq))) {
> >>> +                     vhost_disable_notify(&net->dev, tvq);
> >>> +                     vhost_poll_queue(&tvq->poll);
> >>> +             }
> >>> +     } else if ((sock && sk_has_rx_data(sock->sk)) &&
> >>> +                 !vhost_vq_avail_empty(&net->dev, rvq)) {
> >>> +             vhost_poll_queue(&rvq->poll);
> >>
> >> Now we wait for vq_avail for rx as well, I think you cannot skip
> >> vhost_enable_notify() on tx. Probably you might want to do:
> > I think vhost_enable_notify is needed.
> >
> >> } else if (sock && sk_has_rx_data(sock->sk)) {
> >>          if (!vhost_vq_avail_empty(&net->dev, rvq)) {
> >>                  vhost_poll_queue(&rvq->poll);
> >>          } else if (unlikely(vhost_enable_notify(&net->dev, rvq))) {
> >>                  vhost_disable_notify(&net->dev, rvq);
> >>                  vhost_poll_queue(&rvq->poll);
> >>          }
> >> }
> > As Jason review as before, we only want rx kick when packet is pending at
> > socket but we're out of available buffers. So we just enable notify,
> > but not poll it ?
> >
> >          } else if ((sock && sk_has_rx_data(sock->sk)) &&
> >                      !vhost_vq_avail_empty(&net->dev, rvq)) {
> >                  vhost_poll_queue(&rvq->poll);
> >          else {
> >                  vhost_enable_notify(&net->dev, rvq);
> >          }
>
> When vhost_enable_notify() returns true the avail becomes non-empty
> while we are enabling notify. We may delay the rx process if we don't
> check the return value of vhost_enable_notify().
I got it thanks.
> >> Also it's better to care vhost_net_disable_vq()/vhost_net_enable_vq() on tx?
> > I cant find why it is better, if necessary, we can do it.
>
> The reason is pretty simple... we are busypolling the socket so we don't
> need rx wakeups during it?
OK, but one question, how about rx? do we use the
vhost_net_disable_vq/vhost_net_ensable_vq on rx ?
> --
> Toshiaki Makita
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH net-next v6 3/4] net: vhost: factor out busy polling logic to vhost_net_busy_poll()

[Tonghao Zhang]

On Mon, Jul 23, 2018 at 10:20 PM Toshiaki Makita
<toshiaki.makita1@gmail.com> wrote:
>
> On 18/07/23 (月) 21:43, Tonghao Zhang wrote:
> > On Mon, Jul 23, 2018 at 5:58 PM Toshiaki Makita
> > <makita.toshiaki@lab.ntt.co.jp> wrote:
> >>
> >> On 2018/07/22 3:04, xiangxia.m.yue@gmail.com wrote:
> >>> From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> >>>
> >>> Factor out generic busy polling logic and will be
> >>> used for in tx path in the next patch. And with the patch,
> >>> qemu can set differently the busyloop_timeout for rx queue.
> >>>
> >>> Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> >>> ---
> >> ...
> >>> +static void vhost_net_busy_poll_vq_check(struct vhost_net *net,
> >>> +                                      struct vhost_virtqueue *rvq,
> >>> +                                      struct vhost_virtqueue *tvq,
> >>> +                                      bool rx)
> >>> +{
> >>> +     struct socket *sock = rvq->private_data;
> >>> +
> >>> +     if (rx) {
> >>> +             if (!vhost_vq_avail_empty(&net->dev, tvq)) {
> >>> +                     vhost_poll_queue(&tvq->poll);
> >>> +             } else if (unlikely(vhost_enable_notify(&net->dev, tvq))) {
> >>> +                     vhost_disable_notify(&net->dev, tvq);
> >>> +                     vhost_poll_queue(&tvq->poll);
> >>> +             }
> >>> +     } else if ((sock && sk_has_rx_data(sock->sk)) &&
> >>> +                 !vhost_vq_avail_empty(&net->dev, rvq)) {
> >>> +             vhost_poll_queue(&rvq->poll);
> >>
> >> Now we wait for vq_avail for rx as well, I think you cannot skip
> >> vhost_enable_notify() on tx. Probably you might want to do:
> > I think vhost_enable_notify is needed.
> >
> >> } else if (sock && sk_has_rx_data(sock->sk)) {
> >>          if (!vhost_vq_avail_empty(&net->dev, rvq)) {
> >>                  vhost_poll_queue(&rvq->poll);
> >>          } else if (unlikely(vhost_enable_notify(&net->dev, rvq))) {
> >>                  vhost_disable_notify(&net->dev, rvq);
> >>                  vhost_poll_queue(&rvq->poll);
> >>          }
> >> }
> > As Jason review as before, we only want rx kick when packet is pending at
> > socket but we're out of available buffers. So we just enable notify,
> > but not poll it ?
> >
> >          } else if ((sock && sk_has_rx_data(sock->sk)) &&
> >                      !vhost_vq_avail_empty(&net->dev, rvq)) {
> >                  vhost_poll_queue(&rvq->poll);
> >          else {
> >                  vhost_enable_notify(&net->dev, rvq);
> >          }
>
> When vhost_enable_notify() returns true the avail becomes non-empty
> while we are enabling notify. We may delay the rx process if we don't
> check the return value of vhost_enable_notify().
I got it thanks.
> >> Also it's better to care vhost_net_disable_vq()/vhost_net_enable_vq() on tx?
> > I cant find why it is better, if necessary, we can do it.
>
> The reason is pretty simple... we are busypolling the socket so we don't
> need rx wakeups during it?
OK, but one question, how about rx? do we use the
vhost_net_disable_vq/vhost_net_ensable_vq on rx ?
> --
> Toshiaki Makita

Re: [PATCH net-next v6 3/4] net: vhost: factor out busy polling logic to vhost_net_busy_poll()

[Toshiaki Makita]

On 2018/07/24 2:31, Tonghao Zhang wrote:
> On Mon, Jul 23, 2018 at 10:20 PM Toshiaki Makita
> <toshiaki.makita1@gmail.com> wrote:
>>
>> On 18/07/23 (月) 21:43, Tonghao Zhang wrote:
>>> On Mon, Jul 23, 2018 at 5:58 PM Toshiaki Makita
>>> <makita.toshiaki@lab.ntt.co.jp> wrote:
>>>>
>>>> On 2018/07/22 3:04, xiangxia.m.yue@gmail.com wrote:
>>>>> From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
>>>>>
>>>>> Factor out generic busy polling logic and will be
>>>>> used for in tx path in the next patch. And with the patch,
>>>>> qemu can set differently the busyloop_timeout for rx queue.
>>>>>
>>>>> Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
>>>>> ---
>>>> ...
>>>>> +static void vhost_net_busy_poll_vq_check(struct vhost_net *net,
>>>>> +                                      struct vhost_virtqueue *rvq,
>>>>> +                                      struct vhost_virtqueue *tvq,
>>>>> +                                      bool rx)
>>>>> +{
>>>>> +     struct socket *sock = rvq->private_data;
>>>>> +
>>>>> +     if (rx) {
>>>>> +             if (!vhost_vq_avail_empty(&net->dev, tvq)) {
>>>>> +                     vhost_poll_queue(&tvq->poll);
>>>>> +             } else if (unlikely(vhost_enable_notify(&net->dev, tvq))) {
>>>>> +                     vhost_disable_notify(&net->dev, tvq);
>>>>> +                     vhost_poll_queue(&tvq->poll);
>>>>> +             }
>>>>> +     } else if ((sock && sk_has_rx_data(sock->sk)) &&
>>>>> +                 !vhost_vq_avail_empty(&net->dev, rvq)) {
>>>>> +             vhost_poll_queue(&rvq->poll);
>>>>
>>>> Now we wait for vq_avail for rx as well, I think you cannot skip
>>>> vhost_enable_notify() on tx. Probably you might want to do:
>>> I think vhost_enable_notify is needed.
>>>
>>>> } else if (sock && sk_has_rx_data(sock->sk)) {
>>>>          if (!vhost_vq_avail_empty(&net->dev, rvq)) {
>>>>                  vhost_poll_queue(&rvq->poll);
>>>>          } else if (unlikely(vhost_enable_notify(&net->dev, rvq))) {
>>>>                  vhost_disable_notify(&net->dev, rvq);
>>>>                  vhost_poll_queue(&rvq->poll);
>>>>          }
>>>> }
>>> As Jason review as before, we only want rx kick when packet is pending at
>>> socket but we're out of available buffers. So we just enable notify,
>>> but not poll it ?
>>>
>>>          } else if ((sock && sk_has_rx_data(sock->sk)) &&
>>>                      !vhost_vq_avail_empty(&net->dev, rvq)) {
>>>                  vhost_poll_queue(&rvq->poll);
>>>          else {
>>>                  vhost_enable_notify(&net->dev, rvq);
>>>          }
>>
>> When vhost_enable_notify() returns true the avail becomes non-empty
>> while we are enabling notify. We may delay the rx process if we don't
>> check the return value of vhost_enable_notify().
> I got it thanks.
>>>> Also it's better to care vhost_net_disable_vq()/vhost_net_enable_vq() on tx?
>>> I cant find why it is better, if necessary, we can do it.
>>
>> The reason is pretty simple... we are busypolling the socket so we don't
>> need rx wakeups during it?
> OK, but one question, how about rx? do we use the
> vhost_net_disable_vq/vhost_net_ensable_vq on rx ?

If we are busypolling the sock tx buf? I'm not sure if polling it
improves the performance.

-- 
Toshiaki Makita

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH net-next v6 3/4] net: vhost: factor out busy polling logic to vhost_net_busy_poll()

[Tonghao Zhang]

On Tue, Jul 24, 2018 at 10:53 AM Toshiaki Makita
<makita.toshiaki@lab.ntt.co.jp> wrote:
>
> On 2018/07/24 2:31, Tonghao Zhang wrote:
> > On Mon, Jul 23, 2018 at 10:20 PM Toshiaki Makita
> > <toshiaki.makita1@gmail.com> wrote:
> >>
> >> On 18/07/23 (月) 21:43, Tonghao Zhang wrote:
> >>> On Mon, Jul 23, 2018 at 5:58 PM Toshiaki Makita
> >>> <makita.toshiaki@lab.ntt.co.jp> wrote:
> >>>>
> >>>> On 2018/07/22 3:04, xiangxia.m.yue@gmail.com wrote:
> >>>>> From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> >>>>>
> >>>>> Factor out generic busy polling logic and will be
> >>>>> used for in tx path in the next patch. And with the patch,
> >>>>> qemu can set differently the busyloop_timeout for rx queue.
> >>>>>
> >>>>> Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> >>>>> ---
> >>>> ...
> >>>>> +static void vhost_net_busy_poll_vq_check(struct vhost_net *net,
> >>>>> +                                      struct vhost_virtqueue *rvq,
> >>>>> +                                      struct vhost_virtqueue *tvq,
> >>>>> +                                      bool rx)
> >>>>> +{
> >>>>> +     struct socket *sock = rvq->private_data;
> >>>>> +
> >>>>> +     if (rx) {
> >>>>> +             if (!vhost_vq_avail_empty(&net->dev, tvq)) {
> >>>>> +                     vhost_poll_queue(&tvq->poll);
> >>>>> +             } else if (unlikely(vhost_enable_notify(&net->dev, tvq))) {
> >>>>> +                     vhost_disable_notify(&net->dev, tvq);
> >>>>> +                     vhost_poll_queue(&tvq->poll);
> >>>>> +             }
> >>>>> +     } else if ((sock && sk_has_rx_data(sock->sk)) &&
> >>>>> +                 !vhost_vq_avail_empty(&net->dev, rvq)) {
> >>>>> +             vhost_poll_queue(&rvq->poll);
> >>>>
> >>>> Now we wait for vq_avail for rx as well, I think you cannot skip
> >>>> vhost_enable_notify() on tx. Probably you might want to do:
> >>> I think vhost_enable_notify is needed.
> >>>
> >>>> } else if (sock && sk_has_rx_data(sock->sk)) {
> >>>>          if (!vhost_vq_avail_empty(&net->dev, rvq)) {
> >>>>                  vhost_poll_queue(&rvq->poll);
> >>>>          } else if (unlikely(vhost_enable_notify(&net->dev, rvq))) {
> >>>>                  vhost_disable_notify(&net->dev, rvq);
> >>>>                  vhost_poll_queue(&rvq->poll);
> >>>>          }
> >>>> }
> >>> As Jason review as before, we only want rx kick when packet is pending at
> >>> socket but we're out of available buffers. So we just enable notify,
> >>> but not poll it ?
> >>>
> >>>          } else if ((sock && sk_has_rx_data(sock->sk)) &&
> >>>                      !vhost_vq_avail_empty(&net->dev, rvq)) {
> >>>                  vhost_poll_queue(&rvq->poll);
> >>>          else {
> >>>                  vhost_enable_notify(&net->dev, rvq);
> >>>          }
> >>
> >> When vhost_enable_notify() returns true the avail becomes non-empty
> >> while we are enabling notify. We may delay the rx process if we don't
> >> check the return value of vhost_enable_notify().
> > I got it thanks.
> >>>> Also it's better to care vhost_net_disable_vq()/vhost_net_enable_vq() on tx?
> >>> I cant find why it is better, if necessary, we can do it.
> >>
> >> The reason is pretty simple... we are busypolling the socket so we don't
> >> need rx wakeups during it?
> > OK, but one question, how about rx? do we use the
> > vhost_net_disable_vq/vhost_net_ensable_vq on rx ?
>
> If we are busypolling the sock tx buf? I'm not sure if polling it
> improves the performance.
Not the sock tx buff, when we are busypolling in handle_rx, we will
check the tx vring via  vhost_vq_avail_empty.
So, should we the disable tvq, e.g. vhost_net_disable_vq(net, tvq)?> --
> Toshiaki Makita
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH net-next v6 3/4] net: vhost: factor out busy polling logic to vhost_net_busy_poll()

[Toshiaki Makita]

On 2018/07/24 12:28, Tonghao Zhang wrote:
> On Tue, Jul 24, 2018 at 10:53 AM Toshiaki Makita
> <makita.toshiaki@lab.ntt.co.jp> wrote:
>>
>> On 2018/07/24 2:31, Tonghao Zhang wrote:
>>> On Mon, Jul 23, 2018 at 10:20 PM Toshiaki Makita
>>> <toshiaki.makita1@gmail.com> wrote:
>>>>
>>>> On 18/07/23 (月) 21:43, Tonghao Zhang wrote:
>>>>> On Mon, Jul 23, 2018 at 5:58 PM Toshiaki Makita
>>>>> <makita.toshiaki@lab.ntt.co.jp> wrote:
>>>>>>
>>>>>> On 2018/07/22 3:04, xiangxia.m.yue@gmail.com wrote:
>>>>>>> From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
>>>>>>>
>>>>>>> Factor out generic busy polling logic and will be
>>>>>>> used for in tx path in the next patch. And with the patch,
>>>>>>> qemu can set differently the busyloop_timeout for rx queue.
>>>>>>>
>>>>>>> Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
>>>>>>> ---
>>>>>> ...
>>>>>>> +static void vhost_net_busy_poll_vq_check(struct vhost_net *net,
>>>>>>> +                                      struct vhost_virtqueue *rvq,
>>>>>>> +                                      struct vhost_virtqueue *tvq,
>>>>>>> +                                      bool rx)
>>>>>>> +{
>>>>>>> +     struct socket *sock = rvq->private_data;
>>>>>>> +
>>>>>>> +     if (rx) {
>>>>>>> +             if (!vhost_vq_avail_empty(&net->dev, tvq)) {
>>>>>>> +                     vhost_poll_queue(&tvq->poll);
>>>>>>> +             } else if (unlikely(vhost_enable_notify(&net->dev, tvq))) {
>>>>>>> +                     vhost_disable_notify(&net->dev, tvq);
>>>>>>> +                     vhost_poll_queue(&tvq->poll);
>>>>>>> +             }
>>>>>>> +     } else if ((sock && sk_has_rx_data(sock->sk)) &&
>>>>>>> +                 !vhost_vq_avail_empty(&net->dev, rvq)) {
>>>>>>> +             vhost_poll_queue(&rvq->poll);
>>>>>>
>>>>>> Now we wait for vq_avail for rx as well, I think you cannot skip
>>>>>> vhost_enable_notify() on tx. Probably you might want to do:
>>>>> I think vhost_enable_notify is needed.
>>>>>
>>>>>> } else if (sock && sk_has_rx_data(sock->sk)) {
>>>>>>          if (!vhost_vq_avail_empty(&net->dev, rvq)) {
>>>>>>                  vhost_poll_queue(&rvq->poll);
>>>>>>          } else if (unlikely(vhost_enable_notify(&net->dev, rvq))) {
>>>>>>                  vhost_disable_notify(&net->dev, rvq);
>>>>>>                  vhost_poll_queue(&rvq->poll);
>>>>>>          }
>>>>>> }
>>>>> As Jason review as before, we only want rx kick when packet is pending at
>>>>> socket but we're out of available buffers. So we just enable notify,
>>>>> but not poll it ?
>>>>>
>>>>>          } else if ((sock && sk_has_rx_data(sock->sk)) &&
>>>>>                      !vhost_vq_avail_empty(&net->dev, rvq)) {
>>>>>                  vhost_poll_queue(&rvq->poll);
>>>>>          else {
>>>>>                  vhost_enable_notify(&net->dev, rvq);
>>>>>          }
>>>>
>>>> When vhost_enable_notify() returns true the avail becomes non-empty
>>>> while we are enabling notify. We may delay the rx process if we don't
>>>> check the return value of vhost_enable_notify().
>>> I got it thanks.
>>>>>> Also it's better to care vhost_net_disable_vq()/vhost_net_enable_vq() on tx?
>>>>> I cant find why it is better, if necessary, we can do it.
>>>>
>>>> The reason is pretty simple... we are busypolling the socket so we don't
>>>> need rx wakeups during it?
>>> OK, but one question, how about rx? do we use the
>>> vhost_net_disable_vq/vhost_net_ensable_vq on rx ?
>>
>> If we are busypolling the sock tx buf? I'm not sure if polling it
>> improves the performance.
> Not the sock tx buff, when we are busypolling in handle_rx, we will
> check the tx vring via  vhost_vq_avail_empty.
> So, should we the disable tvq, e.g. vhost_net_disable_vq(net, tvq)?> --

When you want to stop vq kicks from the guest you should call
vhost_disable_notify() and when you want to stop vq wakeups from the
socket you should call vhost_net_disable_vq().

You are polling vq_avail so you want to stop vq kicks thus
vhost_disable_notify() is needed and it is already called.

-- 
Toshiaki Makita

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH net-next v6 3/4] net: vhost: factor out busy polling logic to vhost_net_busy_poll()

[Jason Wang]

On 2018年07月24日 11:28, Tonghao Zhang wrote:
> On Tue, Jul 24, 2018 at 10:53 AM Toshiaki Makita
> <makita.toshiaki@lab.ntt.co.jp>  wrote:
>> On 2018/07/24 2:31, Tonghao Zhang wrote:
>>> On Mon, Jul 23, 2018 at 10:20 PM Toshiaki Makita
>>> <toshiaki.makita1@gmail.com>  wrote:
>>>> On 18/07/23 (月) 21:43, Tonghao Zhang wrote:
>>>>> On Mon, Jul 23, 2018 at 5:58 PM Toshiaki Makita
>>>>> <makita.toshiaki@lab.ntt.co.jp>  wrote:
>>>>>> On 2018/07/22 3:04,xiangxia.m.yue@gmail.com  wrote:
>>>>>>> From: Tonghao Zhang<xiangxia.m.yue@gmail.com>
>>>>>>>
>>>>>>> Factor out generic busy polling logic and will be
>>>>>>> used for in tx path in the next patch. And with the patch,
>>>>>>> qemu can set differently the busyloop_timeout for rx queue.
>>>>>>>
>>>>>>> Signed-off-by: Tonghao Zhang<xiangxia.m.yue@gmail.com>
>>>>>>> ---
>>>>>> ...
>>>>>>> +static void vhost_net_busy_poll_vq_check(struct vhost_net *net,
>>>>>>> +                                      struct vhost_virtqueue *rvq,
>>>>>>> +                                      struct vhost_virtqueue *tvq,
>>>>>>> +                                      bool rx)
>>>>>>> +{
>>>>>>> +     struct socket *sock = rvq->private_data;
>>>>>>> +
>>>>>>> +     if (rx) {
>>>>>>> +             if (!vhost_vq_avail_empty(&net->dev, tvq)) {
>>>>>>> +                     vhost_poll_queue(&tvq->poll);
>>>>>>> +             } else if (unlikely(vhost_enable_notify(&net->dev, tvq))) {
>>>>>>> +                     vhost_disable_notify(&net->dev, tvq);
>>>>>>> +                     vhost_poll_queue(&tvq->poll);
>>>>>>> +             }
>>>>>>> +     } else if ((sock && sk_has_rx_data(sock->sk)) &&
>>>>>>> +                 !vhost_vq_avail_empty(&net->dev, rvq)) {
>>>>>>> +             vhost_poll_queue(&rvq->poll);
>>>>>> Now we wait for vq_avail for rx as well, I think you cannot skip
>>>>>> vhost_enable_notify() on tx. Probably you might want to do:
>>>>> I think vhost_enable_notify is needed.
>>>>>
>>>>>> } else if (sock && sk_has_rx_data(sock->sk)) {
>>>>>>           if (!vhost_vq_avail_empty(&net->dev, rvq)) {
>>>>>>                   vhost_poll_queue(&rvq->poll);
>>>>>>           } else if (unlikely(vhost_enable_notify(&net->dev, rvq))) {
>>>>>>                   vhost_disable_notify(&net->dev, rvq);
>>>>>>                   vhost_poll_queue(&rvq->poll);
>>>>>>           }
>>>>>> }
>>>>> As Jason review as before, we only want rx kick when packet is pending at
>>>>> socket but we're out of available buffers. So we just enable notify,
>>>>> but not poll it ?
>>>>>
>>>>>           } else if ((sock && sk_has_rx_data(sock->sk)) &&
>>>>>                       !vhost_vq_avail_empty(&net->dev, rvq)) {
>>>>>                   vhost_poll_queue(&rvq->poll);
>>>>>           else {
>>>>>                   vhost_enable_notify(&net->dev, rvq);
>>>>>           }
>>>> When vhost_enable_notify() returns true the avail becomes non-empty
>>>> while we are enabling notify. We may delay the rx process if we don't
>>>> check the return value of vhost_enable_notify().
>>> I got it thanks.
>>>>>> Also it's better to care vhost_net_disable_vq()/vhost_net_enable_vq() on tx?
>>>>> I cant find why it is better, if necessary, we can do it.
>>>> The reason is pretty simple... we are busypolling the socket so we don't
>>>> need rx wakeups during it?
>>> OK, but one question, how about rx? do we use the
>>> vhost_net_disable_vq/vhost_net_ensable_vq on rx ?
>> If we are busypolling the sock tx buf? I'm not sure if polling it
>> improves the performance.
> Not the sock tx buff, when we are busypolling in handle_rx, we will
> check the tx vring via  vhost_vq_avail_empty.
> So, should we the disable tvq, e.g. vhost_net_disable_vq(net, tvq)?> --

This could be done on top since tx wakeups only happnes when we run out 
of sndbuf.

Thanks

Re: [PATCH net-next v6 3/4] net: vhost: factor out busy polling logic to vhost_net_busy_poll()

[Jason Wang]

On 2018年07月24日 11:28, Tonghao Zhang wrote:
> On Tue, Jul 24, 2018 at 10:53 AM Toshiaki Makita
> <makita.toshiaki@lab.ntt.co.jp>  wrote:
>> On 2018/07/24 2:31, Tonghao Zhang wrote:
>>> On Mon, Jul 23, 2018 at 10:20 PM Toshiaki Makita
>>> <toshiaki.makita1@gmail.com>  wrote:
>>>> On 18/07/23 (月) 21:43, Tonghao Zhang wrote:
>>>>> On Mon, Jul 23, 2018 at 5:58 PM Toshiaki Makita
>>>>> <makita.toshiaki@lab.ntt.co.jp>  wrote:
>>>>>> On 2018/07/22 3:04,xiangxia.m.yue@gmail.com  wrote:
>>>>>>> From: Tonghao Zhang<xiangxia.m.yue@gmail.com>
>>>>>>>
>>>>>>> Factor out generic busy polling logic and will be
>>>>>>> used for in tx path in the next patch. And with the patch,
>>>>>>> qemu can set differently the busyloop_timeout for rx queue.
>>>>>>>
>>>>>>> Signed-off-by: Tonghao Zhang<xiangxia.m.yue@gmail.com>
>>>>>>> ---
>>>>>> ...
>>>>>>> +static void vhost_net_busy_poll_vq_check(struct vhost_net *net,
>>>>>>> +                                      struct vhost_virtqueue *rvq,
>>>>>>> +                                      struct vhost_virtqueue *tvq,
>>>>>>> +                                      bool rx)
>>>>>>> +{
>>>>>>> +     struct socket *sock = rvq->private_data;
>>>>>>> +
>>>>>>> +     if (rx) {
>>>>>>> +             if (!vhost_vq_avail_empty(&net->dev, tvq)) {
>>>>>>> +                     vhost_poll_queue(&tvq->poll);
>>>>>>> +             } else if (unlikely(vhost_enable_notify(&net->dev, tvq))) {
>>>>>>> +                     vhost_disable_notify(&net->dev, tvq);
>>>>>>> +                     vhost_poll_queue(&tvq->poll);
>>>>>>> +             }
>>>>>>> +     } else if ((sock && sk_has_rx_data(sock->sk)) &&
>>>>>>> +                 !vhost_vq_avail_empty(&net->dev, rvq)) {
>>>>>>> +             vhost_poll_queue(&rvq->poll);
>>>>>> Now we wait for vq_avail for rx as well, I think you cannot skip
>>>>>> vhost_enable_notify() on tx. Probably you might want to do:
>>>>> I think vhost_enable_notify is needed.
>>>>>
>>>>>> } else if (sock && sk_has_rx_data(sock->sk)) {
>>>>>>           if (!vhost_vq_avail_empty(&net->dev, rvq)) {
>>>>>>                   vhost_poll_queue(&rvq->poll);
>>>>>>           } else if (unlikely(vhost_enable_notify(&net->dev, rvq))) {
>>>>>>                   vhost_disable_notify(&net->dev, rvq);
>>>>>>                   vhost_poll_queue(&rvq->poll);
>>>>>>           }
>>>>>> }
>>>>> As Jason review as before, we only want rx kick when packet is pending at
>>>>> socket but we're out of available buffers. So we just enable notify,
>>>>> but not poll it ?
>>>>>
>>>>>           } else if ((sock && sk_has_rx_data(sock->sk)) &&
>>>>>                       !vhost_vq_avail_empty(&net->dev, rvq)) {
>>>>>                   vhost_poll_queue(&rvq->poll);
>>>>>           else {
>>>>>                   vhost_enable_notify(&net->dev, rvq);
>>>>>           }
>>>> When vhost_enable_notify() returns true the avail becomes non-empty
>>>> while we are enabling notify. We may delay the rx process if we don't
>>>> check the return value of vhost_enable_notify().
>>> I got it thanks.
>>>>>> Also it's better to care vhost_net_disable_vq()/vhost_net_enable_vq() on tx?
>>>>> I cant find why it is better, if necessary, we can do it.
>>>> The reason is pretty simple... we are busypolling the socket so we don't
>>>> need rx wakeups during it?
>>> OK, but one question, how about rx? do we use the
>>> vhost_net_disable_vq/vhost_net_ensable_vq on rx ?
>> If we are busypolling the sock tx buf? I'm not sure if polling it
>> improves the performance.
> Not the sock tx buff, when we are busypolling in handle_rx, we will
> check the tx vring via  vhost_vq_avail_empty.
> So, should we the disable tvq, e.g. vhost_net_disable_vq(net, tvq)?> --

This could be done on top since tx wakeups only happnes when we run out 
of sndbuf.

Thanks

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH net-next v6 3/4] net: vhost: factor out busy polling logic to vhost_net_busy_poll()

[Tonghao Zhang]

On Mon, Jul 23, 2018 at 5:58 PM Toshiaki Makita
<makita.toshiaki@lab.ntt.co.jp> wrote:
>
> On 2018/07/22 3:04, xiangxia.m.yue@gmail.com wrote:
> > From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> >
> > Factor out generic busy polling logic and will be
> > used for in tx path in the next patch. And with the patch,
> > qemu can set differently the busyloop_timeout for rx queue.
> >
> > Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> > ---
> ...
> > +static void vhost_net_busy_poll_vq_check(struct vhost_net *net,
> > +                                      struct vhost_virtqueue *rvq,
> > +                                      struct vhost_virtqueue *tvq,
> > +                                      bool rx)
> > +{
> > +     struct socket *sock = rvq->private_data;
> > +
> > +     if (rx) {
> > +             if (!vhost_vq_avail_empty(&net->dev, tvq)) {
> > +                     vhost_poll_queue(&tvq->poll);
> > +             } else if (unlikely(vhost_enable_notify(&net->dev, tvq))) {
> > +                     vhost_disable_notify(&net->dev, tvq);
> > +                     vhost_poll_queue(&tvq->poll);
> > +             }
> > +     } else if ((sock && sk_has_rx_data(sock->sk)) &&
> > +                 !vhost_vq_avail_empty(&net->dev, rvq)) {
> > +             vhost_poll_queue(&rvq->poll);
>
> Now we wait for vq_avail for rx as well, I think you cannot skip
> vhost_enable_notify() on tx. Probably you might want to do:
I think vhost_enable_notify is needed.

> } else if (sock && sk_has_rx_data(sock->sk)) {
>         if (!vhost_vq_avail_empty(&net->dev, rvq)) {
>                 vhost_poll_queue(&rvq->poll);
>         } else if (unlikely(vhost_enable_notify(&net->dev, rvq))) {
>                 vhost_disable_notify(&net->dev, rvq);
>                 vhost_poll_queue(&rvq->poll);
>         }
> }
As Jason review as before, we only want rx kick when packet is pending at
socket but we're out of available buffers. So we just enable notify,
but not poll it ?

        } else if ((sock && sk_has_rx_data(sock->sk)) &&
                    !vhost_vq_avail_empty(&net->dev, rvq)) {
                vhost_poll_queue(&rvq->poll);
        else {
                vhost_enable_notify(&net->dev, rvq);
        }
> Also it's better to care vhost_net_disable_vq()/vhost_net_enable_vq() on tx?
I cant find why it is better, if necessary, we can do it.
> --
> Toshiaki Makita
>

[PATCH net-next v6 3/4] net: vhost: factor out busy polling logic to vhost_net_busy_poll()

[xiangxia.m.yue]

From: Tonghao Zhang <xiangxia.m.yue@gmail.com>

Factor out generic busy polling logic and will be
used for in tx path in the next patch. And with the patch,
qemu can set differently the busyloop_timeout for rx queue.

Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
---
 drivers/vhost/net.c | 114 ++++++++++++++++++++++++++++++++++------------------
 1 file changed, 74 insertions(+), 40 deletions(-)

diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
index 321264c..2dc937e 100644
--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c
@@ -428,6 +428,78 @@ static int vhost_net_enable_vq(struct vhost_net *n,
 	return vhost_poll_start(poll, sock->file);
 }
 
+static int sk_has_rx_data(struct sock *sk)
+{
+	struct socket *sock = sk->sk_socket;
+
+	if (sock->ops->peek_len)
+		return sock->ops->peek_len(sock);
+
+	return skb_queue_empty(&sk->sk_receive_queue);
+}
+
+static void vhost_net_busy_poll_vq_check(struct vhost_net *net,
+					 struct vhost_virtqueue *rvq,
+					 struct vhost_virtqueue *tvq,
+					 bool rx)
+{
+	struct socket *sock = rvq->private_data;
+
+	if (rx) {
+		if (!vhost_vq_avail_empty(&net->dev, tvq)) {
+			vhost_poll_queue(&tvq->poll);
+		} else if (unlikely(vhost_enable_notify(&net->dev, tvq))) {
+			vhost_disable_notify(&net->dev, tvq);
+			vhost_poll_queue(&tvq->poll);
+		}
+	} else if ((sock && sk_has_rx_data(sock->sk)) &&
+		    !vhost_vq_avail_empty(&net->dev, rvq)) {
+		vhost_poll_queue(&rvq->poll);
+	}
+}
+
+static void vhost_net_busy_poll(struct vhost_net *net,
+				struct vhost_virtqueue *rvq,
+				struct vhost_virtqueue *tvq,
+				bool *busyloop_intr,
+				bool rx)
+{
+	unsigned long busyloop_timeout;
+	unsigned long endtime;
+	struct socket *sock;
+	struct vhost_virtqueue *vq = rx ? tvq : rvq;
+
+	mutex_lock_nested(&vq->mutex, rx ? VHOST_NET_VQ_TX: VHOST_NET_VQ_RX);
+	vhost_disable_notify(&net->dev, vq);
+	sock = rvq->private_data;
+
+	busyloop_timeout = rx ? rvq->busyloop_timeout:
+				tvq->busyloop_timeout;
+
+	preempt_disable();
+	endtime = busy_clock() + busyloop_timeout;
+
+	while (vhost_can_busy_poll(endtime)) {
+		if (vhost_has_work(&net->dev)) {
+			*busyloop_intr = true;
+			break;
+		}
+
+		if ((sock && sk_has_rx_data(sock->sk) &&
+		     !vhost_vq_avail_empty(&net->dev, rvq)) ||
+		    !vhost_vq_avail_empty(&net->dev, tvq))
+			break;
+
+		cpu_relax();
+	}
+
+	preempt_enable();
+
+	vhost_net_busy_poll_vq_check(net, rvq, tvq, rx);
+
+	mutex_unlock(&vq->mutex);
+}
+
 static int vhost_net_tx_get_vq_desc(struct vhost_net *net,
 				    struct vhost_virtqueue *vq,
 				    struct iovec iov[], unsigned int iov_size,
@@ -631,16 +703,6 @@ static int peek_head_len(struct vhost_net_virtqueue *rvq, struct sock *sk)
 	return len;
 }
 
-static int sk_has_rx_data(struct sock *sk)
-{
-	struct socket *sock = sk->sk_socket;
-
-	if (sock->ops->peek_len)
-		return sock->ops->peek_len(sock);
-
-	return skb_queue_empty(&sk->sk_receive_queue);
-}
-
 static void vhost_rx_signal_used(struct vhost_net_virtqueue *nvq)
 {
 	struct vhost_virtqueue *vq = &nvq->vq;
@@ -660,41 +722,13 @@ static int vhost_net_rx_peek_head_len(struct vhost_net *net, struct sock *sk,
 	struct vhost_net_virtqueue *tnvq = &net->vqs[VHOST_NET_VQ_TX];
 	struct vhost_virtqueue *rvq = &rnvq->vq;
 	struct vhost_virtqueue *tvq = &tnvq->vq;
-	unsigned long uninitialized_var(endtime);
 	int len = peek_head_len(rnvq, sk);
 
-	if (!len && tvq->busyloop_timeout) {
+	if (!len && rvq->busyloop_timeout) {
 		/* Flush batched heads first */
 		vhost_rx_signal_used(rnvq);
 		/* Both tx vq and rx socket were polled here */
-		mutex_lock_nested(&tvq->mutex, VHOST_NET_VQ_TX);
-		vhost_disable_notify(&net->dev, tvq);
-
-		preempt_disable();
-		endtime = busy_clock() + tvq->busyloop_timeout;
-
-		while (vhost_can_busy_poll(endtime)) {
-			if (vhost_has_work(&net->dev)) {
-				*busyloop_intr = true;
-				break;
-			}
-			if ((sk_has_rx_data(sk) &&
-			     !vhost_vq_avail_empty(&net->dev, rvq)) ||
-			    !vhost_vq_avail_empty(&net->dev, tvq))
-				break;
-			cpu_relax();
-		}
-
-		preempt_enable();
-
-		if (!vhost_vq_avail_empty(&net->dev, tvq)) {
-			vhost_poll_queue(&tvq->poll);
-		} else if (unlikely(vhost_enable_notify(&net->dev, tvq))) {
-			vhost_disable_notify(&net->dev, tvq);
-			vhost_poll_queue(&tvq->poll);
-		}
-
-		mutex_unlock(&tvq->mutex);
+		vhost_net_busy_poll(net, rvq, tvq, busyloop_intr, true);
 
 		len = peek_head_len(rnvq, sk);
 	}
-- 
1.8.3.1

[PATCH net-next v6 4/4] net: vhost: add rx busy polling in tx path

[xiangxia.m.yue]

From: Tonghao Zhang <xiangxia.m.yue@gmail.com>

This patch improves the guest receive performance.
On the handle_tx side, we poll the sock receive queue at the
same time. handle_rx do that in the same way.

We set the poll-us=100us and use the netperf to test throughput
and mean latency. When running the tests, the vhost-net kthread
of that VM, is alway 100% CPU. The commands are shown as below.

Rx performance is greatly improved by this patch. There is not
notable performance change on tx with this series though. This
patch is useful for bi-directional traffic.

netperf -H IP -t TCP_STREAM -l 20 -- -O "THROUGHPUT, THROUGHPUT_UNITS, MEAN_LATENCY"

Topology:
[Host] ->linux bridge -> tap vhost-net ->[Guest]

TCP_STREAM:
* Without the patch:  19842.95 Mbps, 6.50 us mean latency
* With the patch:     37373.10 Mbps, 3.43 us mean latency

Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
---
 drivers/vhost/net.c | 30 +++++++++++-------------------
 1 file changed, 11 insertions(+), 19 deletions(-)

diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
index 2dc937e..a562828 100644
--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c
@@ -501,29 +501,21 @@ static void vhost_net_busy_poll(struct vhost_net *net,
 }
 
 static int vhost_net_tx_get_vq_desc(struct vhost_net *net,
-				    struct vhost_virtqueue *vq,
+				    struct vhost_virtqueue *tvq,
 				    struct iovec iov[], unsigned int iov_size,
 				    unsigned int *out_num, unsigned int *in_num,
 				    bool *busyloop_intr)
 {
-	unsigned long uninitialized_var(endtime);
-	int r = vhost_get_vq_desc(vq, vq->iov, ARRAY_SIZE(vq->iov),
-				  out_num, in_num, NULL, NULL);
-
-	if (r == vq->num && vq->busyloop_timeout) {
-		preempt_disable();
-		endtime = busy_clock() + vq->busyloop_timeout;
-		while (vhost_can_busy_poll(endtime)) {
-			if (vhost_has_work(vq->dev)) {
-				*busyloop_intr = true;
-				break;
-			}
-			if (!vhost_vq_avail_empty(vq->dev, vq))
-				break;
-			cpu_relax();
-		}
-		preempt_enable();
-		r = vhost_get_vq_desc(vq, vq->iov, ARRAY_SIZE(vq->iov),
+	struct vhost_net_virtqueue *rnvq = &net->vqs[VHOST_NET_VQ_RX];
+	struct vhost_virtqueue *rvq = &rnvq->vq;
+
+	int r = vhost_get_vq_desc(tvq, tvq->iov, ARRAY_SIZE(tvq->iov),
+			          out_num, in_num, NULL, NULL);
+
+	if (r == tvq->num && tvq->busyloop_timeout) {
+		vhost_net_busy_poll(net, rvq, tvq, busyloop_intr, false);
+
+		r = vhost_get_vq_desc(tvq, tvq->iov, ARRAY_SIZE(tvq->iov),
 				      out_num, in_num, NULL, NULL);
 	}
 
-- 
1.8.3.1

[PATCH net-next v6 4/4] net: vhost: add rx busy polling in tx path

[xiangxia.m.yue]

From: Tonghao Zhang <xiangxia.m.yue@gmail.com>

This patch improves the guest receive performance.
On the handle_tx side, we poll the sock receive queue at the
same time. handle_rx do that in the same way.

We set the poll-us=100us and use the netperf to test throughput
and mean latency. When running the tests, the vhost-net kthread
of that VM, is alway 100% CPU. The commands are shown as below.

Rx performance is greatly improved by this patch. There is not
notable performance change on tx with this series though. This
patch is useful for bi-directional traffic.

netperf -H IP -t TCP_STREAM -l 20 -- -O "THROUGHPUT, THROUGHPUT_UNITS, MEAN_LATENCY"

Topology:
[Host] ->linux bridge -> tap vhost-net ->[Guest]

TCP_STREAM:
* Without the patch:  19842.95 Mbps, 6.50 us mean latency
* With the patch:     37373.10 Mbps, 3.43 us mean latency

Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
---
 drivers/vhost/net.c | 30 +++++++++++-------------------
 1 file changed, 11 insertions(+), 19 deletions(-)

diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
index 2dc937e..a562828 100644
--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c
@@ -501,29 +501,21 @@ static void vhost_net_busy_poll(struct vhost_net *net,
 }
 
 static int vhost_net_tx_get_vq_desc(struct vhost_net *net,
-				    struct vhost_virtqueue *vq,
+				    struct vhost_virtqueue *tvq,
 				    struct iovec iov[], unsigned int iov_size,
 				    unsigned int *out_num, unsigned int *in_num,
 				    bool *busyloop_intr)
 {
-	unsigned long uninitialized_var(endtime);
-	int r = vhost_get_vq_desc(vq, vq->iov, ARRAY_SIZE(vq->iov),
-				  out_num, in_num, NULL, NULL);
-
-	if (r == vq->num && vq->busyloop_timeout) {
-		preempt_disable();
-		endtime = busy_clock() + vq->busyloop_timeout;
-		while (vhost_can_busy_poll(endtime)) {
-			if (vhost_has_work(vq->dev)) {
-				*busyloop_intr = true;
-				break;
-			}
-			if (!vhost_vq_avail_empty(vq->dev, vq))
-				break;
-			cpu_relax();
-		}
-		preempt_enable();
-		r = vhost_get_vq_desc(vq, vq->iov, ARRAY_SIZE(vq->iov),
+	struct vhost_net_virtqueue *rnvq = &net->vqs[VHOST_NET_VQ_RX];
+	struct vhost_virtqueue *rvq = &rnvq->vq;
+
+	int r = vhost_get_vq_desc(tvq, tvq->iov, ARRAY_SIZE(tvq->iov),
+			          out_num, in_num, NULL, NULL);
+
+	if (r == tvq->num && tvq->busyloop_timeout) {
+		vhost_net_busy_poll(net, rvq, tvq, busyloop_intr, false);
+
+		r = vhost_get_vq_desc(tvq, tvq->iov, ARRAY_SIZE(tvq->iov),
 				      out_num, in_num, NULL, NULL);
 	}
 
-- 
1.8.3.1

Re: [PATCH net-next v6 0/4] net: vhost: improve performance when enable busyloop

[David Miller]

From: xiangxia.m.yue@gmail.com
Date: Sat, 21 Jul 2018 11:03:58 -0700

> From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> 
> This patches improve the guest receive performance.
> On the handle_tx side, we poll the sock receive queue
> at the same time. handle_rx do that in the same way.
> 
> For more performance report, see patch 4.
> 
> v5->v6:
> rebase the codes.

It looks like there is still some dangling discussions about this
patch set.

Please repost this series when those discussions have completed.

Thank you.

Re: [PATCH net-next v6 0/4] net: vhost: improve performance when enable busyloop

[David Miller]

From: xiangxia.m.yue@gmail.com
Date: Sat, 21 Jul 2018 11:03:58 -0700

> From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> 
> This patches improve the guest receive performance.
> On the handle_tx side, we poll the sock receive queue
> at the same time. handle_rx do that in the same way.
> 
> For more performance report, see patch 4.
> 
> v5->v6:
> rebase the codes.

It looks like there is still some dangling discussions about this
patch set.

Please repost this series when those discussions have completed.

Thank you.