On Tue, Jun 26, 2018 at 10:34 PM Lawrence Brakmo <bra...@fb.com> wrote: > The only issue is if it is safe to always use 2 or if it is better to > use min(2, snd_ssthresh) (which could still trigger the problem).
Always using 2 SGTM. I don't think we need min(2, snd_ssthresh), as that should be the same as just 2, since: (a) RFCs mandate ssthresh should not be below 2, e.g. https://tools.ietf.org/html/rfc5681 page 7: ssthresh = max (FlightSize / 2, 2*SMSS) (4) (b) The main loss-based CCs used in Linux (CUBIC, Reno, DCTCP) respect that constraint, and always have an ssthresh of at least 2. And if some CC misbehaves and uses a lower ssthresh, then taking min(2, snd_ssthresh) will trigger problems, as you note. > + tp->snd_cwnd = max((int)tcp_packets_in_flight(tp) + sndcnt, 2); AFAICT this does seem like it will make the sender behavior more aggressive in cases with high loss and/or a very low per-flow fair-share. Old: o send N packets o receive SACKs for last 3 packets o fast retransmit packet 1 o using ACKs, slow-start upward New: o send N packets o receive SACKs for last 3 packets o fast retransmit packets 1 and 2 o using ACKs, slow-start upward In the extreme case, if the available fair share is less than 2 packets, whereas inflight would have oscillated between 1 packet and 2 packets with the existing code, it now seems like with this commit the inflight will now hover at 2. It seems like this would have significantly higher losses than we had with the existing code. This may or may not be OK in practice, but IMHO it is worth mentioning and discussing. neal
