From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by dpdk.org (Postfix) with ESMTP id A87716949 for ; Mon, 9 May 2016 14:19:16 +0200 (CEST) Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 1D7EA80E4A; Mon, 9 May 2016 12:19:16 +0000 (UTC) Received: from redhat.com (ovpn-116-93.ams2.redhat.com [10.36.116.93]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with SMTP id u49CJEAl027804; Mon, 9 May 2016 08:19:14 -0400 Date: Mon, 9 May 2016 15:19:13 +0300 From: "Michael S. Tsirkin" To: Yuanhan Liu Cc: dev@dpdk.org, huawei.xie@intel.com Message-ID: <20160509151754-mutt-send-email-mst@redhat.com> References: <1462603224-29510-1-git-send-email-yuanhan.liu@linux.intel.com> <1462603224-29510-5-git-send-email-yuanhan.liu@linux.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1462603224-29510-5-git-send-email-yuanhan.liu@linux.intel.com> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.23 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.25]); Mon, 09 May 2016 12:19:16 +0000 (UTC) Subject: Re: [dpdk-dev] [PATCH 4/6] vhost: workaround stale vring base X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 09 May 2016 12:19:17 -0000 On Fri, May 06, 2016 at 11:40:22PM -0700, Yuanhan Liu wrote: > When DPDK app crashes (or quits, or gets killed), and when QEMU supports > reconnecting (patches have been sent, not merged yet), a restart of DPDK > app would get stale vring base from QEMU. That would break the kernel > virtio net completely, making it non-work any more, unless a driver reset > is done. > > So, instead of getting the stale vring base from QEMU, Huawei suggested > we could get a proper one from used->idx. > > Cc: "Michael S. Tsirkin" > Suggested-by: "Xie, Huawei" > Signed-off-by: Yuanhan Liu > --- > > Note that a right way to handle reconnect from abnormal quit would be > let the guest driver to initiate a reset when reconnect is detected > from QEMU. As a reset from the virtio net driver would resets all virtio > related meta datas, and trigger the vhost-user re-initiation again, > therefore, it would make the reconnect work as expected. > > What's unforunate is that driver reset on reconnect, as the term "driver" > implies, need driver hackings, which could be a linux kernel virtio net > driver, DPDK pmd driver, or even, the windows driver. Apparently, it will > not work so far, or even for a long while, until we are adapted to the > new kernel. > > The fix (or more precisely, the workaround) from this patch would make > reconnect work in most case, including the following two hypothesis that > might corrupt virtio memory: > > - vring_used_elem might be in stale state when we are in the middle of > updating used vring. Say, we might have updated the "id" field, but > leaving "len" untouched. > > - vring desc buff might also be in stale state, when we are copying > data into it. > > The reason it still works is that we haven't updated used->idx yet, > which means guest driver will not start processing those buggy used > entries. Therefore, no issues. > > However, Michael claims some concerns: he made a good point: a crash > is happening means some memory is corrupted, and it could be the virtio > memory being corrupted. In such case, nothing will work without the > reset. > > But I would say, that an app in product use would rare crash, and even > if it crashes, the chance that virtio memory being corrupted would be > relatively smaller then. Besides that, it would work, say when the app > is killed by ctrl-c or kill command. So, it works in the most cases. > But still, I would say it's just a workaround so far. > > On the other hand, without this patch, it would be 100% not working > from abnormal quit. But with this patch, it works in most cases, just > don't work in a rare case that when virtio memory is corrupted. Therefore, > I'd say, it is still good to have, until we have a perfect solution. > --- > lib/librte_vhost/virtio-net.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/lib/librte_vhost/virtio-net.c b/lib/librte_vhost/virtio-net.c > index c88aaa3..df103aa 100644 > --- a/lib/librte_vhost/virtio-net.c > +++ b/lib/librte_vhost/virtio-net.c > @@ -560,6 +560,14 @@ vhost_set_vring_addr(int vid, struct vhost_vring_addr *addr) > return -1; > } > > + if (vq->last_used_idx != vq->used->idx) { > + RTE_LOG(WARNING, VHOST_CONFIG, > + "last_used_idx (%u) and vq->used->idx (%u) mismatch\n", > + vq->last_used_idx, vq->used->idx); > + vq->last_used_idx = vq->used->idx; > + vq->last_used_idx_res = vq->used->idx; > + } > + > vq->log_guest_addr = addr->log_guest_addr; > > LOG_DEBUG(VHOST_CONFIG, "(%d) mapped address desc: %p\n", Acked-by: Michael S. Tsirkin And I would go further and just ignore the index coming from QEMU. dpdk processes packets in order so it does not need it. > -- > 1.9.0