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

On 2018/07/22 3:04, xiangxia.m.yue at gmail.com wrote:
> From: Tonghao Zhang <xiangxia.m.yue at 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 at 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

On Mon, Jul 23, 2018 at 5:58 PM Toshiaki Makita
<makita.toshiaki at lab.ntt.co.jp> wrote:
>
> On 2018/07/22 3:04, xiangxia.m.yue at gmail.com wrote:
> > From: Tonghao Zhang <xiangxia.m.yue at 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 at 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
>

On Mon, Jul 23, 2018 at 10:20 PM Toshiaki Makita
<toshiaki.makita1 at 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 at lab.ntt.co.jp> wrote:
> >>
> >> On 2018/07/22 3:04, xiangxia.m.yue at gmail.com wrote:
> >>> From: Tonghao Zhang <xiangxia.m.yue at 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 at
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

On 2018/07/24 2:31, Tonghao Zhang wrote:
> On Mon, Jul 23, 2018 at 10:20 PM Toshiaki Makita
> <toshiaki.makita1 at 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 at lab.ntt.co.jp> wrote:
>>>>
>>>> On 2018/07/22 3:04, xiangxia.m.yue at gmail.com wrote:
>>>>> From: Tonghao Zhang <xiangxia.m.yue at 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 at
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