From adb224b3e9e0df3dbd5c5a49a413cfe66e3ea7d4 Mon Sep 17 00:00:00 2001 From: Victor Boivie Date: Fri, 16 Aug 2024 15:50:16 +0200 Subject: [PATCH] dcsctp: Simplify congestion control algorithm There was a feature in the retransmission queue to avoid fragmenting large messages when the congestion window was large. This was a feature that intended to improve data channel performance under Chrome, where communication with the network process (over MOJO) was lossy and losing messages with small fragments could result in unnecessary retransmissions. But back then, the implementation for fast retransmit wasn't implemented correctly, so the benchmarking result don't reproduce any longer. So just improve the algorithm by removing this code. This aligns it with the RFC and makes it easier to implement pluggable congestion control algorithms (that wouldn't want this feature). Bug: webrtc:42223116 Change-Id: Ifaaa82dac4b8fe2f55418158ae8b3da97199212f Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/359681 Reviewed-by: Florent Castelli Commit-Queue: Victor Boivie Cr-Commit-Position: refs/heads/main@{#42797} --- net/dcsctp/socket/dcsctp_socket_test.cc | 63 ------------------- .../socket/transmission_control_block.cc | 4 +- net/dcsctp/tx/retransmission_queue.cc | 9 --- net/dcsctp/tx/retransmission_queue.h | 6 -- net/dcsctp/tx/retransmission_queue_test.cc | 60 ------------------ 5 files changed, 1 insertion(+), 141 deletions(-) diff --git a/net/dcsctp/socket/dcsctp_socket_test.cc b/net/dcsctp/socket/dcsctp_socket_test.cc index d27b260753..a9d424f4bb 100644 --- a/net/dcsctp/socket/dcsctp_socket_test.cc +++ b/net/dcsctp/socket/dcsctp_socket_test.cc @@ -2141,55 +2141,6 @@ TEST_P(DcSctpSocketParametrizedTest, DoesntSendMoreThanMaxBurstPackets) { MaybeHandoverSocketAndSendMessage(a, std::move(z)); } -TEST_P(DcSctpSocketParametrizedTest, SendsOnlyLargePackets) { - SocketUnderTest a("A"); - auto z = std::make_unique("Z"); - - ConnectSockets(a, *z); - z = MaybeHandoverSocket(std::move(z)); - - // A really large message, to ensure that the congestion window is often full. - constexpr size_t kMessageSize = 100000; - a.socket.Send( - DcSctpMessage(StreamID(1), PPID(53), std::vector(kMessageSize)), - kSendOptions); - - bool delivered_packet = false; - std::vector data_packet_sizes; - do { - delivered_packet = false; - std::vector packet_from_a = a.cb.ConsumeSentPacket(); - if (!packet_from_a.empty()) { - data_packet_sizes.push_back(packet_from_a.size()); - delivered_packet = true; - z->socket.ReceivePacket(std::move(packet_from_a)); - } - std::vector packet_from_z = z->cb.ConsumeSentPacket(); - if (!packet_from_z.empty()) { - delivered_packet = true; - a.socket.ReceivePacket(std::move(packet_from_z)); - } - } while (delivered_packet); - - size_t packet_payload_bytes = - a.options.mtu - SctpPacket::kHeaderSize - DataChunk::kHeaderSize; - // +1 accounts for padding, and rounding up. - size_t expected_packets = - (kMessageSize + packet_payload_bytes - 1) / packet_payload_bytes + 1; - EXPECT_THAT(data_packet_sizes, SizeIs(expected_packets)); - - // Remove the last size - it will be the remainder. But all other sizes should - // be large. - data_packet_sizes.pop_back(); - - for (size_t size : data_packet_sizes) { - // The 4 is for padding/alignment. - EXPECT_GE(size, a.options.mtu - 4); - } - - MaybeHandoverSocketAndSendMessage(a, std::move(z)); -} - TEST(DcSctpSocketTest, SendMessagesAfterHandover) { SocketUnderTest a("A"); auto z = std::make_unique("Z"); @@ -2884,20 +2835,6 @@ TEST(DcSctpSocketTest, ResetStreamsDeferred) { kSuccessPerformed)))))))); a.socket.ReceivePacket(reconfig3); - EXPECT_THAT( - data1, - HasChunks(ElementsAre(IsDataChunk(Property(&DataChunk::ssn, SSN(0)))))); - EXPECT_THAT( - data2, - HasChunks(ElementsAre(IsDataChunk(Property(&DataChunk::ssn, SSN(0)))))); - EXPECT_THAT( - data3, - HasChunks(ElementsAre(IsDataChunk(Property(&DataChunk::ssn, SSN(1)))))); - EXPECT_THAT(reconfig, HasChunks(ElementsAre(IsReConfig(HasParameters( - ElementsAre(IsOutgoingResetRequest(Property( - &OutgoingSSNResetRequestParameter::stream_ids, - ElementsAre(StreamID(1)))))))))); - // Send a new message after the stream has been reset. a.socket.Send(DcSctpMessage(StreamID(1), PPID(55), std::vector(kSmallMessageSize)), diff --git a/net/dcsctp/socket/transmission_control_block.cc b/net/dcsctp/socket/transmission_control_block.cc index e179a8e4ae..cf62c159bf 100644 --- a/net/dcsctp/socket/transmission_control_block.cc +++ b/net/dcsctp/socket/transmission_control_block.cc @@ -210,9 +210,7 @@ void TransmissionControlBlock::MaybeSendFastRetransmit() { void TransmissionControlBlock::SendBufferedPackets(SctpPacket::Builder& builder, Timestamp now) { - for (int packet_idx = 0; - packet_idx < options_.max_burst && retransmission_queue_.can_send_data(); - ++packet_idx) { + for (int packet_idx = 0; packet_idx < options_.max_burst; ++packet_idx) { // Only add control chunks to the first packet that is sent, if sending // multiple packets in one go (as allowed by the congestion window). if (packet_idx == 0) { diff --git a/net/dcsctp/tx/retransmission_queue.cc b/net/dcsctp/tx/retransmission_queue.cc index 8c0d227a36..9a246174b1 100644 --- a/net/dcsctp/tx/retransmission_queue.cc +++ b/net/dcsctp/tx/retransmission_queue.cc @@ -46,9 +46,6 @@ namespace dcsctp { namespace { using ::webrtc::TimeDelta; using ::webrtc::Timestamp; - -// Allow sending only slightly less than an MTU, to account for headers. -constexpr float kMinBytesRequiredToSendFactor = 0.9; } // namespace RetransmissionQueue::RetransmissionQueue( @@ -65,7 +62,6 @@ RetransmissionQueue::RetransmissionQueue( bool use_message_interleaving) : callbacks_(*callbacks), options_(options), - min_bytes_required_to_send_(options.mtu * kMinBytesRequiredToSendFactor), partial_reliability_(supports_partial_reliability), log_prefix_(log_prefix), data_chunk_header_size_(use_message_interleaving @@ -534,11 +530,6 @@ std::vector> RetransmissionQueue::GetChunksToSend( return to_be_sent; } -bool RetransmissionQueue::can_send_data() const { - return cwnd_ < options_.avoid_fragmentation_cwnd_mtus * options_.mtu || - max_bytes_to_send() >= min_bytes_required_to_send_; -} - bool RetransmissionQueue::ShouldSendForwardTsn(Timestamp now) { if (!partial_reliability_) { return false; diff --git a/net/dcsctp/tx/retransmission_queue.h b/net/dcsctp/tx/retransmission_queue.h index a0fbb33c47..15720647bf 100644 --- a/net/dcsctp/tx/retransmission_queue.h +++ b/net/dcsctp/tx/retransmission_queue.h @@ -126,9 +126,6 @@ class RetransmissionQueue { // Returns the number of DATA chunks that are in-flight. size_t unacked_items() const { return outstanding_data_.unacked_items(); } - // Indicates if the congestion control algorithm allows data to be sent. - bool can_send_data() const; - // Given the current time `now`, it will evaluate if there are chunks that // have expired and that need to be discarded. It returns true if a // FORWARD-TSN should be sent. @@ -217,9 +214,6 @@ class RetransmissionQueue { DcSctpSocketCallbacks& callbacks_; const DcSctpOptions options_; - // The minimum bytes required to be available in the congestion window to - // allow packets to be sent - to avoid sending too small packets. - const size_t min_bytes_required_to_send_; // If the peer supports RFC3758 - SCTP Partial Reliability Extension. const bool partial_reliability_; const absl::string_view log_prefix_; diff --git a/net/dcsctp/tx/retransmission_queue_test.cc b/net/dcsctp/tx/retransmission_queue_test.cc index eb1e04a5bb..4c802e4531 100644 --- a/net/dcsctp/tx/retransmission_queue_test.cc +++ b/net/dcsctp/tx/retransmission_queue_test.cc @@ -1405,66 +1405,6 @@ TEST_F(RetransmissionQueueTest, CwndRecoversWhenAcking) { EXPECT_EQ(queue.cwnd(), kCwnd + serialized_size); } -// Verifies that it doesn't produce tiny packets, when getting close to -// the full congestion window. -TEST_F(RetransmissionQueueTest, OnlySendsLargePacketsOnLargeCongestionWindow) { - RetransmissionQueue queue = CreateQueue(); - size_t intial_cwnd = options_.avoid_fragmentation_cwnd_mtus * options_.mtu; - queue.set_cwnd(intial_cwnd); - EXPECT_EQ(queue.cwnd(), intial_cwnd); - - // Fill the congestion window almost - leaving 500 bytes. - size_t chunk_size = intial_cwnd - 500; - EXPECT_CALL(producer_, Produce) - .WillOnce([chunk_size, this](Timestamp, size_t) { - return SendQueue::DataToSend( - OutgoingMessageId(0), - gen_.Ordered(std::vector(chunk_size), "BE")); - }) - .WillRepeatedly([](Timestamp, size_t) { return absl::nullopt; }); - - EXPECT_TRUE(queue.can_send_data()); - std::vector> chunks_to_send = - queue.GetChunksToSend(now_, 10000); - EXPECT_THAT(chunks_to_send, ElementsAre(Pair(TSN(10), _))); - - // To little space left - will not send more. - EXPECT_FALSE(queue.can_send_data()); - - // But when the first chunk is acked, it will continue. - queue.HandleSack(now_, SackChunk(TSN(10), kArwnd, {}, {})); - - EXPECT_TRUE(queue.can_send_data()); - EXPECT_EQ(queue.unacked_bytes(), 0u); - EXPECT_EQ(queue.cwnd(), intial_cwnd + kMaxMtu); -} - -TEST_F(RetransmissionQueueTest, AllowsSmallFragmentsOnSmallCongestionWindow) { - RetransmissionQueue queue = CreateQueue(); - size_t intial_cwnd = - options_.avoid_fragmentation_cwnd_mtus * options_.mtu - 1; - queue.set_cwnd(intial_cwnd); - EXPECT_EQ(queue.cwnd(), intial_cwnd); - - // Fill the congestion window almost - leaving 500 bytes. - size_t chunk_size = intial_cwnd - 500; - EXPECT_CALL(producer_, Produce) - .WillOnce([chunk_size, this](Timestamp, size_t) { - return SendQueue::DataToSend( - OutgoingMessageId(0), - gen_.Ordered(std::vector(chunk_size), "BE")); - }) - .WillRepeatedly([](Timestamp, size_t) { return absl::nullopt; }); - - EXPECT_TRUE(queue.can_send_data()); - std::vector> chunks_to_send = - queue.GetChunksToSend(now_, 10000); - EXPECT_THAT(chunks_to_send, ElementsAre(Pair(TSN(10), _))); - - // With congestion window under limit, allow small packets to be created. - EXPECT_TRUE(queue.can_send_data()); -} - TEST_F(RetransmissionQueueTest, ReadyForHandoverWhenHasNoOutstandingData) { RetransmissionQueue queue = CreateQueue(); EXPECT_CALL(producer_, Produce)