Re: [PATCH v3 net 0/5] tcp: fixes to non-SACK TCP

On Tue, 13 Mar 2018, Eric Dumazet wrote:

On Tue, Mar 13, 2018 at 3:25 AM, Ilpo Jï¿œrvinen
[off-list ref] wrote:

quoted

Here is a series of fixes to issues that occur when SACK is not
enabled for a TCP connection. These are not fixes to just some
remote corner cases of recovery but many/almost all recoveries
without SACK will be impacted by one (or even more than one) of
them. The sender-side changes (1-4) are not mainly, if any, to
improve performance (throughput) but address correctness
(congestion control responses should not get incorrectly
reverted) and burstiness (that may cause additional problems
later as some of the packets in such bursts may get dropped
needing again to resort to loss recovery that is likely
similarly bursty).

GRO (or similar) adoption a long time ago (not only by linux) had a
serious impact on non SACK flow.
Should we also disable GRO by default ?
(my answer is no, just in case someone wonders)

I am sorry, but I am still not convinced by your patches, trying to
give a wrong incentive to people keeping their prehistoric stacks,
that have serious problems on wifi anyway, and or middle boxes
"aggregating/compressing ACKS"

So now you think that I'm against high perf enhancements (even after 
having written some bits of them)?

However, I think that in case somebody disables something that is enabled 
by default, be it SACK, GRO, timestamps, etc., TCP he/she will get should 
still make sense (and that regardless of some middleboxes trying hard to 
break otherwise working stuff). But I guess you don't agree here(?) and 
would perhaps even want to remove ability to disable them? Although 
obviously that wouldn't even work with non-SACK (unless RST or so starts 
to get used but even that could be worked-around unfortunately).

Also, I'm somewhat confused your position here. On one hand you said that
tests should be added to regression tests to avoid breaking these non-SACK
cases again implying that things should be fixed and on the other hand you 
seem to say that non-SACK must be left broken?

The last patch is particularly problematic in my opinion, you have not
provided packetdrill tests to prove there was no danger.

Can that even be done? Proving absence of danger with packetdrill?
AFAIK that kind of proofs are available only with formal verification.
 

It seems you leave to us the task of dealing with possible issues,
only added a bold changelog.

Heh, I tried to add details to the changelog because you explicitly said
I should and now you fault me on doing that :-). Also, you're leaving out 
the detail that I tried to understand the condition that worried you and
found out that the code already disallows any shrinking of advertized 
window for duplicate ACKs (but I guess there just might have been some 
miscommunication between us given that your last reply to 5/5
v1 made no sense to me).

Since the bugs you claim to fix are at least 10 years old, and your
fixes are far from being trivial,
please wait our packetdrill regression tests being published.
This should happen in less than one month I believe, in time for linux-4.17

Note that the publication of the updated packetdrill and test suite is
not an easy task,
since we accumulated a lot of hacks both in kernel to cope with timer
slacks and in packetdrill
for various experimental or private features that are not yet in
upstream kernels.

So we are cleaning all this, and will be happy to help you if you need.

I slightly disagree on the trivial bit here as I think it's trivial to 
see the changes only affect non-SACK flows (of which you seem to say you 
don't care if they're broken as that gives incentive to use SACK). But 
then I'm not too worried about waiting a month.


-- 
 i.