From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jason Wang Subject: Re: [PATCH net-next v6 3/4] net: vhost: factor out busy polling logic to vhost_net_busy_poll() Date: Mon, 30 Jul 2018 11:16:59 +0800 Message-ID: References: <1532196242-2998-1-git-send-email-xiangxia.m.yue@gmail.com> <1532196242-2998-4-git-send-email-xiangxia.m.yue@gmail.com> <2b0efbf4-09e2-0ee9-091f-e2d9e10483a1@gmail.com> <14d01d2d-0eb8-172b-1c53-7dadc5fffbac@lab.ntt.co.jp> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Cc: toshiaki.makita1@gmail.com, mst@redhat.com, virtualization@lists.linux-foundation.org, Linux Kernel Network Developers To: Tonghao Zhang , makita.toshiaki@lab.ntt.co.jp Return-path: Received: from mx3-rdu2.redhat.com ([66.187.233.73]:41784 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1725775AbeG3Et7 (ORCPT ); Mon, 30 Jul 2018 00:49:59 -0400 In-Reply-To: Content-Language: en-US Sender: netdev-owner@vger.kernel.org List-ID: On 2018年07月24日 11:28, Tonghao Zhang wrote: > On Tue, Jul 24, 2018 at 10:53 AM Toshiaki Makita > wrote: >> On 2018/07/24 2:31, Tonghao Zhang wrote: >>> On Mon, Jul 23, 2018 at 10:20 PM Toshiaki Makita >>> wrote: >>>> On 18/07/23 (月) 21:43, Tonghao Zhang wrote: >>>>> On Mon, Jul 23, 2018 at 5:58 PM Toshiaki Makita >>>>> wrote: >>>>>> On 2018/07/22 3:04,xiangxia.m.yue@gmail.com wrote: >>>>>>> From: Tonghao Zhang >>>>>>> >>>>>>> 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 >>>>>>> --- >>>>>> ... >>>>>>> +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