lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Tue, 9 Nov 2010 21:00:58 +0530 From: Krishna Kumar2 <krkumar2@...ibm.com> To: "Michael S. Tsirkin" <mst@...hat.com> Cc: davem@...emloft.net, netdev@...r.kernel.org, Rusty Russell <rusty@...tcorp.com.au>, yvugenfi@...hat.com Subject: Re: [PATCH] virtio_net: Fix queue full check "Michael S. Tsirkin" <mst@...hat.com> 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 <rusty@...tcorp.com.au> 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" <mst@...hat.com> > > > > > > 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 <mst@...hat.com> > > > Signed-off-by: Rusty Russell <rusty@...tcorp.com.au> > > > Reported-By: Krishna Kumar2 <krkumar2@...ibm.com> > > > > 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 <mst@...hat.com> > > Signed-off-by: Rusty Russell <rusty@...tcorp.com.au> > > Reported-By: Krishna Kumar2 <krkumar2@...ibm.com> > > Tested-By: Krishna Kumar2 <krkumar2@...ibm.com> > > > > 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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@...r.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists