From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57407) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZCNxk-0003Ew-Gg for qemu-devel@nongnu.org; Tue, 07 Jul 2015 04:10:45 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZCNxg-0002sW-9e for qemu-devel@nongnu.org; Tue, 07 Jul 2015 04:10:44 -0400 Received: from mx1.redhat.com ([209.132.183.28]:45172) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZCNxg-0002sS-23 for qemu-devel@nongnu.org; Tue, 07 Jul 2015 04:10:40 -0400 Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) by mx1.redhat.com (Postfix) with ESMTPS id B7235B6681 for ; Tue, 7 Jul 2015 08:10:39 +0000 (UTC) Date: Tue, 7 Jul 2015 11:10:37 +0300 From: "Michael S. Tsirkin" Message-ID: <20150707110949-mutt-send-email-mst@redhat.com> References: <1435633611-14023-1-git-send-email-famz@redhat.com> <559254CC.4000106@redhat.com> <20150702124626.GE21214@stefanha-thinkpad.home> <5599F6C9.7070603@redhat.com> <20150706152116.GA11925@stefanha-thinkpad.redhat.com> <20150706200525-mutt-send-email-mst@redhat.com> <20150707005359.GB23721@ad.nay.redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20150707005359.GB23721@ad.nay.redhat.com> Subject: Re: [Qemu-devel] [PATCH] virtio-net: Drop net_virtio_info.can_receive List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Fam Zheng Cc: Jason Wang , qemu-devel@nongnu.org, Stefan Hajnoczi On Tue, Jul 07, 2015 at 08:53:59AM +0800, Fam Zheng wrote: > On Mon, 07/06 20:09, Michael S. Tsirkin wrote: > > On Mon, Jul 06, 2015 at 04:21:16PM +0100, Stefan Hajnoczi wrote: > > > On Mon, Jul 06, 2015 at 11:32:25AM +0800, Jason Wang wrote: > > > > > > > > > > > > On 07/02/2015 08:46 PM, Stefan Hajnoczi wrote: > > > > > On Tue, Jun 30, 2015 at 04:35:24PM +0800, Jason Wang wrote: > > > > >> On 06/30/2015 11:06 AM, Fam Zheng wrote: > > > > >>> virtio_net_receive still does the check by calling > > > > >>> virtio_net_can_receive, if the device or driver is not ready, the packet > > > > >>> is dropped. > > > > >>> > > > > >>> This is necessary because returning false from can_receive complicates > > > > >>> things: the peer would disable sending until we explicitly flush the > > > > >>> queue. > > > > >>> > > > > >>> Signed-off-by: Fam Zheng > > > > >>> --- > > > > >>> hw/net/virtio-net.c | 1 - > > > > >>> 1 file changed, 1 deletion(-) > > > > >>> > > > > >>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c > > > > >>> index d728233..dbef0d0 100644 > > > > >>> --- a/hw/net/virtio-net.c > > > > >>> +++ b/hw/net/virtio-net.c > > > > >>> @@ -1503,7 +1503,6 @@ static int virtio_net_load_device(VirtIODevice *vdev, QEMUFile *f, > > > > >>> static NetClientInfo net_virtio_info = { > > > > >>> .type = NET_CLIENT_OPTIONS_KIND_NIC, > > > > >>> .size = sizeof(NICState), > > > > >>> - .can_receive = virtio_net_can_receive, > > > > >>> .receive = virtio_net_receive, > > > > >>> .link_status_changed = virtio_net_set_link_status, > > > > >>> .query_rx_filter = virtio_net_query_rxfilter, > > > > >> A side effect of this patch is it will read and then drop packet is > > > > >> guest driver is no ok. > > > > > I think that the semantics of .can_receive() and .receive() return > > > > > values are currently incorrect in many NICs. They have .can_receive() > > > > > functions that return false for conditions where .receive() would > > > > > discard the packet. So what happens is that packets get queued when > > > > > they should actually be discarded. > > > > > > > > Yes, but they are bugs more or less. > > > > > > > > > > > > > > The purpose of the flow control (queuing) mechanism is to tell the > > > > > sender to hold off until the receiver has more rx buffers available. > > > > > It's a short-term thing that doesn't included link down, rx disable, or > > > > > NIC reset states. > > > > > > > > > > Therefore, I think this patch will not introduce a regression. It is > > > > > adjusting the code to stop queuing packets when they should actually be > > > > > dropped. > > > > > > > > > > Thoughts? > > > > > > > > I agree there's no functional issue. But it cause wasting of cpu cycles > > > > (consider guest is being flooded). Sometime it maybe even dangerous. For > > > > tap, we're probably ok since we have 756ae78b but for other backend, we > > > > don't. > > > > > > If the guest uses iptables rules or other mechanisms to drop bogus > > > packets the cost is even higher than discarding them at the QEMU layer. > > > > > > What's more is that if you're using link down as a DoS mitigation > > > strategy then you might as well hot unplug the NIC. > > > > > > Stefan > > > > > > > > Frankly, I don't see the point of the patch. Is this supposed to be a > > bugfix? If so, there's should be a description about how to trigger the > > bug. Is this an optimization? If so there should be some numbers > > showing a gain. > > It's a bug fix, we are not flushing the queue when DIRVER_OK is being set or > when buffer is becoming available (the virtio_net_can_receive conditions). Not > an issue before a90a7425cf but since that the semantics is enforced. > > Fam I think the safest and obvious fix is to flush on DRIVER_OK then (unless vhost started). That might be 2.4 material. -- MST