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

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?

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?

-- 
MST

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"

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?