From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:49850) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UWNBc-0008FY-0t for qemu-devel@nongnu.org; Sun, 28 Apr 2013 04:42:20 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UWNBb-0001GQ-0d for qemu-devel@nongnu.org; Sun, 28 Apr 2013 04:42:19 -0400 Received: from mx1.redhat.com ([209.132.183.28]:55243) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UWNBa-0001GL-Ox for qemu-devel@nongnu.org; Sun, 28 Apr 2013 04:42:18 -0400 Message-ID: <517CE0DE.9080308@redhat.com> Date: Sun, 28 Apr 2013 16:42:06 +0800 From: Jason Wang MIME-Version: 1.0 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> <20130428082534.GB7106@redhat.com> In-Reply-To: <20130428082534.GB7106@redhat.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit 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: "Michael S. Tsirkin" Cc: aliguori@us.ibm.com, qemu-devel@nongnu.org On 04/28/2013 04:25 PM, Michael S. Tsirkin wrote: > 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. But this is also what other nic does such as e1000. Anyway it's harmless so we can just keep it. > >>> >>>>> 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