Skip to content

Commit f2ccf2a

Browse files
tomhendersontomhenderson
committed
tcp: (fixes #1351) Refine PRR RecoverFS with RFC 9937 SACK terms
Assisted-by: Claude Fable 5
1 parent 6523c1a commit f2ccf2a

7 files changed

Lines changed: 60 additions & 21 deletions

File tree

‎src/internet/model/tcp-prr-recovery.cc‎

Lines changed: 37 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -55,26 +55,47 @@ void
5555
TcpPrrRecovery::EnterRecovery(Ptr<TcpSocketState> tcb,
5656
uint32_t dupAckCount [[maybe_unused]],
5757
uint32_t unAckDataCount,
58-
uint32_t deliveredBytes)
58+
uint32_t deliveredBytes,
59+
uint32_t bytesSacked)
5960
{
60-
NS_LOG_FUNCTION(this << tcb << dupAckCount << unAckDataCount);
61+
NS_LOG_FUNCTION(this << tcb << dupAckCount << unAckDataCount << deliveredBytes << bytesSacked);
6162

6263
m_prrOut = 0;
6364
m_prrDelivered = 0;
64-
// RFC 9937 Section 6.1 (and RFC 6937 line 296): RecoverFS is the FlightSize
65-
// (SND.NXT - SND.UNA) at the start of recovery, NOT the RFC 6675 pipe
66-
// (FlightSize - sacked - lost). unAckDataCount carries SND.NXT - SND.UNA.
65+
// RFC 9937 Section 6.1 (and RFC 6937 line 296): RecoverFS is based on the
66+
// FlightSize (SND.NXT - SND.UNA) at the start of recovery, NOT the RFC 6675
67+
// pipe (FlightSize - sacked - lost). unAckDataCount carries SND.NXT - SND.UNA.
6768
// Using the smaller pipe here inflates the proportional send count and makes
68-
// PRR overshoot ssThresh at recovery exit. RFC 9937 additionally refines
69-
// this base with SACK scoreboard terms (- sacked + newlySacked
70-
// + newlyCumAcked); that second-order correction is not applied here because
71-
// the recovery-ops interface does not expose the scoreboard. Omitting it is
72-
// safe: it can only make RecoverFS larger (more conservative), never smaller.
69+
// PRR overshoot ssThresh at recovery exit.
7370
m_recoveryFlightSize = unAckDataCount;
7471

72+
// RFC 9937 Section 6.1 SACK refinement. Bytes SACKed before entering
73+
// recovery are already delivered and must not be counted as flight to be
74+
// recovered, so they are subtracted; bytes newly SACKed and newly
75+
// cumulatively acknowledged by the triggering ACK are added back:
76+
// RecoverFS = SND.NXT - SND.UNA - (bytes SACKed in scoreboard)
77+
// + (bytes newly SACKed) + (bytes newly cumulatively acked)
78+
// deliveredBytes already carries (newly SACKed + newly cumulatively acked)
79+
// (it is the caller's SACK-derived DeliveredData), and bytesSacked is the
80+
// post-ACK scoreboard SACKed total, so subtracting bytesSacked and adding
81+
// deliveredBytes nets out to removing only the pre-existing SACKed bytes.
82+
// Applied only with SACK: without SACK there is no real scoreboard (the
83+
// reno-sack estimate is handled per-ACK in DoRecovery), so RecoverFS stays
84+
// at the plain FlightSize. Signed math guards a transient underflow, and
85+
// the result is clamped to at least 1 SMSS to keep it a valid divisor.
86+
if (tcb->m_sackEnabled)
87+
{
88+
int64_t recoverFs = static_cast<int64_t>(unAckDataCount) -
89+
static_cast<int64_t>(bytesSacked) +
90+
static_cast<int64_t>(deliveredBytes);
91+
m_recoveryFlightSize =
92+
static_cast<uint32_t>(std::max<int64_t>(recoverFs, tcb->m_segmentSize));
93+
}
94+
7595
NS_LOG_INFO("Enter recovery: cWnd " << tcb->m_cWnd << " ssThresh " << tcb->m_ssThresh
7696
<< " recoveryFlightSize " << m_recoveryFlightSize
77-
<< " unAckDataCount " << unAckDataCount);
97+
<< " unAckDataCount " << unAckDataCount << " bytesSacked "
98+
<< bytesSacked << " deliveredBytes " << deliveredBytes);
7899

79100
DoRecovery(tcb, deliveredBytes, true);
80101
}
@@ -89,6 +110,11 @@ TcpPrrRecovery::DoRecovery(Ptr<TcpSocketState> tcb, uint32_t deliveredBytes, boo
89110
// With SACK (the ns-3 default), the caller already supplies the SACK-derived
90111
// DeliveredData (change in SND.UNA plus newly SACKed bytes), so adding a
91112
// segment here would double-count and inflate prr_delivered.
113+
// The m_prrDelivered < m_recoveryFlightSize guard is the RFC's Savage99
114+
// anti-inflation mitigation (RFC 9937 Section 6.2): without SACK, PRR
115+
// disallows incrementing DeliveredData once the total delivered in the
116+
// episode would exceed RecoverFS. This bound is spec-mandated, not
117+
// incidental; do not drop it.
92118
if (isDupAck && !tcb->m_sackEnabled && m_prrDelivered < m_recoveryFlightSize)
93119
{
94120
deliveredBytes += tcb->m_segmentSize;

‎src/internet/model/tcp-prr-recovery.h‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,8 @@ class TcpPrrRecovery : public TcpClassicRecovery
5555
void EnterRecovery(Ptr<TcpSocketState> tcb,
5656
uint32_t dupAckCount,
5757
uint32_t unAckDataCount,
58-
uint32_t deliveredBytes) override;
58+
uint32_t deliveredBytes,
59+
uint32_t bytesSacked) override;
5960

6061
void DoRecovery(Ptr<TcpSocketState> tcb, uint32_t deliveredBytes, bool isDupAck) override;
6162

‎src/internet/model/tcp-recovery-ops.cc‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -86,7 +86,8 @@ void
8686
TcpClassicRecovery::EnterRecovery(Ptr<TcpSocketState> tcb,
8787
uint32_t dupAckCount,
8888
uint32_t unAckDataCount [[maybe_unused]],
89-
uint32_t deliveredBytes [[maybe_unused]])
89+
uint32_t deliveredBytes [[maybe_unused]],
90+
uint32_t bytesSacked [[maybe_unused]])
9091
{
9192
NS_LOG_FUNCTION(this << tcb << dupAckCount << unAckDataCount);
9293
tcb->m_cWnd = tcb->m_ssThresh;

‎src/internet/model/tcp-recovery-ops.h‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -85,13 +85,15 @@ class TcpRecoveryOps : public Object
8585
*
8686
* @param tcb internal congestion state
8787
* @param dupAckCount duplicate acknowledgement count
88-
* @param unAckDataCount total bytes of data unacknowledged
88+
* @param unAckDataCount total bytes of data unacknowledged (SND.NXT - SND.UNA)
8989
* @param deliveredBytes bytes (S)ACKed in the last (S)ACK
90+
* @param bytesSacked total bytes SACKed in the scoreboard (post-ACK)
9091
*/
9192
virtual void EnterRecovery(Ptr<TcpSocketState> tcb,
9293
uint32_t dupAckCount,
9394
uint32_t unAckDataCount,
94-
uint32_t deliveredBytes) = 0;
95+
uint32_t deliveredBytes,
96+
uint32_t bytesSacked) = 0;
9597

9698
/**
9799
* @brief Performs recovery based on the recovery algorithm
@@ -178,7 +180,8 @@ class TcpClassicRecovery : public TcpRecoveryOps
178180
void EnterRecovery(Ptr<TcpSocketState> tcb,
179181
uint32_t dupAckCount,
180182
uint32_t unAckDataCount,
181-
uint32_t deliveredBytes) override;
183+
uint32_t deliveredBytes,
184+
uint32_t bytesSacked) override;
182185

183186
void DoRecovery(Ptr<TcpSocketState> tcb, uint32_t deliveredBytes, bool isDupAck) override;
184187

‎src/internet/model/tcp-socket-base.cc‎

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1732,7 +1732,11 @@ TcpSocketBase::EnterCwr(uint32_t currentDelivered)
17321732
if (!m_congestionControl->HasCongControl())
17331733
{
17341734
// If there is a recovery algorithm, invoke it.
1735-
m_recoveryOps->EnterRecovery(m_tcb, m_dupAckCount, UnAckDataCount(), currentDelivered);
1735+
m_recoveryOps->EnterRecovery(m_tcb,
1736+
m_dupAckCount,
1737+
UnAckDataCount(),
1738+
currentDelivered,
1739+
m_txBuffer->GetSacked());
17361740
NS_LOG_INFO("Enter CWR recovery mode; set cwnd to " << m_tcb->m_cWnd << ", ssthresh to "
17371741
<< m_tcb->m_ssThresh << ", recover to "
17381742
<< m_recover);
@@ -1787,7 +1791,11 @@ TcpSocketBase::EnterRecovery(uint32_t currentDelivered)
17871791

17881792
if (!m_congestionControl->HasCongControl())
17891793
{
1790-
m_recoveryOps->EnterRecovery(m_tcb, m_dupAckCount, UnAckDataCount(), currentDelivered);
1794+
m_recoveryOps->EnterRecovery(m_tcb,
1795+
m_dupAckCount,
1796+
UnAckDataCount(),
1797+
currentDelivered,
1798+
m_txBuffer->GetSacked());
17911799
NS_LOG_INFO(m_dupAckCount << " dupack. Enter fast recovery mode."
17921800
<< "Reset cwnd to " << m_tcb->m_cWnd << ", ssthresh to "
17931801
<< m_tcb->m_ssThresh << " at fast recovery seqnum " << m_recover

‎src/internet/test/tcp-classic-recovery-test.cc‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -79,7 +79,7 @@ ClassicRecoveryTest::DoRun()
7979
"TcpClassicRecovery",
8080
"The name of recovery used should be TcpClassicRecovery");
8181

82-
recovery->EnterRecovery(m_state, m_dupAckCount, 1000, 0);
82+
recovery->EnterRecovery(m_state, m_dupAckCount, 1000, 0, 0);
8383
NS_TEST_ASSERT_MSG_EQ(m_state->m_cWnd,
8484
m_state->m_ssThresh,
8585
"cWnd should be set to ssThresh on entering recovery");

‎src/internet/test/tcp-prr-recovery-test.cc‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -97,7 +97,7 @@ PrrRecoveryTest::DoRun()
9797

9898
Ptr<TcpPrrRecovery> recovery = CreateObject<TcpPrrRecovery>();
9999

100-
recovery->EnterRecovery(m_state, 3, m_unAckDataCount, 0);
100+
recovery->EnterRecovery(m_state, 3, m_unAckDataCount, 0, 0);
101101

102102
NS_TEST_ASSERT_MSG_GT_OR_EQ(m_state->m_cWnd.Get(),
103103
m_cWnd + m_segmentSize,
@@ -208,7 +208,7 @@ PrrRecoveryArithmeticTest::DoRun()
208208
Ptr<TcpPrrRecovery> recovery = CreateObject<TcpPrrRecovery>();
209209

210210
// EnterRecovery sets RecoverFS = bytesInFlight and performs the first DoRecovery.
211-
recovery->EnterRecovery(state, 3, m_bytesInFlight, 0);
211+
recovery->EnterRecovery(state, 3, m_bytesInFlight, 0, 0);
212212

213213
// Report data sent during recovery, advancing m_prrOut one segment at a time.
214214
for (uint32_t sent = 0; sent < m_bytesSentInRecovery; sent += m_segmentSize)

0 commit comments

Comments
 (0)