fix(s2n-quic-transport): only allow GSO of MTU-sized packets (#1613)

* fix(s2n-quic-transport): only allow GSO of MTU-sized packets * increase post-connection allocation limit * Update quic/s2n-quic-transport/src/connection/transmission.rs Co-authored-by: Wesley Rosenblum <[email protected]> --------- Co-authored-by: Wesley Rosenblum <[email protected]>
aws · Jan 28, 2023 · ef0f3db · ef0f3db
1 parent a8954cf
commit ef0f3db
Show file tree

Hide file tree

Showing 7 changed files with 38 additions and 23 deletions.
diff --git a/quic/s2n-quic-platform/src/buffer/vec.rs b/quic/s2n-quic-platform/src/buffer/vec.rs
@@ -6,10 +6,18 @@ use core::{
     fmt,
     ops::{Deref, DerefMut},
 };
+use lazy_static::lazy_static;
 use s2n_quic_core::path::DEFAULT_MAX_MTU;
 
 // TODO decide on better defaults
-const DEFAULT_MESSAGE_COUNT: usize = 1024;
+lazy_static! {
+    static ref DEFAULT_MESSAGE_COUNT: usize = {
+        std::env::var("S2N_UNSTABLE_DEFAULT_MESSAGE_COUNT")
+            .ok()
+            .and_then(|v| v.parse().ok())
+            .unwrap_or(1024 * 2)
+    };
+}
 
 pub struct VecBuffer {
     region: alloc::vec::Vec<u8>,
@@ -33,7 +41,7 @@ impl Default for VecBuffer {
         if cfg!(test) {
             Self::new(64, 1200)
         } else {
-            Self::new(DEFAULT_MESSAGE_COUNT, DEFAULT_MAX_MTU.into())
+            Self::new(*DEFAULT_MESSAGE_COUNT, DEFAULT_MAX_MTU.into())
         }
     }
 }

diff --git a/quic/s2n-quic-transport/src/connection/connection_container.rs b/quic/s2n-quic-transport/src/connection/connection_container.rs
@@ -645,7 +645,7 @@ macro_rules! iterate_interruptible {
             }
 
             match result {
-                ConnectionContainerIterationResult::BreakAndInsertAtBack => {
+                ConnectionContainerIterationResult::BreakAndInsertAtFront => {
                     $sel.interest_lists
                         .$list_name
                         .front_mut()
@@ -1072,11 +1072,12 @@ impl<C: connection::Trait, L: connection::Lock<C>> ConnectionContainer<C, L> {
 /// The value instructs the iterator whether iteration will be continued.
 #[derive(Clone, Copy, Debug)]
 pub enum ConnectionContainerIterationResult {
-    /// Continue iteration over the list
+    /// Continue iteration over the list and insert the current connection
+    /// to the back
     Continue,
     /// Aborts the iteration over a list and add the remaining items at the
-    /// back of the list
-    BreakAndInsertAtBack,
+    /// front of the list
+    BreakAndInsertAtFront,
 }
 
 #[cfg(test)]

diff --git a/quic/s2n-quic-transport/src/connection/connection_container/tests.rs b/quic/s2n-quic-transport/src/connection/connection_container/tests.rs
@@ -517,7 +517,7 @@ fn container_test() {
                         assert!(conn.interests.transmission);
 
                         if count == 0 {
-                            ConnectionContainerIterationResult::BreakAndInsertAtBack
+                            ConnectionContainerIterationResult::BreakAndInsertAtFront
                         } else {
                             count -= 1;
                             ConnectionContainerIterationResult::Continue
@@ -530,7 +530,7 @@ fn container_test() {
                         assert!(conn.interests.new_connection_id);
 
                         if count == 0 {
-                            ConnectionContainerIterationResult::BreakAndInsertAtBack
+                            ConnectionContainerIterationResult::BreakAndInsertAtFront
                         } else {
                             count -= 1;
                             ConnectionContainerIterationResult::Continue

diff --git a/quic/s2n-quic-transport/src/connection/transmission.rs b/quic/s2n-quic-transport/src/connection/transmission.rs
@@ -87,10 +87,9 @@ impl<'a, 'sub, Config: endpoint::Config> tx::Message for ConnectionTransmission<
         }
 
         // If a packet can be GSO'd it means it's limited to the previously written packet
-        // size. This becomes a problem for MTU probes where they will likely exceed that amount.
-        // As such, if we're probing we want to let the IO layer know to not GSO the current
-        // packet.
-        !self.context.transmission_mode.is_mtu_probing()
+        // size. We want to avoid sending several small packets and artificially clamping packets to
+        // less than an MTU.
+        segment_len >= self.context.path().mtu(self.context.transmission_mode)
     }
 
     fn write_payload(

diff --git a/quic/s2n-quic-transport/src/endpoint/mod.rs b/quic/s2n-quic-transport/src/endpoint/mod.rs
@@ -131,28 +131,35 @@ impl<Cfg: Config> s2n_quic_core::endpoint::Endpoint for Endpoint<Cfg> {
     {
         self.on_timeout(clock.get_time());
 
+        // the queue doesn't have the capacity for transmissions so don't try to push
+        if !queue.has_capacity() {
+            return;
+        }
+
         // Iterate over all connections which want to transmit data
-        let mut transmit_result = Ok(());
         let endpoint_context = self.config.context();
 
         let timestamp = clock.get_time();
 
         self.connections.iterate_transmission_list(|connection| {
-            transmit_result = connection.on_transmit(
+            // if we no longer have capacity, then put the connection at the front of the queue for
+            // next time
+            if !queue.has_capacity() {
+                return ConnectionContainerIterationResult::BreakAndInsertAtFront;
+            }
+
+            // ignore the transmission error and just query the queue capacity instead
+            let _ = connection.on_transmit(
                 queue,
                 timestamp,
                 endpoint_context.event_subscriber,
                 endpoint_context.packet_interceptor,
             );
-            if transmit_result.is_err() {
-                // If one connection fails, return
-                ConnectionContainerIterationResult::BreakAndInsertAtBack
-            } else {
-                ConnectionContainerIterationResult::Continue
-            }
+
+            ConnectionContainerIterationResult::Continue
         });
 
-        if transmit_result.is_ok() {
+        if queue.has_capacity() {
             let mut publisher = event::EndpointPublisherSubscriber::new(
                 event::builder::EndpointMeta {
                     endpoint_type: Cfg::ENDPOINT_TYPE,

diff --git a/quic/s2n-quic-transport/src/path/mod.rs b/quic/s2n-quic-transport/src/path/mod.rs
@@ -406,7 +406,7 @@ impl<Config: endpoint::Config> Path<Config> {
     }
 
     #[inline]
-    fn mtu(&self, transmission_mode: transmission::Mode) -> usize {
+    pub fn mtu(&self, transmission_mode: transmission::Mode) -> usize {
         match transmission_mode {
             // Use the minimum MTU for loss recovery probes to allow detection of packets
             // lost when the previously confirmed path MTU is no longer supported.

diff --git a/tools/memory-report/src/main.rs b/tools/memory-report/src/main.rs
@@ -34,7 +34,7 @@ impl Snapshot {
                     assert!(rss < 30_000, "{rss}");
                 }
                 "post-close" => {
-                    assert_eq!(rss, 0, "{rss}");
+                    assert!(rss < 128, "{rss}");
                 }
                 e => unimplemented!("{}", e),
             }