From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:47165) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UWMva-0001WL-P6 for qemu-devel@nongnu.org; Sun, 28 Apr 2013 04:25:49 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UWMvY-0004iW-D8 for qemu-devel@nongnu.org; Sun, 28 Apr 2013 04:25:46 -0400 Received: from mx1.redhat.com ([209.132.183.28]:6570) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UWMvY-0004iS-5G for qemu-devel@nongnu.org; Sun, 28 Apr 2013 04:25:44 -0400 Date: Sun, 28 Apr 2013 11:25:34 +0300 From: "Michael S. Tsirkin" Message-ID: <20130428082534.GB7106@redhat.com> References: <1366972060-21606-1-git-send-email-jasowang@redhat.com> <20130426122628.GA15119@redhat.com> <517B5DF4.5000508@redhat.com> <20130427193239.GB30188@redhat.com> <517CD504.7060307@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <517CD504.7060307@redhat.com> Subject: Re: [Qemu-devel] [PATCH] virtio-net: properly check the vhost status during status set List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Jason Wang Cc: aliguori@us.ibm.com, qemu-devel@nongnu.org On Sun, Apr 28, 2013 at 03:51:32PM +0800, Jason Wang wrote: > On 04/28/2013 03:32 AM, Michael S. Tsirkin wrote: > > On Sat, Apr 27, 2013 at 01:11:16PM +0800, Jason Wang wrote: > >> On 04/26/2013 08:26 PM, Michael S. Tsirkin wrote: > >>> On Fri, Apr 26, 2013 at 06:27:40PM +0800, Jason Wang wrote: > >>>> Commit 32993698 (vhost: disable on tap link down) tries to disable the vhost > >>>> also when the peer's link is down. But the check was not done properly, the > >>>> vhost were only started when: > >>>> > >>>> 1) peer's link is not down > >>>> 2) virtio-net has already been started. > >>>> > >>>> Since == have a higher precedence than &&, place a brace to make sure both the > >>>> conditions were met then does the check. This fixes the crash when doing a savem > >>>> after set the link off which let qemu crash and complains: > >>>> > >>>> virtio_net_save: Assertion `!n->vhost_started' failed. > >>>> > >>>> Cc: Michael S. Tsirkin > >>>> Signed-off-by: Jason Wang > >>> Hmm okay, but now that I think about this, > >>> e.g. if link is up later, vhost will not be started. > >> If vm has been stopeed, and the link is up later, vhost won't be > >> started. this is expected. > >> If vm has been started, and the link is up later, since n->vhost_started > >> is false but both virtio_net_started() and !nc->peer->link_down is true, > >> so the vhost will be started. > >> > >> Looks ok? > > Let me clarify: virtio link is up but peer link is down. > > So guest will send packets. Will they never be > > completed? > > qemu_deliver_packet_iov() will assume the packet were sent when peer > link is down. So we are still ok? Right so I think userspace will start dropping packets. I think this is unnecessarily fragile, I think it's best to make sure vhost=on means userspace does not process tx/rx rings. > > > > > >>> So the correct thing is maybe to start vhost but use > >>> some backend that will drop all packets. > >>> And add a callback so we know peer state changed. > >>> Hmm do we need a kernel change for this? > >>> > >>>> --- > >>>> hw/net/virtio-net.c | 4 ++-- > >>>> 1 files changed, 2 insertions(+), 2 deletions(-) > >>>> > >>>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c > >>>> index 4d2cdd2..6222039 100644 > >>>> --- a/hw/net/virtio-net.c > >>>> +++ b/hw/net/virtio-net.c > >>>> @@ -114,8 +114,8 @@ static void virtio_net_vhost_status(VirtIONet *n, uint8_t status) > >>>> return; > >>>> } > >>>> > >>>> - if (!!n->vhost_started == virtio_net_started(n, status) && > >>>> - !nc->peer->link_down) { > >>>> + if (!!n->vhost_started == > >>>> + (virtio_net_started(n, status) && !nc->peer->link_down)) { > >>>> return; > >>>> } > >>>> if (!n->vhost_started) { > >>>> -- > >>>> 1.7.1