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

[Toshiaki Makita <toshiaki.makita1@gmail.com>]

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?