From 32ee3b88ea46d6b1db79a505e0d0e3cb239be997 Mon Sep 17 00:00:00 2001 From: Victor Boivie Date: Wed, 19 May 2021 22:22:42 +0200 Subject: [PATCH] dcsctp: Ensure RTO is always greater than RTT The retransmission timeout (RTO) value is updated on every measured RTT and is a function of the RTT value and its stability. In reality, the RTT is never constant - it fluctuates, which makes the RTO become much larger than the RTT. But for extremely stable RTTs, which we get in simulations, the RTO value can become the same as the RTT, and that makes expiration timers be scheduled to the RTT value, and will race with packets that are expected to stop the expiration timer. And that race should be avoided in simulations. So ensuring that the RTO value is always greater, if only be a single millisecond, will work fine in these simulations. Bug: webrtc:12614 Change-Id: I30cf9c97e50449849ab35de52696c618d8498128 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/219680 Commit-Queue: Victor Boivie Reviewed-by: Florent Castelli Cr-Commit-Position: refs/heads/master@{#34084} --- net/dcsctp/tx/retransmission_timeout.cc | 5 ++ net/dcsctp/tx/retransmission_timeout_test.cc | 55 +++++++++++++------- 2 files changed, 40 insertions(+), 20 deletions(-) diff --git a/net/dcsctp/tx/retransmission_timeout.cc b/net/dcsctp/tx/retransmission_timeout.cc index f38b94d32c..7d545a07d0 100644 --- a/net/dcsctp/tx/retransmission_timeout.cc +++ b/net/dcsctp/tx/retransmission_timeout.cc @@ -58,6 +58,11 @@ void RetransmissionTimeout::ObserveRTT(DurationMs measured_rtt) { rto_ = srtt_ + 4 * rttvar_; } + // If the RTO becomes smaller or equal to RTT, expiration timers will be + // scheduled at the same time as packets are expected. Only happens in + // extremely stable RTTs, i.e. in simulations. + rto_ = std::fmax(rto_, rtt + 1); + // Clamp RTO between min and max. rto_ = std::fmin(std::fmax(rto_, min_rto_), max_rto_); } diff --git a/net/dcsctp/tx/retransmission_timeout_test.cc b/net/dcsctp/tx/retransmission_timeout_test.cc index eb5e72e7ba..3b2e3399fe 100644 --- a/net/dcsctp/tx/retransmission_timeout_test.cc +++ b/net/dcsctp/tx/retransmission_timeout_test.cc @@ -80,29 +80,29 @@ TEST(RetransmissionTimeoutTest, WillNeverGoAboveMaximumRto) { TEST(RetransmissionTimeoutTest, CalculatesRtoForStableRtt) { RetransmissionTimeout rto_(MakeOptions()); rto_.ObserveRTT(DurationMs(124)); - EXPECT_THAT(*rto_.rto(), 372); + EXPECT_EQ(*rto_.rto(), 372); rto_.ObserveRTT(DurationMs(128)); - EXPECT_THAT(*rto_.rto(), 314); + EXPECT_EQ(*rto_.rto(), 314); rto_.ObserveRTT(DurationMs(123)); - EXPECT_THAT(*rto_.rto(), 268); + EXPECT_EQ(*rto_.rto(), 268); rto_.ObserveRTT(DurationMs(125)); - EXPECT_THAT(*rto_.rto(), 233); + EXPECT_EQ(*rto_.rto(), 233); rto_.ObserveRTT(DurationMs(127)); - EXPECT_THAT(*rto_.rto(), 208); + EXPECT_EQ(*rto_.rto(), 208); } TEST(RetransmissionTimeoutTest, CalculatesRtoForUnstableRtt) { RetransmissionTimeout rto_(MakeOptions()); rto_.ObserveRTT(DurationMs(124)); - EXPECT_THAT(*rto_.rto(), 372); + EXPECT_EQ(*rto_.rto(), 372); rto_.ObserveRTT(DurationMs(402)); - EXPECT_THAT(*rto_.rto(), 622); + EXPECT_EQ(*rto_.rto(), 622); rto_.ObserveRTT(DurationMs(728)); - EXPECT_THAT(*rto_.rto(), 800); + EXPECT_EQ(*rto_.rto(), 800); rto_.ObserveRTT(DurationMs(89)); - EXPECT_THAT(*rto_.rto(), 800); + EXPECT_EQ(*rto_.rto(), 800); rto_.ObserveRTT(DurationMs(126)); - EXPECT_THAT(*rto_.rto(), 800); + EXPECT_EQ(*rto_.rto(), 800); } TEST(RetransmissionTimeoutTest, WillStabilizeAfterAWhile) { @@ -112,25 +112,40 @@ TEST(RetransmissionTimeoutTest, WillStabilizeAfterAWhile) { rto_.ObserveRTT(DurationMs(728)); rto_.ObserveRTT(DurationMs(89)); rto_.ObserveRTT(DurationMs(126)); - EXPECT_THAT(*rto_.rto(), 800); + EXPECT_EQ(*rto_.rto(), 800); rto_.ObserveRTT(DurationMs(124)); - EXPECT_THAT(*rto_.rto(), 800); + EXPECT_EQ(*rto_.rto(), 800); rto_.ObserveRTT(DurationMs(122)); - EXPECT_THAT(*rto_.rto(), 709); + EXPECT_EQ(*rto_.rto(), 709); rto_.ObserveRTT(DurationMs(123)); - EXPECT_THAT(*rto_.rto(), 630); + EXPECT_EQ(*rto_.rto(), 630); rto_.ObserveRTT(DurationMs(124)); - EXPECT_THAT(*rto_.rto(), 561); + EXPECT_EQ(*rto_.rto(), 561); rto_.ObserveRTT(DurationMs(122)); - EXPECT_THAT(*rto_.rto(), 504); + EXPECT_EQ(*rto_.rto(), 504); rto_.ObserveRTT(DurationMs(124)); - EXPECT_THAT(*rto_.rto(), 453); + EXPECT_EQ(*rto_.rto(), 453); rto_.ObserveRTT(DurationMs(124)); - EXPECT_THAT(*rto_.rto(), 409); + EXPECT_EQ(*rto_.rto(), 409); rto_.ObserveRTT(DurationMs(124)); - EXPECT_THAT(*rto_.rto(), 372); + EXPECT_EQ(*rto_.rto(), 372); rto_.ObserveRTT(DurationMs(124)); - EXPECT_THAT(*rto_.rto(), 339); + EXPECT_EQ(*rto_.rto(), 339); } + +TEST(RetransmissionTimeoutTest, WillAlwaysStayAboveRTT) { + // In simulations, it's quite common to have a very stable RTT, and having an + // RTO at the same value will cause issues as expiry timers will be scheduled + // to be expire exactly when a packet is supposed to arrive. The RTO must be + // larger than the RTT. In non-simulated environments, this is a non-issue as + // any jitter will increase the RTO. + RetransmissionTimeout rto_(MakeOptions()); + + for (int i = 0; i < 100; ++i) { + rto_.ObserveRTT(DurationMs(124)); + } + EXPECT_GT(*rto_.rto(), 124); +} + } // namespace } // namespace dcsctp