Fix a byte counting bug in QUIC BBR2.
If a packet is detected as lost and then acked, its size is double counted towards both bytes_lost and bytes_acked in BandwidthSampler. With the fix it will only be counted as lost.
Credit to slusnys@gmail.com from https://groups.google.com/g/bbr-dev/c/fJYeAawFD90
Protected by FLAGS_quic_bbr2_fix_spurious_loss_bytes_counting.
PiperOrigin-RevId: 592228843
diff --git a/quiche/quic/core/congestion_control/bandwidth_sampler.cc b/quiche/quic/core/congestion_control/bandwidth_sampler.cc
index fe42e08..e79a106 100644
--- a/quiche/quic/core/congestion_control/bandwidth_sampler.cc
+++ b/quiche/quic/core/congestion_control/bandwidth_sampler.cc
@@ -301,6 +301,15 @@
SendTimeState last_acked_packet_send_state;
QuicBandwidth max_send_rate = QuicBandwidth::Zero();
for (const auto& packet : acked_packets) {
+ if (GetQuicReloadableFlag(quic_bbr2_fix_spurious_loss_bytes_counting) &&
+ packet.spurious_loss) {
+ QUIC_RELOADABLE_FLAG_COUNT(quic_bbr2_fix_spurious_loss_bytes_counting);
+ // If the packet has been detected as lost before, QuicSentPacketManager
+ // should set the AckedPacket.bytes_acked to 0 before passing the packet
+ // to the congestion controller.
+ QUICHE_DCHECK_EQ(packet.bytes_acked, 0);
+ continue;
+ }
BandwidthSample sample =
OnPacketAcknowledged(ack_time, packet.packet_number);
if (!sample.state_at_send.is_valid) {
diff --git a/quiche/quic/core/congestion_control/bbr2_misc.cc b/quiche/quic/core/congestion_control/bbr2_misc.cc
index f3306ec..75be707 100644
--- a/quiche/quic/core/congestion_control/bbr2_misc.cc
+++ b/quiche/quic/core/congestion_control/bbr2_misc.cc
@@ -8,6 +8,7 @@
#include "quiche/quic/core/quic_bandwidth.h"
#include "quiche/quic/core/quic_time.h"
#include "quiche/quic/core/quic_types.h"
+#include "quiche/quic/platform/api/quic_bug_tracker.h"
#include "quiche/quic/platform/api/quic_flag_utils.h"
#include "quiche/quic/platform/api/quic_flags.h"
#include "quiche/quic/platform/api/quic_logging.h"
@@ -152,11 +153,19 @@
congestion_event->prior_bytes_in_flight -
congestion_event->bytes_acked - congestion_event->bytes_lost;
} else {
- QUIC_LOG_FIRST_N(ERROR, 1)
- << "prior_bytes_in_flight:" << congestion_event->prior_bytes_in_flight
- << " is smaller than the sum of bytes_acked:"
- << congestion_event->bytes_acked
- << " and bytes_lost:" << congestion_event->bytes_lost;
+ if (GetQuicReloadableFlag(quic_bbr2_fix_spurious_loss_bytes_counting)) {
+ QUIC_BUG(quic_bbr2_prior_in_flight_too_small)
+ << "prior_bytes_in_flight:" << congestion_event->prior_bytes_in_flight
+ << " is smaller than the sum of bytes_acked:"
+ << congestion_event->bytes_acked
+ << " and bytes_lost:" << congestion_event->bytes_lost;
+ } else {
+ QUIC_LOG_FIRST_N(ERROR, 1)
+ << "prior_bytes_in_flight:" << congestion_event->prior_bytes_in_flight
+ << " is smaller than the sum of bytes_acked:"
+ << congestion_event->bytes_acked
+ << " and bytes_lost:" << congestion_event->bytes_lost;
+ }
congestion_event->bytes_in_flight = 0;
}
diff --git a/quiche/quic/core/congestion_control/bbr2_sender.h b/quiche/quic/core/congestion_control/bbr2_sender.h
index a10d24b..636d340 100644
--- a/quiche/quic/core/congestion_control/bbr2_sender.h
+++ b/quiche/quic/core/congestion_control/bbr2_sender.h
@@ -139,6 +139,8 @@
DebugState ExportDebugState() const;
+ const Bbr2NetworkModel& GetNetworkModel() const { return model_; }
+
private:
void UpdatePacingRate(QuicByteCount bytes_acked);
void UpdateCongestionWindow(QuicByteCount bytes_acked);
diff --git a/quiche/quic/core/congestion_control/bbr2_simulator_test.cc b/quiche/quic/core/congestion_control/bbr2_simulator_test.cc
index 9eadce2..f9bc24a 100644
--- a/quiche/quic/core/congestion_control/bbr2_simulator_test.cc
+++ b/quiche/quic/core/congestion_control/bbr2_simulator_test.cc
@@ -15,6 +15,7 @@
#include "quiche/quic/core/quic_bandwidth.h"
#include "quiche/quic/core/quic_packet_number.h"
#include "quiche/quic/core/quic_types.h"
+#include "quiche/quic/platform/api/quic_expect_bug.h"
#include "quiche/quic/platform/api/quic_flags.h"
#include "quiche/quic/platform/api/quic_logging.h"
#include "quiche/quic/platform/api/quic_test.h"
@@ -1799,7 +1800,7 @@
}
now = now + params.RTT();
sender_->OnCongestionEvent(
- /*rtt_updated=*/true, kDefaultMaxPacketSize, now,
+ /*rtt_updated=*/true, 2 * kDefaultMaxPacketSize, now,
{AckedPacket(next_packet_number - 1, kDefaultMaxPacketSize, now)},
{LostPacket(next_packet_number - 2, kDefaultMaxPacketSize)}, 0, 0);
@@ -1845,6 +1846,60 @@
EXPECT_EQ(prior_bandwidth_estimate, sender_->BandwidthEstimate());
}
+// Simulate the case where a packet is considered lost but then acked.
+TEST_F(Bbr2DefaultTopologyTest, SpuriousLossEvent) {
+ DefaultTopologyParams params;
+ CreateNetwork(params);
+
+ DriveOutOfStartup(params);
+
+ // Make sure we have something in flight.
+ if (sender_unacked_map()->bytes_in_flight() == 0) {
+ sender_endpoint_.AddBytesToTransfer(50 * 1024 * 1024);
+ bool simulator_result = simulator_.RunUntilOrTimeout(
+ [&]() { return sender_unacked_map()->bytes_in_flight() > 0; },
+ QuicTime::Delta::FromSeconds(5));
+ ASSERT_TRUE(simulator_result);
+ }
+
+ // Lose all in flight packets.
+ QuicTime now = simulator_.GetClock()->Now() + params.RTT() * 0.25;
+ const QuicByteCount prior_inflight = sender_unacked_map()->bytes_in_flight();
+ LostPacketVector lost_packets;
+ for (QuicPacketNumber packet_number = sender_unacked_map()->GetLeastUnacked();
+ sender_unacked_map()->HasInFlightPackets(); packet_number++) {
+ const auto& info = sender_unacked_map()->GetTransmissionInfo(packet_number);
+ if (!info.in_flight) {
+ continue;
+ }
+ lost_packets.emplace_back(packet_number, info.bytes_sent);
+ sender_unacked_map()->RemoveFromInFlight(packet_number);
+ }
+ ASSERT_FALSE(lost_packets.empty());
+ sender_->OnCongestionEvent(false, prior_inflight, now, {}, lost_packets, 0,
+ 0);
+
+ // Pretend the first lost packet number is acked.
+ now = now + params.RTT() * 0.5;
+ AckedPacketVector acked_packets;
+ acked_packets.emplace_back(lost_packets[0].packet_number, 0, now);
+ acked_packets.back().spurious_loss = true;
+ EXPECT_EQ(sender_unacked_map()->bytes_in_flight(), 0);
+ sender_->OnCongestionEvent(false, sender_unacked_map()->bytes_in_flight(),
+ now, acked_packets, {}, 0, 0);
+
+ if (GetQuicReloadableFlag(quic_bbr2_fix_spurious_loss_bytes_counting)) {
+ EXPECT_EQ(sender_->GetNetworkModel().total_bytes_sent(),
+ sender_->GetNetworkModel().total_bytes_acked() +
+ sender_->GetNetworkModel().total_bytes_lost());
+ } else {
+ // This is the bug fixed by the flag.
+ EXPECT_LT(sender_->GetNetworkModel().total_bytes_sent(),
+ sender_->GetNetworkModel().total_bytes_acked() +
+ sender_->GetNetworkModel().total_bytes_lost());
+ }
+}
+
// After quiescence, if the sender is in PROBE_RTT, it should transition to
// PROBE_BW immediately on the first sent packet after quiescence.
TEST_F(Bbr2DefaultTopologyTest, ProbeRttAfterQuiescenceImmediatelyExits) {
diff --git a/quiche/quic/core/quic_flags_list.h b/quiche/quic/core/quic_flags_list.h
index 3824bff..79a1e21 100644
--- a/quiche/quic/core/quic_flags_list.h
+++ b/quiche/quic/core/quic_flags_list.h
@@ -79,6 +79,8 @@
QUIC_FLAG(quic_reloadable_flag_quic_limit_sending_max_streams2, true)
// If true, enable server retransmittable on wire PING.
QUIC_FLAG(quic_reloadable_flag_quic_enable_server_on_wire_ping, true)
+// If true, fix a QUIC BBR2 bytes counting issue caused by spurious losses.
+QUIC_FLAG(quic_reloadable_flag_quic_bbr2_fix_spurious_loss_bytes_counting, false)
// If true, include stream information in idle timeout connection close detail.
QUIC_FLAG(quic_reloadable_flag_quic_add_stream_info_to_idle_close_detail, true)
// If true, reject or send error response code upon receiving invalid request or response headers.
diff --git a/quiche/quic/core/quic_sent_packet_manager.cc b/quiche/quic/core/quic_sent_packet_manager.cc
index 23e5e06..9dc1048 100644
--- a/quiche/quic/core/quic_sent_packet_manager.cc
+++ b/quiche/quic/core/quic_sent_packet_manager.cc
@@ -1394,6 +1394,7 @@
if (info->in_flight) {
acked_packet.bytes_acked = info->bytes_sent;
} else {
+ acked_packet.spurious_loss = (info->state == LOST);
// Unackable packets are skipped earlier.
largest_newly_acked_ = acked_packet.packet_number;
}
diff --git a/quiche/quic/core/quic_types.h b/quiche/quic/core/quic_types.h
index 0389e74..b626531 100644
--- a/quiche/quic/core/quic_types.h
+++ b/quiche/quic/core/quic_types.h
@@ -571,6 +571,9 @@
QuicPacketNumber packet_number;
// Number of bytes sent in the packet that was acknowledged.
QuicPacketLength bytes_acked;
+ // Whether the packet has been marked as lost before the ack. |bytes_acked|
+ // should be 0 if this is true.
+ bool spurious_loss = false;
// The time |packet_number| was received by the peer, according to the
// optional timestamp the peer included in the ACK frame which acknowledged
// |packet_number|. Zero if no timestamp was available for this packet.
