From fe4459dbac3f4aa7fad9d414fec851a520e5cc16 Mon Sep 17 00:00:00 2001
From: Kevin Krakauer <krakauer@google.com>
Date: Wed, 15 May 2024 14:12:32 -0700
Subject: [PATCH] netstack: don't count PMTUD when detecting loss

Throughput could be lowered by entering fast recovery unnecessarily. When a
larger-than-MTU segment was retransmitted as multiple segments, loss detection
could fire either because we hit the dupack threshold or RACK detected loss due
to ACKs from retransmissions.

RACK was more succeptible to this because it's better at detecting loss and can
do so even without 3 dupacks. Thus it fell into this trap more often.

Addresses #10344.

PiperOrigin-RevId: 634071568
---
 pkg/tcpip/transport/tcp/rack.go              |  6 ++++--
 pkg/tcpip/transport/tcp/snd.go               | 14 ++++++++++----
 pkg/tcpip/transport/tcp/test/e2e/tcp_test.go |  1 +
 3 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/pkg/tcpip/transport/tcp/rack.go b/pkg/tcpip/transport/tcp/rack.go
index 66ea6e5b0..35e6cc725 100644
--- a/pkg/tcpip/transport/tcp/rack.go
+++ b/pkg/tcpip/transport/tcp/rack.go
@@ -357,11 +357,13 @@ func (rc *rackControl) detectLoss(rcvTime tcpip.MonotonicTime) int {
 	var timeout time.Duration
 	numLost := 0
 	for seg := rc.snd.writeList.Front(); seg != nil && seg.xmitCount != 0; seg = seg.Next() {
-		if rc.snd.ep.scoreboard.IsSACKED(seg.sackBlock()) {
+		// xmitCount can be 0 for packets that are broken up for PMTUD.
+		// The initial transmission "doesn't count" WRT loss detection.
+		if rc.snd.ep.scoreboard.IsSACKED(seg.sackBlock()) || seg.xmitCount == 0 {
 			continue
 		}
 
-		if seg.lost && seg.xmitCount == 1 {
+		if seg.lost && seg.xmitCount > 1 {
 			numLost++
 			continue
 		}
diff --git a/pkg/tcpip/transport/tcp/snd.go b/pkg/tcpip/transport/tcp/snd.go
index 5dccf81d1..4e51d8b03 100644
--- a/pkg/tcpip/transport/tcp/snd.go
+++ b/pkg/tcpip/transport/tcp/snd.go
@@ -342,10 +342,16 @@ func (s *sender) updateMaxPayloadSize(mtu, count int) {
 			break
 		}
 
-		if nextSeg == s.writeNext && seg.payloadSize() > m {
-			// We found a segment exceeding the MTU. Rewind
-			// writeNext and try to retransmit it.
-			nextSeg = seg
+		if seg.payloadSize() > m {
+			// xmitCount is used for loss detection, but
+			// retransmission doesn't indicate congestion here,
+			// it's just PMTUD.
+			seg.xmitCount = 0
+			if nextSeg == s.writeNext {
+				// We found a segment exceeding the MTU. Rewind
+				// writeNext and try to retransmit it.
+				nextSeg = seg
+			}
 		}
 
 		if s.ep.SACKPermitted && s.ep.scoreboard.IsSACKED(seg.sackBlock()) {
diff --git a/pkg/tcpip/transport/tcp/test/e2e/tcp_test.go b/pkg/tcpip/transport/tcp/test/e2e/tcp_test.go
index 9dcb8de85..5981be60f 100644
--- a/pkg/tcpip/transport/tcp/test/e2e/tcp_test.go
+++ b/pkg/tcpip/transport/tcp/test/e2e/tcp_test.go
@@ -5739,6 +5739,7 @@ func TestPathMTUDiscovery(t *testing.T) {
 			}
 			checker.IPv4(t, p,
 				checker.PayloadLen(size+header.TCPMinimumSize),
+				checker.FragmentFlags(header.IPv4FlagDontFragment),
 				checker.TCP(
 					checker.DstPort(context.TestPort),
 					checker.TCPSeqNum(seqNum),