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.