On 17 May 2018 at 22:41, Toke Høiland-Jørgensen <t...@toke.dk> wrote: > Eric Dumazet <eric.duma...@gmail.com> writes: > >> On 05/17/2018 04:23 AM, Toke Høiland-Jørgensen wrote: >> >>> >>> We don't do full parsing of SACKs, no; we were trying to keep things >>> simple... We do detect the presence of SACK options, though, and the >>> presence of SACK options on an ACK will make previous ACKs be considered >>> redundant. >>> >> >> But they are not redundant in some cases, particularly when reorders >> happen in the network. > > Huh. I was under the impression that SACKs were basically cumulative > until cleared. > > I.e., in packet sequence ABCDE where B and D are lost, C would have > SACK(B) and E would have SACK(B,D). Are you saying that E would only > have SACK(D)?
SACK works by acknowledging additional ranges above those that have been ACKed, rather than ACKing up to the largest seen sequence number and reporting missing ranges before that. A - ACK(A) B - lost C - ACK(A) + SACK(C) D - lost E - ACK(A) + SACK(C, E) Cake does check that the ACK sequence number is greater, or if it is equal and the 'newer' ACK has the SACK option present. It doesn't compare the sequence numbers inside two SACKs. If the two SACKs in the above example had been reordered before reaching cake's ACK filter in aggressive mode, the wrong one will be filtered. This is a limitation of my naive SACK handling in cake. The default 'conservative' mode happens to mitigate the problem in the above scenario, but the issue could still present itself in more pathological cases. It's fixable, however I'm not sure this corner case is sufficiently common or severe to warrant the extra complexity. Ryan.