From ef0f3db57d69c314912a8f7640035e97de156544 Mon Sep 17 00:00:00 2001 From: Cameron Bytheway Date: Fri, 27 Jan 2023 17:07:08 -0700 Subject: [PATCH] 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 <55108558+WesleyRosenblum@users.noreply.github.com> --------- Co-authored-by: Wesley Rosenblum <55108558+WesleyRosenblum@users.noreply.github.com> --- quic/s2n-quic-platform/src/buffer/vec.rs | 12 +++++++-- .../src/connection/connection_container.rs | 9 ++++--- .../connection/connection_container/tests.rs | 4 +-- .../src/connection/transmission.rs | 7 +++--- quic/s2n-quic-transport/src/endpoint/mod.rs | 25 ++++++++++++------- quic/s2n-quic-transport/src/path/mod.rs | 2 +- tools/memory-report/src/main.rs | 2 +- 7 files changed, 38 insertions(+), 23 deletions(-) diff --git a/quic/s2n-quic-platform/src/buffer/vec.rs b/quic/s2n-quic-platform/src/buffer/vec.rs index 78c6b9fa44..6a26f9d3f9 100644 --- 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, @@ -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 index d17f55515f..167df2ee15 100644 --- 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> ConnectionContainer { /// 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 index 5bf90c65de..13d8a25dec 100644 --- 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 index 2f514a784b..9421c1b64c 100644 --- 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 index 75fcb24d14..9e1aad6193 100644 --- a/quic/s2n-quic-transport/src/endpoint/mod.rs +++ b/quic/s2n-quic-transport/src/endpoint/mod.rs @@ -131,28 +131,35 @@ impl s2n_quic_core::endpoint::Endpoint for Endpoint { { 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 index 903e3c71e6..2b1b7a3dff 100644 --- a/quic/s2n-quic-transport/src/path/mod.rs +++ b/quic/s2n-quic-transport/src/path/mod.rs @@ -406,7 +406,7 @@ impl Path { } #[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 index beacee9026..650a583a38 100644 --- 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), }