Fix packet conservation logic in QUIC BBR. Protected by FLAGS_quic_reloadable_flag_quic_bbr_fix_co…

net/quic/core/congestion_control/bbr_sender_test.cc

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "net/quic/core/congestion_control/bbr_sender.h"
 
 #include <algorithm>
 #include <map>
 #include <memory>
 
@@ skipping 72 matching lines @@
                   "BBR sender",
                   Perspective::IS_SERVER,
                   /*connection_id=*/GetPeerInMemoryConnectionId(42)),
         competing_receiver_(&simulator_,
                             "Competing receiver",
                             "Competing sender",
                             Perspective::IS_SERVER,
                             /*connection_id=*/GetPeerInMemoryConnectionId(43)),
         receiver_multiplexer_("Receiver multiplexer",
                               {&receiver_, &competing_receiver_}) {
+    // These will be changed by the appropriate tests as necessary.
+    FLAGS_quic_reloadable_flag_quic_bbr_keep_sending_at_recent_rate = false;
+    FLAGS_quic_reloadable_flag_quic_bbr_slow_recent_delivery = false;
+    FLAGS_quic_reloadable_flag_quic_bbr_add_tso_cwnd = false;
     // TODO(ianswett): Determine why tests become flaky with CWND based on SRTT.
     FLAGS_quic_reloadable_flag_quic_bbr_base_cwnd_on_srtt = false;
     FLAGS_quic_reloadable_flag_quic_bbr_extra_conservation = true;
+    FLAGS_quic_reloadable_flag_quic_bbr_fix_conservation = true;
     rtt_stats_ = bbr_sender_.connection()->sent_packet_manager().GetRttStats();
     sender_ = SetupBbrSender(&bbr_sender_);
 
     clock_ = simulator_.GetClock();
     simulator_.set_random_generator(&random_);
 
     uint64_t seed = QuicRandom::GetInstance()->RandUint64();
     random_.set_seed(seed);
     QUIC_LOG(INFO) << "BbrSenderTest simulator set up.  Seed: " << seed;
   }
@@ skipping 202 matching lines @@
   // TODO(ianswett): Tighten this bound once we understand why BBR is
   // overestimating bandwidth with aggregation. b/36022633
   EXPECT_GE(kTestLinkBandwidth * 1.5f,
             sender_->ExportDebugState().max_bandwidth);
   // TODO(ianswett): Expect 0 packets are lost once BBR no longer measures
   // bandwidth higher than the link rate.
   EXPECT_FALSE(sender_->ExportDebugState().last_sample_is_app_limited);
   // The margin here is high, because the aggregation greatly increases
   // smoothed rtt.
   EXPECT_GE(kTestRtt * 4, rtt_stats_->smoothed_rtt());
-  ExpectApproxEq(kTestRtt, rtt_stats_->min_rtt(), 0.1f);
+  ExpectApproxEq(kTestRtt, rtt_stats_->min_rtt(), 0.12f);
 }
 
 TEST_F(BbrSenderTest, SimpleTransfer2RTTAggregationKeepSending) {
   FLAGS_quic_reloadable_flag_quic_bbr_ack_aggregation_bytes = false;
   FLAGS_quic_reloadable_flag_quic_bbr_add_tso_cwnd = false;
   FLAGS_quic_reloadable_flag_quic_bbr_keep_sending_at_recent_rate = true;
   CreateDefaultSetup();
   // 2 RTTs of aggregation, with a max of 10kb.
   EnableAggregation(10 * 1024, 2 * kTestRtt);
 
@@ skipping 111 matching lines @@
             sender_->ExportDebugState().max_bandwidth);
   // TODO(ianswett): Tighten this bound once we understand why BBR is
   // overestimating bandwidth with aggregation. b/36022633
   EXPECT_GE(kTestLinkBandwidth * 1.5f,
             sender_->ExportDebugState().max_bandwidth);
   // TODO(ianswett): Expect 0 packets are lost once BBR no longer measures
   // bandwidth higher than the link rate.
   EXPECT_FALSE(sender_->ExportDebugState().last_sample_is_app_limited);
   // The margin here is high, because the aggregation greatly increases
   // smoothed rtt.
-  EXPECT_GE(kTestRtt * 4, rtt_stats_->smoothed_rtt());
-  ExpectApproxEq(kTestRtt, rtt_stats_->min_rtt(), 0.1f);
+  EXPECT_GE(kTestRtt * 5, rtt_stats_->smoothed_rtt());
+  ExpectApproxEq(kTestRtt, rtt_stats_->min_rtt(), 0.25f);
 }
 
 // Test the number of losses incurred by the startup phase in a situation when
 // the buffer is less than BDP.
 TEST_F(BbrSenderTest, PacketLossOnSmallBufferStartup) {
   CreateSmallBufferSetup();
 
   DriveOutOfStartup();
   float loss_rate =
       static_cast<float>(bbr_sender_.connection()->GetStats().packets_lost) /
       bbr_sender_.connection()->GetStats().packets_sent;
-  EXPECT_LE(loss_rate, 0.27);
+  EXPECT_LE(loss_rate, 0.31);
 }
 
 // Ensures the code transitions loss recovery states correctly (NOT_IN_RECOVERY
 // -> CONSERVATION -> GROWTH -> NOT_IN_RECOVERY).
 TEST_F(BbrSenderTest, RecoveryStates) {
   // Set seed to the position where the gain cycling causes the sender go
   // into conservation upon entering PROBE_BW.
   //
   // TODO(vasilvv): there should be a better way to test this.
   random_.set_seed(UINT64_C(14719894707049085006));
 
   const QuicTime::Delta timeout = QuicTime::Delta::FromSeconds(10);
   bool simulator_result;
   CreateSmallBufferSetup();
 
   bbr_sender_.AddBytesToTransfer(100 * 1024 * 1024);
   ASSERT_EQ(BbrSender::NOT_IN_RECOVERY,
             sender_->ExportDebugState().recovery_state);
 
   simulator_result = simulator_.RunUntilOrTimeout(
       [this]() {
         return sender_->ExportDebugState().recovery_state !=
                BbrSender::NOT_IN_RECOVERY;
       },
       timeout);
   ASSERT_TRUE(simulator_result);
   ASSERT_EQ(BbrSender::CONSERVATION,
             sender_->ExportDebugState().recovery_state);
 
+  const QuicByteCount cwnd_at_recovery_start = sender_->GetCongestionWindow();
   simulator_result = simulator_.RunUntilOrTimeout(
-      [this]() {
+      [this, cwnd_at_recovery_start]() {
+        // Ensure that the CWND never drops due to conservation.
+        if (sender_->GetCongestionWindow() < cwnd_at_recovery_start) {
+          return true;
+        }
         return sender_->ExportDebugState().recovery_state !=
                BbrSender::CONSERVATION;
       },
       timeout);
+  ASSERT_GE(sender_->GetCongestionWindow(), cwnd_at_recovery_start);
   ASSERT_TRUE(simulator_result);
   ASSERT_EQ(BbrSender::GROWTH, sender_->ExportDebugState().recovery_state);
 
   simulator_result = simulator_.RunUntilOrTimeout(
       [this]() {
         return sender_->ExportDebugState().recovery_state != BbrSender::GROWTH;
       },
       timeout);
 
   ASSERT_EQ(BbrSender::PROBE_BW, sender_->ExportDebugState().mode);
@@ skipping 299 matching lines @@
   sender_->ResumeConnectionState(params, false);
   EXPECT_EQ(kTestLinkBandwidth, sender_->ExportDebugState().max_bandwidth);
   EXPECT_EQ(kTestLinkBandwidth, sender_->BandwidthEstimate());
   ExpectApproxEq(kTestRtt, sender_->ExportDebugState().min_rtt, 0.01f);
 
   DriveOutOfStartup();
 }
 
 }  // namespace test
 }  // namespace net