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;
MIME-Version: 1.0
References: <20230517124201.441634-1-imagedong@tencent.com>
 <20230517124201.441634-4-imagedong@tencent.com> <CANn89iKLf=V664AsUYC52h_q-xjEq9xC3KqTq8q+t262T91qVQ@mail.gmail.com>
 <CADxym3a0gmzmD3Vwu_shoJnAHm-xjD5tJRuKwTvAXnVk_H55AA@mail.gmail.com>
 <CADVnQynZ67511+cKF=hyiaLx5-fqPGGmpyJ-5Lk6ge-ivmAf-w@mail.gmail.com>
 <CADxym3ZiyYK7Vyz05qLv8jOPmNZXXepCsTbZxdkhSQxRx0cdSA@mail.gmail.com>
 <CADVnQy=JQkVGRsbL0u=-oZSpdaFBpz907yX24p3uUu2pMhUjGg@mail.gmail.com>
 <CADxym3awe-c29C-e1Y+efepLdpFWrG520ezJO1EjJ5C3arq6Eg@mail.gmail.com>
 <CADVnQyk2y68HKScad4W2jOy9uqe7TTCyY-StwdLWFPJhXU+CUA@mail.gmail.com>
 <CADxym3bbGkOv4dwATp6wT0KA4ZPiPGfxvqvYtEzF45GJDe=RXQ@mail.gmail.com>
 <CADVnQymzzZ9m30-1ZA+muvmt-bjaebtMT0Bh8Wp5vXZuYifONQ@mail.gmail.com>
 <CADxym3bLnLS=V5L6hZ9-PhrE3ZPpW2YD_cAG=XntEH9ky1Z0aw@mail.gmail.com>
 <CADVnQyk4Wew4iv9WRGAOiJE6fKD+OuvnwF8UQcDf+G5Ou25yUA@mail.gmail.com> <CADxym3YRLU7QVv1gWxZuV3W3tnFTbU+JKMLL_t9B+LxZCQEdig@mail.gmail.com>
In-Reply-To: <CADxym3YRLU7QVv1gWxZuV3W3tnFTbU+JKMLL_t9B+LxZCQEdig@mail.gmail.com>
From:   Neal Cardwell <ncardwell@google.com>
Date:   Wed, 24 May 2023 10:49:48 -0400
Message-ID: <CADVnQy=KO5N=8S9Ow9Z40d-UYqvCH_ZLery_CiaJYDGmjB5qkg@mail.gmail.com>
Subject: Re: [PATCH net-next 3/3] net: tcp: handle window shrink properly
To:     Menglong Dong <menglong8.dong@gmail.com>
Cc:     Eric Dumazet <edumazet@google.com>, kuba@kernel.org,
        davem@davemloft.net, pabeni@redhat.com, dsahern@kernel.org,
        netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
        Menglong Dong <imagedong@tencent.com>,
        Yuchung Cheng <ycheng@google.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Wed, May 24, 2023 at 8:16=E2=80=AFAM Menglong Dong <menglong8.dong@gmail=
.com> wrote:
>
> On Tue, May 23, 2023 at 9:27=E2=80=AFPM Neal Cardwell <ncardwell@google.c=
om> wrote:
> > > Oh, I understand what you mean now. You are saying that
> > > retransmit that first packet in the retransmit queue instead
> > > of zero-window probe packet when OOM of the receiver,
> > > isn't it? In other word, retransmit the unacked data and ignore
> > > the tcp_retries2 when we find the receiver is in OOM state.
> >
> > Yes. The idea would be to use a heuristic to estimate the receiver is
> > currently OOM and use ICSK_TIME_PROBE0 / tcp_probe_timer() /
> > tcp_write_wakeup() in this case instead of ICSK_TIME_RETRANS /
> > tcp_retransmit_timer().
> >
>
> Well, I think that maybe we should use ICSK_TIME_PROBE0 /
> tcp_probe_timer() / tcp_retransmit_skb(), isn't it?
>
> What tcp_write_wakeup() does is send new data if the receive
> window available, which means push new data into the retransmit
> queue. However, what we need to do now is retransmit the first
> packet in the rtx queue, isn't it?
>
> In the tcp_ack(), we estimate that if the receiver is OOM and
> mark the sk with OOM state, and raise ICSK_TIME_PROBE0.
> When new data is acked, we leave the OOM state.
>
> The OOM state can only happen when the rtx queue is not empty,
> otherwise the tcp connection will enter normal zero-window probe
> state. So when the timeout of ICSK_TIME_PROBE0, we need
> retransmit the skb in the rtx queue.
>
> tcp_write_wakeup() don't do the job the retransmit packet, but
> send new data.
>
> Am I right?

Yes, that's a good point that the tcp_write_wakeup() code is not
currently a good fit for the OOM case, since it currently can only
send unsent data.

cheers,
neal


> Thanks!
> Menglong Dong
>
> > > That's an option, and we can make the length of the data we
> > > send to 1 byte, which means we keep retransmitting the first
> > > byte that has not be acked in the retransmit queue.
> >
> > I don't think it would be worth adding special-case code to only send
> > 1 byte.  If the data receiver is not OOM then for CPU and memory
> > efficiency reasons (as well as simplicity) the data sender should send
> > it a full MSS. So for those reasons I would suggest that in this
> > potential approach tcp_write_wakeup() should stay the same.
> >
> > neal