On 2021-01-12 09:30 (+0100), Eric Dumazet <eduma...@google.com> wrote:
> 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.
Patches 2 and 4 might be arguable, though.
Is you objection primarily about the limit removed by patch 4?
> 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).
> 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. 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.
> 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?
> If you want to do research, this is fine, but please do not break
> production TCP stack.
>
> Thank you.
