Linux Virtualization - Nov 2013 - [PATCH net] virtio-net: fix page refcnt leaking when fail to allocate frag skb

On Wed, Nov 20, 2013 at 07:16:33AM -0800, Eric Dumazet
wrote:
> On Wed, 2013-11-20 at 10:58 +0200, Michael S. Tsirkin wrote:
> > On Tue, Nov 19, 2013 at 02:00:11PM -0800, Eric Dumazet wrote:
> > > On Tue, 2013-11-19 at 23:53 +0200, Michael S. Tsirkin wrote:
> > > 
> > > > Which NIC? Virtio? Prior to
2613af0ed18a11d5c566a81f9a6510b73180660a
> > > > it didn't drop packets received from host as far as I
can tell.
> > > > virtio is more like a pipe than a real NIC in this respect.
> > > 
> > > Prior/after to this patch, you were not posting buffers, so if
packets
> > > were received on a physical NIC, you were dropping the packets
anyway.
> > >
> > > It makes no difference at all, adding a cushion might make you
feel
> > > better, but its really not worth it.
> > > 
> > > Under memory stress, it makes better sense to drop a super big
GRO
> > > packet (The one needing frag_list extension ...)
> > > 
> > > It gives a better signal to the sender to reduce its pressure,
and gives
> > > opportunity to free more of your memory.
> > > 
> > 
> > OK, but in that case one wonders whether we should do more to free
memory?
> > 
> > E.g. imagine that we dropped a packet of a specific TCP flow
> > because we couldn't allocate a new packet.
> > 
> > What happens now is that the old packet is freed as well.
> > 
> > So quite likely the next packet in queue will get processed
> > since it will reuse the memory we have just freed.
> > 
> > The next packet and the next after it etc all will have to go through
> > the net stack until they get at the socket and are dropped then
> > because we missed a segment.  Even worse, GRO gets disabled so the
load
> > on receiver goes up instead of down.
> > 
> > Sounds like a problem doesn't it?
> 
> I see no problem at all. GRO is a hint for high rates (and obviously
> when there is enough memory)
> 
> > 
> > GRO actually detects it's the same flow and can see packet is
> > out of sequence. Why doesn't it drop the packet then?
> > Alternatively, we could (for example using the pre-allocated skb
> > like I suggested) notify GRO that it should start dropping packets
> > of this flow.
> > 
> > What do you think?
> > 
> 
> I think we disagree a lot on memory management on networking stacks.
> 
> We did a lot of work in TCP stack and Qdisc layers to lower memory
> pressure (and bufferbloat), an you seem to try hard to introduce yet
> another layer of buffer bloat in virtio_net.
> 
> So add whatever you want to proudly state to your management :
> 
> "Look how smart we are : we drop no packets in our layer"
> 
Hmm some kind of disconnect here.
I got you rmanagement about bufferbloat.

What I am saying is that maybe we should drop packets more
aggressively: when we drop one packet of a flow, why not
drop everything that's queued and is for the same flow?

On Wed, 2013-11-20 at 18:06 +0200, Michael S. Tsirkin wrote:
> Hmm some kind of disconnect here.
> I got you rmanagement about bufferbloat.
> 
> What I am saying is that maybe we should drop packets more
> aggressively: when we drop one packet of a flow, why not
> drop everything that's queued and is for the same flow?
I really hope your TCP flows use SACK ;)

Please read the rfc 2018 introduction for details.

If a packet is dropped, it doesn't mean following packets _have_ to be
dropped.

On Wed, Nov 20, 2013 at 08:14:21AM -0800, Eric Dumazet
wrote:
> On Wed, 2013-11-20 at 18:06 +0200, Michael S. Tsirkin wrote:
> 
> > Hmm some kind of disconnect here.
> > I got you rmanagement about bufferbloat.
> > 
> > What I am saying is that maybe we should drop packets more
> > aggressively: when we drop one packet of a flow, why not
> > drop everything that's queued and is for the same flow?
> 
> I really hope your TCP flows use SACK ;)
> 
> Please read the rfc 2018 introduction for details.
> 
> If a packet is dropped, it doesn't mean following packets _have_ to be
> dropped.

Got it, thanks.