From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Michael S. Tsirkin" Subject: Re: [PATCH] virtio_net: Fix queue full check Date: Tue, 9 Nov 2010 17:30:41 +0200 Message-ID: <20101109153041.GA26153@redhat.com> References: <20101028051036.25340.23442.sendpatchset@krkumar2.in.ibm.com> <20101102161730.GA32311@redhat.com> <20101104122424.GA29830@redhat.com> <201011080938.47938.rusty@rustcorp.com.au> <20101109131555.GE22705@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: davem@davemloft.net, netdev@vger.kernel.org, Rusty Russell , yvugenfi@redhat.com To: Krishna Kumar2 Return-path: Received: from mx1.redhat.com ([209.132.183.28]:50012 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751439Ab0KIPav (ORCPT ); Tue, 9 Nov 2010 10:30:51 -0500 Content-Disposition: inline In-Reply-To: Sender: netdev-owner@vger.kernel.org List-ID: On Tue, Nov 09, 2010 at 09:00:58PM +0530, Krishna Kumar2 wrote: > "Michael S. Tsirkin" wrote on 11/09/2010 06:45:55 PM: > > > Re: [PATCH] virtio_net: Fix queue full check > > > > On Tue, Nov 09, 2010 at 09:56:03AM +0530, Krishna Kumar2 wrote: > > > Rusty Russell wrote on 11/08/2010 04:38:47 AM: > > > > > > > Re: [PATCH] virtio_net: Fix queue full check > > > > > > > > On Thu, 4 Nov 2010 10:54:24 pm Michael S. Tsirkin wrote: > > > > > I thought about this some more. I think the original > > > > > code is actually correct in returning ENOSPC: indirect > > > > > buffers are nice, but it's a mistake > > > > > to rely on them as a memory allocation might fail. > > > > > > > > > > And if you look at virtio-net, it is dropping packets > > > > > under memory pressure which is not really a happy outcome: > > > > > the packet will get freed, reallocated and we get another one, > > > > > adding pressure on the allocator instead of releasing it > > > > > until we free up some buffers. > > > > > > > > > > So I now think we should calculate the capacity > > > > > assuming non-indirect entries, and if we manage to > > > > > use indirect, all the better. > > > > > > > > I've long said it's a weakness in the network stack that it insists > > > > drivers stop the tx queue before they *might* run out of room, > leading to > > > > worst-case assumptions and underutilization of the tx ring. > > > > > > > > However, I lost that debate, and so your patch is the way it's > supposed > > > to > > > > work. The other main indirect user (block) doesn't care as its queue > > > > allows for post-attempt blocking. > > > > > > > > I enhanced your commentry a little: > > > > > > > > Subject: virtio: return correct capacity to users > > > > Date: Thu, 4 Nov 2010 14:24:24 +0200 > > > > From: "Michael S. Tsirkin" > > > > > > > > We can't rely on indirect buffers for capacity > > > > calculations because they need a memory allocation > > > > which might fail. In particular, virtio_net can get > > > > into this situation under stress, and it drops packets > > > > and performs badly. > > > > > > > > So return the number of buffers we can guarantee users. > > > > > > > > Signed-off-by: Michael S. Tsirkin > > > > Signed-off-by: Rusty Russell > > > > Reported-By: Krishna Kumar2 > > > > > > I have tested this patch for 3-4 hours but so far I have not got the tx > > > full > > > error. I am not sure if "Tested-By" applies to this situation, but just > in > > > case: > > > > > > Signed-off-by: Michael S. Tsirkin > > > Signed-off-by: Rusty Russell > > > Reported-By: Krishna Kumar2 > > > Tested-By: Krishna Kumar2 > > > > > > I think both this patch and the original patch I submitted > > > are needed? That patch removes ENOMEM check and the increment > > > of dev->stats.tx_fifo_errors, and reports "memory failure". > > > > > > Thanks, > > > > > > - KK > > > > So I think your patch on top of this one would be wrong: > > we actually make sure out of memory does not affect TX path > > at all, so any error would be unexpected. > > > > Incrementing tx fifo errors is probably also helpful for debugging. > > > > If you like, we could kill the special handling for ENOMEM. > > Not sure whether it matters. > > Since that is dead code, we could remove it (and the fifo error > should disappear too - tx_dropped should be the only counter to > be incremented?). Sorry if I misunderstood something. > > Thanks, > > - KK It's just a sanity check. The capacity checking is tricky enough that I'm happier with some way to detect overflow both from ifconfig and dmesg. I don't really care which counter gets incremented really, since this is some TX bug fifo error seems appropriate but I don't really care much. -- MST