diff --git a/quinn-proto/src/connection/mod.rs b/quinn-proto/src/connection/mod.rs index 6986d6314..d6368ca1b 100644 --- a/quinn-proto/src/connection/mod.rs +++ b/quinn-proto/src/connection/mod.rs @@ -32,7 +32,7 @@ use crate::{ token::ResetToken, transport_parameters::TransportParameters, Dir, Duration, EndpointConfig, Frame, Instant, Side, StreamId, Transmit, TransportError, - TransportErrorCode, VarInt, MAX_CID_SIZE, MAX_STREAM_COUNT, MIN_INITIAL_SIZE, + TransportErrorCode, VarInt, INITIAL_MTU, MAX_CID_SIZE, MAX_STREAM_COUNT, MIN_INITIAL_SIZE, TIMER_GRANULARITY, }; @@ -692,13 +692,21 @@ impl Connection { // waste large amounts of bandwidth. The exact threshold is a bit arbitrary // and might benefit from further tuning, though there's no universally // optimal value. + // + // Additionally, if this datagram is a loss probe and `segment_size` is + // larger than `INITIAL_MTU`, then padding it to `segment_size` to continue + // the GSO batch would risk failure to recover from a reduction in path + // MTU. Loss probes are the only packets for which we might grow + // `buf_capacity` by less than `segment_size`. const MAX_PADDING: usize = 16; let packet_len_unpadded = cmp::max(builder.min_size, buf.len()) - datagram_start + builder.tag_len; - if packet_len_unpadded + MAX_PADDING < segment_size { + if packet_len_unpadded + MAX_PADDING < segment_size + || datagram_start + segment_size > buf_capacity + { trace!( - "GSO truncated by demand for {} padding bytes", + "GSO truncated by demand for {} padding bytes or loss probe", segment_size - packet_len_unpadded ); builder_storage = Some(builder); @@ -741,7 +749,17 @@ impl Connection { } // Allocate space for another datagram - buf_capacity += segment_size; + let next_datagram_size_limit = match self.spaces[space_id].loss_probes { + 0 => segment_size, + _ => { + self.spaces[space_id].loss_probes -= 1; + // Clamp the datagram to at most the minimum MTU to ensure that loss probes + // can get through and enable recovery even if the path MTU has shrank + // unexpectedly. + usize::from(INITIAL_MTU) + } + }; + buf_capacity += next_datagram_size_limit; if buf.capacity() < buf_capacity { // We reserve the maximum space for sending `max_datagrams` upfront // to avoid any reallocations if more datagrams have to be appended later on. diff --git a/quinn-proto/src/connection/packet_builder.rs b/quinn-proto/src/connection/packet_builder.rs index bc1cd7ffd..868a8c7ca 100644 --- a/quinn-proto/src/connection/packet_builder.rs +++ b/quinn-proto/src/connection/packet_builder.rs @@ -1,5 +1,3 @@ -use std::cmp; - use bytes::Bytes; use rand::Rng; use tracing::{trace, trace_span}; @@ -8,7 +6,7 @@ use super::{spaces::SentPacket, Connection, SentFrames}; use crate::{ frame::{self, Close}, packet::{Header, InitialHeader, LongType, PacketNumber, PartialEncode, SpaceId, FIXED_BIT}, - ConnectionId, Instant, TransportError, TransportErrorCode, INITIAL_MTU, + ConnectionId, Instant, TransportError, TransportErrorCode, }; pub(super) struct PacketBuilder { @@ -38,7 +36,7 @@ impl PacketBuilder { space_id: SpaceId, dst_cid: ConnectionId, buffer: &mut Vec, - mut buffer_capacity: usize, + buffer_capacity: usize, datagram_start: usize, ack_eliciting: bool, conn: &mut Connection, @@ -79,13 +77,6 @@ impl PacketBuilder { } let space = &mut conn.spaces[space_id]; - - if space.loss_probes != 0 { - space.loss_probes -= 1; - // Clamp the packet size to at most the minimum MTU to ensure that loss probes can get - // through and enable recovery even if the path MTU has shrank unexpectedly. - buffer_capacity = cmp::min(buffer_capacity, datagram_start + usize::from(INITIAL_MTU)); - } let exact_number = match space_id { SpaceId::Data => conn.packet_number_filter.allocate(&mut conn.rng, space), _ => space.get_tx_number(),