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/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

On Tue, Jul 24, 2018 at 10:53 AM Toshiaki Makita
<makita.toshiaki at 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 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.
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
>

On 2018/07/24 12:28, Tonghao Zhang wrote:
> On Tue, Jul 24, 2018 at 10:53 AM Toshiaki Makita
> <makita.toshiaki at 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 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.
> 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

On 2018?07?24? 11:28, Tonghao Zhang wrote:
> On Tue, Jul 24, 2018 at 10:53 AM Toshiaki Makita
> <makita.toshiaki at 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 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.
> 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