Repository: quinn-rs/quinn
Pull request #2185 (2025-03-23 19:45:47)

fix: respecting max_datagrams in poll_transmit
Hello,

I've been playing with quinn-proto recently and I noticed that, sometimes, when the maximum segment is large enough quinn will go over the max_datagrams limit of poll_transmit. I believe this PR will fix this issue.

Also, considering that quin-proto is a low level crate, it feels like this code:
https://github.com/quinn-rs/quinn/blob/434c35861e68aac1da568bcd0b1523603f73f255/quinn-proto/src/connection/mod.rs#L454-L457
should be removed and left for the upper level code to decide what max limit they truly want to use.



Ralith: > Also, considering that quin-proto is a low level crate, it feels like this code:

That code exists so that the application layer can easily opt out of GSO without needing to replace the UDP socket abstraction.
filipe-cantarelli: > > Also, considering that quin-proto is a low level crate, it feels like this code:
> 
> That code exists so that the application layer can easily opt out of GSO without needing to replace the UDP socket abstraction.

How about `max_datagrams.min(MAX_TRANSMIT_SEGMENTS)`, is it possible to remove this limitation so the app code request more than 10 segments?
Ralith: Oh, you were concerned about the upper bound? We could probably get rid of that entirely, yeah.
filipe-cantarelli: > Oh, you were concerned about the upper bound? We could probably get rid of that entirely, yeah.

Is this something that could be done here in this PR?
djc: (Be sure to rebase on main to appease CI.)
filipe-cantarelli: Could I then move the 10 limit to quinn in this PR and then open an issue to follow up relaxing it? I could maybe work on that later.

As far as where to cap it, maybe here is a good place:
https://github.com/quinn-rs/quinn/blob/main/quinn/src/connection.rs#L984

Ralith: > Could I then move the 10 limit to quinn in this PR and then open an issue to follow up relaxing it? I could maybe work on that later.

SGTM, thanks!

> As far as where to cap it, maybe here is a good place:

Yep, that's the spot.
filipe-cantarelli: @Ralith Follow up issue created https://github.com/quinn-rs/quinn/issues/2189
filipe-cantarelli: @djc, @Ralith what are the next steps for this pr?
djc: Would be good to get another round of review from @Ralith before merging.

Patch:
diff --git a/quinn-proto/src/connection/mod.rs b/quinn-proto/src/connection/mod.rs
index 3ddf2b97..9f10ec10 100644
--- a/quinn-proto/src/connection/mod.rs
+++ b/quinn-proto/src/connection/mod.rs
@@ -453,7 +453,7 @@ impl Connection {
         assert!(max_datagrams != 0);
         let max_datagrams = match self.config.enable_segmentation_offload {
             false => 1,
-            true => max_datagrams.min(MAX_TRANSMIT_SEGMENTS),
+            true => max_datagrams,
         };
 
         let mut num_datagrams = 0;
@@ -564,7 +564,7 @@ impl Connection {
                 // We need to send 1 more datagram and extend the buffer for that.
 
                 // Is 1 more datagram allowed?
-                if buf_capacity >= segment_size * max_datagrams {
+                if num_datagrams >= max_datagrams {
                     // No more datagrams allowed
                     break;
                 }
@@ -577,7 +577,7 @@ impl Connection {
                 // (see https://github.com/quinn-rs/quinn/issues/1082)
                 if self
                     .path
-                    .anti_amplification_blocked(segment_size as u64 * num_datagrams + 1)
+                    .anti_amplification_blocked(segment_size as u64 * (num_datagrams as u64) + 1)
                 {
                     trace!("blocked by anti-amplification");
                     break;
@@ -965,7 +965,7 @@ impl Connection {
         trace!("sending {} bytes in {} datagrams", buf.len(), num_datagrams);
         self.path.total_sent = self.path.total_sent.saturating_add(buf.len() as u64);
 
-        self.stats.udp_tx.on_sent(num_datagrams, buf.len());
+        self.stats.udp_tx.on_sent(num_datagrams as u64, buf.len());
 
         Some(Transmit {
             destination: self.path.remote,
@@ -3950,13 +3950,6 @@ const MIN_PACKET_SPACE: usize = MAX_HANDSHAKE_OR_0RTT_HEADER_SIZE + 32;
 const MAX_HANDSHAKE_OR_0RTT_HEADER_SIZE: usize =
     1 + 4 + 1 + MAX_CID_SIZE + 1 + MAX_CID_SIZE + VarInt::from_u32(u16::MAX as u32).size() + 4;
 
-/// The maximum amount of datagrams that are sent in a single transmit
-///
-/// This can be lower than the maximum platform capabilities, to avoid excessive
-/// memory allocations when calling `poll_transmit()`. Benchmarks have shown
-/// that numbers around 10 are a good compromise.
-const MAX_TRANSMIT_SEGMENTS: usize = 10;
-
 /// Perform key updates this many packets before the AEAD confidentiality limit.
 ///
 /// Chosen arbitrarily, intended to be large enough to prevent spurious connection loss.
diff --git a/quinn/src/connection.rs b/quinn/src/connection.rs
index 983b695e..5fef2f99 100644
--- a/quinn/src/connection.rs
+++ b/quinn/src/connection.rs
@@ -981,7 +981,10 @@ impl State {
         let now = self.runtime.now();
         let mut transmits = 0;
 
-        let max_datagrams = self.socket.max_transmit_segments();
+        let max_datagrams = self
+            .socket
+            .max_transmit_segments()
+            .min(MAX_TRANSMIT_SEGMENTS);
 
         loop {
             // Retry the last transmit, or get a new one.
@@ -1288,3 +1291,10 @@ pub enum SendDatagramError {
 /// This limits the amount of CPU resources consumed by datagram generation,
 /// and allows other tasks (like receiving ACKs) to run in between.
 const MAX_TRANSMIT_DATAGRAMS: usize = 20;
+
+/// The maximum amount of datagrams that are sent in a single transmit
+///
+/// This can be lower than the maximum platform capabilities, to avoid excessive
+/// memory allocations when calling `poll_transmit()`. Benchmarks have shown
+/// that numbers around 10 are a good compromise.
+const MAX_TRANSMIT_SEGMENTS: usize = 10;


Test patch:
diff --git a/quinn-proto/src/tests/mod.rs b/quinn-proto/src/tests/mod.rs
index ef1893cc..af949d87 100644
--- a/quinn-proto/src/tests/mod.rs
+++ b/quinn-proto/src/tests/mod.rs
@@ -1829,6 +1829,51 @@ fn tail_loss_small_segment_size() {
     assert_eq!(server_stats.frame_rx.datagram, DGRAM_NUM);
 }
 
+// Respect max_datagrams when TLP happens
+#[test]
+fn tail_loss_respect_max_datagrams() {
+    let _guard = subscribe();
+    let client_config = {
+        let mut c_config = client_config();
+        let mut t_config = TransportConfig::default();
+        //Disabling GSO, so only a single segment should be sent per iops
+        t_config.enable_segmentation_offload(false);
+        c_config.transport_config(t_config.into());
+        c_config
+    };
+    let mut pair = Pair::default();
+    let (client_ch, _) = pair.connect_with(client_config);
+
+    const DGRAM_LEN: usize = 1000; // High enough so GSO batch could be built
+    const DGRAM_NUM: u64 = 5; // Enough to build a GSO batch.
+
+    info!("Sending an ack-eliciting datagram");
+    pair.client_conn_mut(client_ch).ping();
+    pair.drive_client();
+
+    // Drop these packets on the server side.
+    assert!(!pair.server.inbound.is_empty());
+    pair.server.inbound.clear();
+
+    // Doing one step makes the client advance time to the PTO fire time.
+    info!("stepping forward to PTO");
+    pair.step();
+
+    // start sending datagram batches but the first should be a TLP
+    info!("Sending datagram batch");
+    for _ in 0..DGRAM_NUM {
+        pair.client_datagrams(client_ch)
+            .send(vec![0; DGRAM_LEN].into(), false)
+            .unwrap();
+    }
+
+    pair.drive();
+
+    // Finally checking the number of sent udp datagrams match the number of iops
+    let client_stats = pair.client_conn_mut(client_ch).stats();
+    assert_eq!(client_stats.udp_tx.ios, client_stats.udp_tx.datagrams);
+}
+
 #[test]
 fn datagram_send_recv() {
     let _guard = subscribe();