From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tonghao Zhang Subject: Re: [PATCH net-next v7 3/4] net: vhost: factor out busy polling logic to vhost_net_busy_poll() Date: Fri, 3 Aug 2018 11:24:12 +0800 Message-ID: References: <1533092454-37196-1-git-send-email-xiangxia.m.yue@gmail.com> <1533092454-37196-4-git-send-email-xiangxia.m.yue@gmail.com> <30e62749-3cbd-ae88-6582-c20087884b20@redhat.com> <3272c3b4-a44c-8554-329e-8a5e1a59aafd@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Cc: makita.toshiaki@lab.ntt.co.jp, mst@redhat.com, virtualization@lists.linux-foundation.org, Linux Kernel Network Developers To: jasowang@redhat.com Return-path: Received: from mail-oi0-f65.google.com ([209.85.218.65]:36268 "EHLO mail-oi0-f65.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725972AbeHCFSg (ORCPT ); Fri, 3 Aug 2018 01:18:36 -0400 Received: by mail-oi0-f65.google.com with SMTP id n21-v6so7454002oig.3 for ; Thu, 02 Aug 2018 20:24:24 -0700 (PDT) In-Reply-To: Sender: netdev-owner@vger.kernel.org List-ID: On Fri, Aug 3, 2018 at 11:07 AM Jason Wang wrote: > > > > On 2018=E5=B9=B408=E6=9C=8803=E6=97=A5 10:51, Tonghao Zhang wrote: > > On Thu, Aug 2, 2018 at 5:23 PM Jason Wang wrote: > >> > >> > >> On 2018=E5=B9=B408=E6=9C=8802=E6=97=A5 16:41, Toshiaki Makita wrote: > >>> On 2018/08/02 17:18, Jason Wang wrote: > >>>> On 2018=E5=B9=B408=E6=9C=8801=E6=97=A5 17:52, Tonghao Zhang wrote: > >>>>>> +static void vhost_net_busy_poll_check(struct vhost_net *net, > >>>>>> + struct vhost_virtqueue *rvq, > >>>>>> + struct vhost_virtqueue *tvq, > >>>>>> + bool rx) > >>>>>> +{ > >>>>>> + struct socket *sock =3D rvq->private_data; > >>>>>> + > >>>>>> + if (rx) > >>>>>> + vhost_net_busy_poll_try_queue(net, tvq); > >>>>>> + else if (sock && sk_has_rx_data(sock->sk)) > >>>>>> + vhost_net_busy_poll_try_queue(net, rvq); > >>>>>> + else { > >>>>>> + /* On tx here, sock has no rx data, so we > >>>>>> + * will wait for sock wakeup for rx, and > >>>>>> + * vhost_enable_notify() is not needed. */ > >>>>> A possible case is we do have rx data but guest does not refill the= rx > >>>>> queue. In this case we may lose notifications from guest. > >>>> Yes, should consider this case. thanks. > >>> I'm a bit confused. Isn't this covered by the previous > >>> "else if (sock && sk_has_rx_data(...))" block? > >> The problem is it does nothing if vhost_vq_avail_empty() is true and > >> vhost_enble_notify() is false. > >> > >>>>>>> + > >>>>>>> + cpu_relax(); > >>>>>>> + } > >>>>>>> + > >>>>>>> + preempt_enable(); > >>>>>>> + > >>>>>>> + if (!rx) > >>>>>>> + vhost_net_enable_vq(net, vq); > >>>>>> No need to enable rx virtqueue, if we are sure handle_rx() will be > >>>>>> called soon. > >>>>> If we disable rx virtqueue in handle_tx and don't send packets from > >>>>> guest anymore(handle_tx is not called), so we can wake up for sock = rx. > >>>>> so the network is broken. > >>>> Not sure I understand here. I mean is we schedule work for handle_rx= (), > >>>> there's no need to enable it since handle_rx() will do this for us. > >>> Looks like in the last "else" block in vhost_net_busy_poll_check() we > >>> need to enable vq since in that case we have no rx data and handle_rx= () > >>> is not scheduled. > >>> > >> Yes. > > So we will use the vhost_has_work() to check whether or not the > > handle_rx is scheduled ? > > If we use the vhost_has_work(), the work in the dev work_list may be > > rx work, or tx work, right ? > > Yes. We can add a boolean to record whether or not we've called > vhost_poll_queue() for rvq. And avoid calling vhost_net_enable_vq() if > it was true. so, the commit be294a51a "vhost_net: Avoid rx queue wake-ups during busypol= l" may not consider the case: work is tx work in the dev work list. > So here's the needed changes: > > 1) Split the wakeup disabling to another patch > 2) Squash the vhost_net_busy_poll_try_queue() and > vhost_net_busy_poll_check() into vhost_net_busy_poll() and reduce > duplicated checks. > 3) If possible, rename the boolean rx in vhost_net_busy_poll() to > poll_rx, this makes code more readable. OK > Thanks > > >> Thanks >