From f03c7e48e71009a64c30b57648fd234710fd578a Mon Sep 17 00:00:00 2001 From: Bhasker Hariharan Date: Tue, 29 Jan 2019 18:12:31 -0800 Subject: Fix IsLost check to match the description in RFC6675. quoting what "rscheff@gmx.at" pointed out over email. "IsLost in RFC3517 is defined as >= (DupThresh * SMSS) while RFC6675 improves upon this, and defines IsLost as > ((DupThresh - 1) * SMSS + 1). The latter addresses situations where partial segments (size < MSS) are sent (eg. last segment of a http protocol message sent with PSH being less than MSS is common)." PiperOrigin-RevId: 231512331 Change-Id: I1addd4a92e3e7baeb0bdda46463ebfae435da958 --- pkg/tcpip/transport/tcp/sack_scoreboard.go | 2 +- pkg/tcpip/transport/tcp/sack_scoreboard_test.go | 21 +++++++++++---------- 2 files changed, 12 insertions(+), 11 deletions(-) diff --git a/pkg/tcpip/transport/tcp/sack_scoreboard.go b/pkg/tcpip/transport/tcp/sack_scoreboard.go index d251d32db..5dba8a165 100644 --- a/pkg/tcpip/transport/tcp/sack_scoreboard.go +++ b/pkg/tcpip/transport/tcp/sack_scoreboard.go @@ -201,7 +201,7 @@ func (s *SACKScoreboard) IsLost(r header.SACKBlock) bool { } nDupSACKBytes += sacked.Start.Size(sacked.End) nDupSACK++ - if nDupSACK >= nDupAckThreshold || nDupSACKBytes >= seqnum.Size(nDupAckThreshold*s.smss) { + if nDupSACK >= nDupAckThreshold || nDupSACKBytes >= seqnum.Size((nDupAckThreshold-1)*s.smss) { isLost = true return false } diff --git a/pkg/tcpip/transport/tcp/sack_scoreboard_test.go b/pkg/tcpip/transport/tcp/sack_scoreboard_test.go index db4b52aca..1a1178999 100644 --- a/pkg/tcpip/transport/tcp/sack_scoreboard_test.go +++ b/pkg/tcpip/transport/tcp/sack_scoreboard_test.go @@ -94,20 +94,21 @@ func TestSACKScoreboardIsLost(t *testing.T) { s.Insert(header.SACKBlock{51, 100}) s.Insert(header.SACKBlock{111, 120}) s.Insert(header.SACKBlock{101, 110}) - s.Insert(header.SACKBlock{121, 151}) + s.Insert(header.SACKBlock{121, 141}) testCases := []struct { block header.SACKBlock lost bool }{ - {header.SACKBlock{0, 1}, true}, - {header.SACKBlock{1, 2}, false}, - {header.SACKBlock{1, 45}, false}, - {header.SACKBlock{50, 51}, true}, - // This one should return true because there are > 3* 10 (smss) - // bytes that have been sacked above this sequence number. - {header.SACKBlock{119, 120}, true}, - {header.SACKBlock{120, 121}, true}, - {header.SACKBlock{125, 126}, false}, + {block: header.SACKBlock{0, 1}, lost: true}, + {block: header.SACKBlock{1, 2}, lost: false}, + {block: header.SACKBlock{1, 45}, lost: false}, + {block: header.SACKBlock{50, 51}, lost: true}, + // This one should return true because there are + // > (nDupAckThreshold - 1) * 10 (smss) bytes that have been sacked above + // this sequence number. + {block: header.SACKBlock{119, 120}, lost: true}, + {block: header.SACKBlock{120, 121}, lost: true}, + {block: header.SACKBlock{125, 126}, lost: false}, } for _, tc := range testCases { if want, got := tc.lost, s.IsLost(tc.block); got != want { -- cgit v1.2.3