From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54458) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1c4poJ-00077N-Oh for qemu-devel@nongnu.org; Thu, 10 Nov 2016 08:54:37 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1c4poG-0000aL-Mx for qemu-devel@nongnu.org; Thu, 10 Nov 2016 08:54:35 -0500 Received: from mx1.redhat.com ([209.132.183.28]:45680) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1c4poG-0000ZI-EW for qemu-devel@nongnu.org; Thu, 10 Nov 2016 08:54:32 -0500 Date: Thu, 10 Nov 2016 15:54:30 +0200 From: "Michael S. Tsirkin" Message-ID: <20161110154953-mutt-send-email-mst@kernel.org> References: <1478704922-3400-1-git-send-email-yuri.benditovich@daynix.com> <1478704922-3400-4-git-send-email-yuri.benditovich@daynix.com> <20161109221300-mutt-send-email-mst@kernel.org> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v2 3/3] net: virtio-net discards TX data after link down List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Yuri Benditovich Cc: Jason Wang , qemu-devel@nongnu.org, Dmitry Fleytman , Yan Vugenfirer On Thu, Nov 10, 2016 at 01:56:05AM +0200, Yuri Benditovich wrote: >=20 >=20 > On Wed, Nov 9, 2016 at 10:28 PM, Michael S. Tsirkin wr= ote: >=20 > On Wed, Nov 09, 2016 at 05:22:02PM +0200, yuri.benditovich@daynix.c= om > wrote: > > From: Yuri Benditovich > > > > https://bugzilla.redhat.com/show_bug.cgi?id=3D1295637 > > Upon set_link monitor command or upon netdev deletion > > virtio-net sends link down indication to the guest > > and stops vhost if one is used. > > Guest driver can still submit data for TX until it > > recognizes link loss. If these packets not returned by > > the host, the Windows guest will never be able to finish > > disable/removal/shutdown. > > Now each packet sent by guest after NIC indicated link > > down will be completed immediately. > > > > Signed-off-by: Yuri Benditovich > > --- > >=A0 hw/net/virtio-net.c | 28 ++++++++++++++++++++++++++++ > >=A0 1 file changed, 28 insertions(+) > > > > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c > > index 06bfe4b..ab4e18a 100644 > > --- a/hw/net/virtio-net.c > > +++ b/hw/net/virtio-net.c > > @@ -218,6 +218,16 @@ static void virtio_net_vnet_endian_status(Vi= rtIONet > *n, uint8_t status) > >=A0 =A0 =A0 } > >=A0 } > > > > +static void virtio_net_drop_tx_queue_data(VirtIODevice *vdev, Vi= rtQueue > *vq) > > +{ > > +=A0 =A0 VirtQueueElement *elem; > > +=A0 =A0 while ((elem =3D virtqueue_pop(vq, sizeof(VirtQueueEleme= nt)))) { > > +=A0 =A0 =A0 =A0 virtqueue_push(vq, elem, 0); > > +=A0 =A0 =A0 =A0 virtio_notify(vdev, vq); > > +=A0 =A0 =A0 =A0 g_free(elem); > > +=A0 =A0 } > > +} > > + > >=A0 static void virtio_net_set_status(struct VirtIODevice *vdev, u= int8_t > status) > >=A0 { > >=A0 =A0 =A0 VirtIONet *n =3D VIRTIO_NET(vdev); >=20 > I don't like this part. This does too much queue parsing, > I would like to just copy head from avail to used ring. >=20 > For example, people want to support rings >1K in size. > Let's add bool virtqueue_drop(vq) and be done with it. > =20 >=20 > Please note that this code works only when link is down. > For me this was too complicated to write simpler procedure > with the same result. Yes - it's somewhat problematic and risky that we process the ring in qemu, but I don't see an easy way around that. But at least let's limit the processing and assumptions we make. >=20 > =20 > > @@ -262,6 +272,14 @@ static void virtio_net_set_status(struct > VirtIODevice *vdev, uint8_t status) > >=A0 =A0 =A0 =A0 =A0 =A0 =A0 } else { > >=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 qemu_bh_cancel(q->tx_bh); > >=A0 =A0 =A0 =A0 =A0 =A0 =A0 } > > +=A0 =A0 =A0 =A0 =A0 =A0 if ((n->status & VIRTIO_NET_S_LINK_UP) =3D= =3D 0 && > > +=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 (queue_status & VIRTIO_CONFIG_S_= DRIVER_OK)) { > > +=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 /* if tx is waiting we are likel= y have some packets in ... we likely have some ... > tx queue > > +=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0* and disabled notification *= / what does this refer to? > > +=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 q->tx_waiting =3D 0; > > +=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 virtio_queue_set_notification(q-= >tx_vq, 1); > > +=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 virtio_net_drop_tx_queue_data(vd= ev, q->tx_vq); > > +=A0 =A0 =A0 =A0 =A0 =A0 } > >=A0 =A0 =A0 =A0 =A0 } > >=A0 =A0 =A0 } > >=A0 } >=20 > OK but what if guest keeps sending packets? What will drop them? >=20 >=20 > This code fixes following problem in original code (example): > We are in vhost=3Doff and receive kick ->virtio_net_handle_tx_timer > -> tx_waiting=3D1, notification disabled, timer set > Now we receive link loss, cancel the timer and stay with packets in the= queue > and with > disabled notification. Nobody will return them. (easy to reproduce with= timer > set to 5ms) >=20 > Added code drops packets we already have and ensure we will report them > as completed to guest. If guest keeps sending packets, they will be dro= pped=A0 > in=A0virtio_net_handle_tx_timer and=A0in =A0virtio_net_handle_tx_bh (in= procedures > just below) > as we already with link down.=A0 Yes I get that. I'm just not 100% sure all paths have us listen on the ioeventfd and handle kicks without races - this was previously assumed not to matter. >=20 >=20 > > @@ -1319,6 +1337,11 @@ static void virtio_net_handle_tx_timer( > VirtIODevice *vdev, VirtQueue *vq) > >=A0 =A0 =A0 VirtIONet *n =3D VIRTIO_NET(vdev); > >=A0 =A0 =A0 VirtIONetQueue *q =3D &n->vqs[vq2q(virtio_get_queue_in= dex(vq))]; > > > > +=A0 =A0 if (unlikely((n->status & VIRTIO_NET_S_LINK_UP) =3D=3D 0= )) { > > +=A0 =A0 =A0 =A0 virtio_net_drop_tx_queue_data(vdev, vq); > > +=A0 =A0 =A0 =A0 return; > > +=A0 =A0 } > > + > >=A0 =A0 =A0 /* This happens when device was stopped but VCPU wasn'= t. */ > >=A0 =A0 =A0 if (!vdev->vm_running) { > >=A0 =A0 =A0 =A0 =A0 q->tx_waiting =3D 1; > > @@ -1345,6 +1368,11 @@ static void virtio_net_handle_tx_bh(VirtIO= Device > *vdev, VirtQueue *vq) > >=A0 =A0 =A0 VirtIONet *n =3D VIRTIO_NET(vdev); > >=A0 =A0 =A0 VirtIONetQueue *q =3D &n->vqs[vq2q(virtio_get_queue_in= dex(vq))]; > > > > +=A0 =A0 if (unlikely((n->status & VIRTIO_NET_S_LINK_UP) =3D=3D 0= )) { > > +=A0 =A0 =A0 =A0 virtio_net_drop_tx_queue_data(vdev, vq); > > +=A0 =A0 =A0 =A0 return; > > +=A0 =A0 } > > + > >=A0 =A0 =A0 if (unlikely(q->tx_waiting)) { > >=A0 =A0 =A0 =A0 =A0 return; > >=A0 =A0 =A0 } > > -- > > 1.9.1 >=20 >=20