Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Content-Type: text/plain;
        charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3731.700.6\))
Subject: Re: [PATCH net-next v3 3/3] net: tcp: fix unexcepted socket die when
 snd_wnd is 0
From:   Menglong Dong <menglong8.dong@gmail.com>
In-Reply-To: <CANn89iK16069CvbA+p=WyZVYftvHs=FviQp1GSWUTG2ihRfKDA@mail.gmail.com>
Date:   Tue, 8 Aug 2023 21:07:40 +0800
Cc:     ncardwell@google.com, davem@davemloft.net, kuba@kernel.org,
        pabeni@redhat.com, dsahern@kernel.org, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org, flyingpeng@tencent.com,
        Menglong Dong <imagedong@tencent.com>
Content-Transfer-Encoding: quoted-printable
Message-Id: <19C01818-58B5-43BF-9D59-123B30949B95@gmail.com>
References: <20230808115835.2862058-1-imagedong@tencent.com>
 <20230808115835.2862058-4-imagedong@tencent.com>
 <CANn89iK16069CvbA+p=WyZVYftvHs=FviQp1GSWUTG2ihRfKDA@mail.gmail.com>
To:     Eric Dumazet <edumazet@google.com>
Precedence: bulk


> On Aug 8, 2023, at 20:49, Eric Dumazet <edumazet@google.com> wrote:
>=20
> On Tue, Aug 8, 2023 at 1:59=E2=80=AFPM <menglong8.dong@gmail.com> =
wrote:
>>=20
[=E2=80=A6=E2=80=A6]
>>=20
>> diff --git a/net/ipv4/tcp_timer.c b/net/ipv4/tcp_timer.c
>> index d45c96c7f5a4..f30d1467771c 100644
>> --- a/net/ipv4/tcp_timer.c
>> +++ b/net/ipv4/tcp_timer.c
>> @@ -454,6 +454,18 @@ static void tcp_fastopen_synack_timer(struct =
sock *sk, struct request_sock *req)
>>                          req->timeout << req->num_timeout, =
TCP_RTO_MAX);
>> }
>>=20
>> +static bool tcp_rtx_probe0_timed_out(struct sock *sk)
>=20
> const struct sock *sk
>=20
>> +{
>> +       struct tcp_sock *tp =3D tcp_sk(sk);
>=20
> const struct tcp_sock *tp =3D tcp_sk(sk);
>=20
>> +       u32 timeout_ts, rtx_ts, rcv_ts;
>> +
>> +       rtx_ts =3D tp->retrans_stamp;
>> +       rcv_ts =3D tp->rcv_tstamp;
>> +       timeout_ts =3D after(rtx_ts, rcv_ts) ? rtx_ts : rcv_ts;
>> +       timeout_ts +=3D TCP_RTO_MAX;
>=20
> If we are concerned with a socket dying too soon, I would suggest
> adding 2*TCP_RTO_MAX instead of TCP_RTO_MAX
>=20
> When a receiver is OOMing, it is possible the ACK RWIN 0 can not be =
sent all,
> so tp->rcv_tstamp will not be refreshed. Or ACK could be lost in the
> network path.

Yeah, I concern abort this too. I introduce the funtion =
=E2=80=9Ctcp_rtx_probe0_timed_out()=E2=80=9D
here is to offer a more reliable way to check the timeout in the =
feature.
And for this time, we can fix the problem first, as you advised, adding
2*TCP_RTO_MAX instead of TCP_RTO_MAX.

> This also suggests the net_dbg_ratelimited("Peer %pI4:%u/%u
> unexpectedly shrunk window %u:%u (repaired)\n"...) messages
> are slightly wrong, because they could be printed even if we did not
> receive a new ACK packet from the remote peer.
>=20
> Perhaps we should change them to include delays (how long @skb stayed
> in rtx queue, how old is the last ACK we received)

Sounds great, which is more valuable. I=E2=80=99ll change them
in the next version.

Thanks!
Menglong Dong

>> +
>> +       return after(inet_csk(sk)->icsk_timeout, timeout_ts);
>> +}
>>=20
>> /**
>>  *  tcp_retransmit_timer() - The TCP retransmit timeout handler
>> @@ -519,7 +531,7 @@ void tcp_retransmit_timer(struct sock *sk)
>>                                            tp->snd_una, tp->snd_nxt);
>>                }
>> #endif
>> -               if (tcp_jiffies32 - tp->rcv_tstamp > TCP_RTO_MAX) {
>> +               if (tcp_rtx_probe0_timed_out(sk)) {
>>                        tcp_write_err(sk);
>>                        goto out;
>>                }
>> --
>> 2.40.1