netdev - Re: [PATCH] virtio_net: Fix queue full check

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <OF5977B5FB.669A3250-ON652577D6.00168221-652577D6.00180738@in.ibm.com>
Date:	Tue, 9 Nov 2010 09:56:03 +0530
From:	Krishna Kumar2 <krkumar2@...ibm.com>
To:	Rusty Russell <rusty@...tcorp.com.au>
Cc:	davem@...emloft.net, "Michael S. Tsirkin" <mst@...hat.com>,
	netdev@...r.kernel.org, yvugenfi@...hat.com
Subject: Re: [PATCH] virtio_net: Fix queue full check

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

--
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