From mboxrd@z Thu Jan  1 00:00:00 1970
From: Jason Wang <jasowang@redhat.com>
Subject: Re: [PATCH net-next v5 3/4] net: vhost: factor out busy polling logic
 to vhost_net_busy_poll()
Date: Wed, 4 Jul 2018 19:59:41 +0800
Message-ID: <e32caceb-f7f1-ee76-2b74-711e4f44e88f@redhat.com>
References: <1530678698-33427-1-git-send-email-xiangxia.m.yue@gmail.com>
 <1530678698-33427-4-git-send-email-xiangxia.m.yue@gmail.com>
 <808dea9b-6240-8055-acaa-a1b96389a673@lab.ntt.co.jp>
 <6ca28b13-637e-8650-30a4-bb3f2ea96852@redhat.com>
 <CAMDZJNXiD=ygWUaAEhEUDhSWZvhjfGR8NcttCjvorheBiKW4dg@mail.gmail.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 8bit
Cc: makita.toshiaki@lab.ntt.co.jp, mst@redhat.com,
        virtualization@lists.linux-foundation.org,
        Linux Kernel Network Developers <netdev@vger.kernel.org>,
        Tonghao Zhang <zhangtonghao@didichuxing.com>
To: Tonghao Zhang <xiangxia.m.yue@gmail.com>
Return-path: <netdev-owner@vger.kernel.org>
Received: from mx3-rdu2.redhat.com ([66.187.233.73]:37730 "EHLO mx1.redhat.com"
        rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP
        id S932733AbeGDL7w (ORCPT <rfc822;netdev@vger.kernel.org>);
        Wed, 4 Jul 2018 07:59:52 -0400
In-Reply-To: <CAMDZJNXiD=ygWUaAEhEUDhSWZvhjfGR8NcttCjvorheBiKW4dg@mail.gmail.com>
Content-Language: en-US
Sender: netdev-owner@vger.kernel.org
List-ID: <netdev.vger.kernel.org>



On 2018年07月04日 17:46, Tonghao Zhang wrote:
> On Wed, Jul 4, 2018 at 5:18 PM Jason Wang <jasowang@redhat.com> wrote:
>>
>>
>> On 2018年07月04日 15:59, Toshiaki Makita wrote:
>>> On 2018/07/04 13:31, xiangxia.m.yue@gmail.com wrote:
>>> ...
>>>> +static void vhost_net_busy_poll(struct vhost_net *net,
>>>> +                            struct vhost_virtqueue *rvq,
>>>> +                            struct vhost_virtqueue *tvq,
>>>> +                            bool rx)
>>>> +{
>>>> +    unsigned long uninitialized_var(endtime);
>>>> +    unsigned long busyloop_timeout;
>>>> +    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(tvq->dev, endtime) &&
>>>> +           !(sock && sk_has_rx_data(sock->sk)) &&
>>>> +           vhost_vq_avail_empty(tvq->dev, tvq))
>>>> +            cpu_relax();
>>>> +    preempt_enable();
>>>> +
>>>> +    if ((rx && !vhost_vq_avail_empty(&net->dev, vq)) ||
>>>> +        (!rx && (sock && sk_has_rx_data(sock->sk)))) {
>>>> +            vhost_poll_queue(&vq->poll);
>>>> +    } else if (vhost_enable_notify(&net->dev, vq) && rx) {
>>> Hmm... on tx here sock has no rx data, so you are waiting for sock
>>> wakeup for rx and vhost_enable_notify() seems not needed. Do you want
>>> this actually?
>>>
>>> } else if (rx && vhost_enable_notify(&net->dev, vq)) {
>> Right, rx need to be checked first here.
> thanks， if we dont call the vhost_enable_notify for tx. so we dont
> need to call vhost_disable_notify for tx?
>
> @@ -451,7 +451,9 @@ static void vhost_net_busy_poll(struct vhost_net *net,
>                                                tvq->busyloop_timeout;
>
>          mutex_lock_nested(&vq->mutex, rx ? VHOST_NET_VQ_TX: VHOST_NET_VQ_RX);
> -       vhost_disable_notify(&net->dev, vq);
> +
> +       if (rx)
> +               vhost_disable_notify(&net->dev, vq);
>
>          preempt_disable();
>          endtime = busy_clock() + busyloop_timeout;

Sorry for being unclear. We need enable tx notification at end end of 
the loop.

I meant we need replace

+    } else if (vhost_enable_notify(&net->dev, vq) && rx) {

with

+    } else if (rx && vhost_enable_notify(&net->dev, vq)) {

We only need rx notification when there's no avail buffers. This means 
we need only enable notification for tx.

And maybe we can simplify the logic as

if (rx) {
......
} else {
......
}

here. (not a must).

Thanks


>
>> Thanks
>>
>>>> +            vhost_disable_notify(&net->dev, vq);
>>>> +            vhost_poll_queue(&vq->poll);
>>>> +    }