netdev - Re: [PATCH net-next v6 3/4] net: vhost: factor out busy polling logic to vhost_net_busy

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <2b0efbf4-09e2-0ee9-091f-e2d9e10483a1@gmail.com>
Date:   Mon, 23 Jul 2018 23:20:54 +0900
From:   Toshiaki Makita <toshiaki.makita1@...il.com>
To:     Tonghao Zhang <xiangxia.m.yue@...il.com>,
        makita.toshiaki@....ntt.co.jp
Cc:     jasowang@...hat.com, mst@...hat.com,
        virtualization@...ts.linux-foundation.org,
        Linux Kernel Network Developers <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next v6 3/4] net: vhost: factor out busy polling logic
 to vhost_net_busy_poll()

On 18/07/23 (月) 21:43, Tonghao Zhang wrote:
> On Mon, Jul 23, 2018 at 5:58 PM Toshiaki Makita
> <makita.toshiaki@....ntt.co.jp> wrote:
>>
>> On 2018/07/22 3:04, xiangxia.m.yue@...il.com wrote:
>>> From: Tonghao Zhang <xiangxia.m.yue@...il.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@...il.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?

--
Toshiaki Makita