quinn-proto: avoid unnecessary PING when a DATAGRAM fits into a tail-loss probe

quinn-proto/src/connection/mod.rs

@@ -475,9 +475,19 @@ impl Connection {
 
         // If we need to send a probe, make sure we have something to send.
         for space in SpaceId::iter() {
+            let has_ack_eliciting_data = space == SpaceId::Data && {
+                let pn = self.packet_number_filter.peek(&self.spaces[SpaceId::Data]);
+                let probe_size = segment_size.min(usize::from(INITIAL_MTU));
+                let frame_space = probe_size.saturating_sub(self.predict_1rtt_overhead(Some(pn)));
+                self.can_send_1rtt(frame_space)
+            };
             let request_immediate_ack =
                 space == SpaceId::Data && self.peer_supports_ack_frequency();
-            self.spaces[space].maybe_queue_probe(request_immediate_ack, &self.streams);
+            self.spaces[space].maybe_queue_probe(
+                request_immediate_ack,
+                has_ack_eliciting_data,
+                &self.streams,
+            );
         }
 
         // Check whether we need to send a close message
@@ -3592,6 +3602,11 @@ impl Connection {
         self.peer_params.min_ack_delay.is_some()
     }
 
+    #[cfg(test)]
+    pub(crate) fn disable_peer_ack_frequency(&mut self) {
+        self.peer_params.min_ack_delay = None;
+    }
+
     /// Send an IMMEDIATE_ACK frame to the remote endpoint
     ///
     /// According to the spec, this will result in an error if the remote endpoint does not support

quinn-proto/src/connection/spaces.rs

@@ -119,6 +119,7 @@ impl PacketSpace {
     pub(super) fn maybe_queue_probe(
         &mut self,
         request_immediate_ack: bool,
+        has_ack_eliciting_data: bool,
         streams: &StreamsState,
     ) {
         if self.loss_probes == 0 {
@@ -136,6 +137,11 @@ impl PacketSpace {
             return;
         }
 
+        if has_ack_eliciting_data {
+            // New data can make the probe ack-eliciting, so no fallback frame is needed.
+            return;
+        }
+
         // Retransmit the data of the oldest in-flight packet
         for packet in self.sent_packets.values_mut() {
             if !packet.retransmits.is_empty(streams) {

quinn-proto/src/tests/mod.rs

@@ -1917,6 +1917,7 @@ fn tail_loss_small_segment_size() {
     let _guard = subscribe();
     let mut pair = Pair::default();
     let (client_ch, server_ch) = pair.connect();
+    pair.client_conn_mut(client_ch).disable_peer_ack_frequency();
 
     // No datagrams frames received in the handshake.
     let server_stats = pair.server_conn_mut(server_ch).stats();
@@ -1936,6 +1937,7 @@ fn tail_loss_small_segment_size() {
     // Doing one step makes the client advance time to the PTO fire time.
     info!("stepping forward to PTO");
     pair.step();
+    let ping_count = pair.client_conn_mut(client_ch).stats().frame_tx.ping;
 
     // Still no datagrams frames received by the server.
     let server_stats = pair.server_conn_mut(server_ch).stats();
@@ -1958,6 +1960,59 @@ fn tail_loss_small_segment_size() {
     // Finally the server should have received some datagrams.
     let server_stats = pair.server_conn_mut(server_ch).stats();
     assert_eq!(server_stats.frame_rx.datagram, DGRAM_NUM);
+
+    // DATAGRAM frames are ack-eliciting, so the loss probe does not need an additional PING.
+    let client_stats = pair.client_conn_mut(client_ch).stats();
+    assert_eq!(client_stats.frame_tx.ping, ping_count);
+}
+
+#[test]
+fn tail_loss_probe_keeps_ping_when_datagram_does_not_fit() {
+    let _guard = subscribe();
+
+    const PATH_MTU: u16 = 1452;
+
+    let client_config = {
+        let mut config = client_config();
+        Arc::get_mut(&mut config.transport)
+            .unwrap()
+            .initial_mtu(PATH_MTU)
+            .mtu_discovery_config(None);
+        config
+    };
+
+    let mut pair = Pair::default();
+    pair.mtu = PATH_MTU as usize;
+    let (client_ch, server_ch) = pair.connect_with(client_config);
+
+    pair.client_conn_mut(client_ch).disable_peer_ack_frequency();
+    assert_eq!(pair.client_conn_mut(client_ch).path_mtu(), PATH_MTU);
+
+    // Establish an outstanding ack-eliciting packet and discard it.
+    pair.client_conn_mut(client_ch).ping();
+    pair.drive_client();
+    assert!(!pair.server.inbound.is_empty());
+    pair.server.inbound.clear();
+
+    // Advance to the PTO without transmitting the queued loss probe yet.
+    pair.step();
+    let ping_count = pair.client_conn_mut(client_ch).stats().frame_tx.ping;
+
+    // This fits the normal path MTU, but not a loss probe capped to INITIAL_MTU.
+    let datagram_len = pair.client_datagrams(client_ch).max_size().unwrap();
+    assert!(datagram_len > INITIAL_MTU as usize);
+    pair.client_datagrams(client_ch)
+        .send(vec![0; datagram_len].into(), false)
+        .unwrap();
+
+    pair.drive();
+
+    // The probe must retain its PING when the queued DATAGRAM cannot fit in that packet.
+    let client_stats = pair.client_conn_mut(client_ch).stats();
+    assert_eq!(client_stats.frame_tx.ping, ping_count + 2);
+
+    let server_stats = pair.server_conn_mut(server_ch).stats();
+    assert_eq!(server_stats.frame_rx.datagram, 1);
 }
 
 // Respect max_datagrams when TLP happens