Message-ID: <20250127112712.50bb6341@elisabeth>
Date: Mon, 27 Jan 2025 11:27:12 +0100
From: Stefano Brivio <sbrivio@...hat.com>
To: Eric Dumazet <edumazet@...gle.com>
Cc: Jon Maloy <jmaloy@...hat.com>, Neal Cardwell <ncardwell@...gle.com>,
 netdev@...r.kernel.org, davem@...emloft.net, kuba@...nel.org,
 passt-dev@...st.top, lvivier@...hat.com, dgibson@...hat.com,
 eric.dumazet@...il.com, Menglong Dong <menglong8.dong@...il.com>
Subject: Re: [net,v2] tcp: correct handling of extreme memory squeeze

On Mon, 27 Jan 2025 11:06:07 +0100
Eric Dumazet <edumazet@...gle.com> wrote:

> On Mon, Jan 27, 2025 at 11:01 AM Stefano Brivio <sbrivio@...hat.com> wrote:
> >
> > On Fri, 24 Jan 2025 12:40:16 -0500
> > Jon Maloy <jmaloy@...hat.com> wrote:
> >  
> > > I can certainly clear tp->pred_flags and post it again, maybe with
> > > an improved and shortened log. Would that be acceptable?  
> >
> > Talking about an improved log, what strikes me the most of the whole
> > problem is:
> >
> > $ tshark -r iperf3_jon_zero_window.pcap -td -Y 'frame.number in { 1064 .. 1068 }'
> >  1064   0.004416 192.168.122.1 → 192.168.122.198 TCP 65534 34482 → 5201 [ACK] Seq=1611679466 Ack=1 Win=36864 Len=65480
> >  1065   0.007334 192.168.122.1 → 192.168.122.198 TCP 65534 34482 → 5201 [ACK] Seq=1611744946 Ack=1 Win=36864 Len=65480
> >  1066   0.005104 192.168.122.1 → 192.168.122.198 TCP 56382 [TCP Window Full] 34482 → 5201 [ACK] Seq=1611810426 Ack=1 Win=36864 Len=56328
> >  1067   0.015226 192.168.122.198 → 192.168.122.1 TCP 54 [TCP ZeroWindow] 5201 → 34482 [ACK] Seq=1 Ack=1611090146 Win=0 Len=0
> >  1068   6.298138 fe80::44b3:f5ff:fe86:c529 → ff02::2      ICMPv6 70 Router Solicitation from 46:b3:f5:86:c5:29
> >
> > ...and then the silence, 192.168.122.198 never announces that its
> > window is not zero, so the peer gives up 15 seconds later:
> >
> > $ tshark -r iperf3_jon_zero_window_cut.pcap -td -Y 'frame.number in { 1069 .. 1070 }'
> >  1069   8.709313 192.168.122.1 → 192.168.122.198 TCP 55 34466 → 5201 [ACK] Seq=166 Ack=5 Win=36864 Len=1
> >  1070   0.008943 192.168.122.198 → 192.168.122.1 TCP 54 5201 → 34482 [FIN, ACK] Seq=1 Ack=1611090146 Win=778240 Len=0
> >
> > Data in frame #1069 is iperf3 ending the test.
> >
> > This didn't happen before e2142825c120 ("net: tcp: send zero-window
> > ACK when no memory") so it's a relatively recent (17 months) regression.
> >
> > It actually looks pretty simple (and rather serious) to me.
> 
> With all that, it should be pretty easy to cook a packetdrill test, right ?

Not really :( because to reproduce this exact condition you need to
somehow get the right amount of memory pressure so that you can
actually establish a connection, start the transfer, and then exhaust
the receive buffer at the right moment.

And packetdrill doesn't do that. Sure, it would be great if it did, and
it's probably a nice feature to implement... given enough time. Given
less time, I guess fixing regressions has a higher priority.

One could perhaps tweak sk->sk_rcvbuf as you suggested but that just
artificially reproduces one part of it. It's not a really fitting test.
For example: when would you increase it back?

> packetdrill tests are part of tools/testing/selftests/net/ already, we
> are not asking for something unreasonable.

I would agree, in general, except that I don't see a way to craft a
test like this with packetdrill. At least not trivially with the
current feature set.

On top of that, this is not a new feature, it's a fix for a regression
(that was introduced without adding any test, of course). And the fix
itself was definitely tested, just not with packetdrill.

Requesting that tests are 1. automated and 2. written with a specific
tool is something I can quite understand for general convenience, but
I don't think it always makes sense.

Especially as this fix has been blocked for about 9 months now because
of the fact that automating a test for it is quite hard.

-- 
Stefano

Powered by blists - more mailing lists