Re: [PATCH net-next 4/4] tcp: remove limit on initial receive window

On Tue, Jan 12, 2021 at 5:02 PM Heath Caldwell [off-list ref] wrote:

On 2021-01-12 09:30 (+0100), Eric Dumazet [off-list ref] wrote:

quoted

I think the whole patch series is an attempt to badly break TCP stack.

Can you explain the concern that you have about how these changes might
break the TCP stack?

Patches 1 and 3 fix clear bugs.

Not clear to me at least.

If they were bug fixes, a FIxes: tag would be provided.

You are a first time contributor to linux TCP stack, so better make
sure your claims are solid.

Patches 2 and 4 might be arguable, though.

So we have to pick up whatever pleases us ?

Is you objection primarily about the limit removed by patch 4?

quoted

Hint : 64K is really the max allowed by TCP standards. Yes, this is
sad, but this is it.

Do you mean the limit imposed by the size of the "Window Size" header
field?  This limitation is directly addressed by the check in
__tcp_transmit_skb():

        if (likely(!(tcb->tcp_flags & TCPHDR_SYN))) {
                th->window      = htons(tcp_select_window(sk));
                tcp_ecn_send(sk, skb, th, tcp_header_size);
        } else {
                /* RFC1323: The window in SYN & SYN/ACK segments
                 * is never scaled.
                 */
                th->window      = htons(min(tp->rcv_wnd, 65535U));
        }

and checking (and capping it there) allows for the field to not overflow
while also not artificially restricting the size of the window which
will later be advertised (once window scaling is negotiated).

quoted

I will not spend hours of work running  packetdrill tests over your
changes, but I am sure they are now quite broken.

If you believe auto tuning is broken, fix it properly, without trying
to change all the code so that you can understand it.

The removal of the limit specifically addresses the situation where auto
tuning cannot work: on the initial burst.

Which standard allows for a very big initial burst ?
AFAIK, IW10 got a lot of pushback, and that was only for a burst of ~14600 bytes
Allowing arbitrarily large windows needs IETF approval.

 There is no way to know
whether an installation desires to receive a larger first burst unless
it is specifically configured - and this limit prevents such
configuration.

quoted

I strongly advise you read RFC 7323 before doing any changes in TCP
stack, and asking us to spend time reviewing your patches.

Can you point out the part of the RFC which would be violated by
initially (that is, the first packet after the SYN) advertising a window
larger than 64KB?

We handle gradual increase of rwin based on the behavior of
applications and senders (skb len/truesize ratio)

Again if you believe autotuning is broken please fix it, instead of
throwing it away.

Changing initial RWIN can have subtle differences on how fast DRS algo
can kick in.

This kind of change would need to be heavily tested, with millions of
TCP flows in the wild.
(ie not in a lab environment)

Have you done this ? What happens if flows are malicious and sent 100
bytes per segment ?