Skip to content

Commit

Permalink
refactor transport parameter
Browse files Browse the repository at this point in the history
  • Loading branch information
@sterlingdeng
sterlingdeng committed Oct 1, 2024
1 parent 1cc4043 commit 67af041
Show file tree
Hide file tree
Showing 3 changed files with 62 additions and 73 deletions.
16 changes: 9 additions & 7 deletions quinn-proto/src/config.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ pub struct TransportConfig {
pub(crate) min_mtu: u16,
pub(crate) mtu_discovery_config: Option<MtuDiscoveryConfig>,
pub(crate) ack_frequency_config: Option<AckFrequencyConfig>,
pub(crate) ack_timestamp_config: Option<AckTimestampsConfig>,
pub(crate) ack_timestamp_config: AckTimestampsConfig,

pub(crate) persistent_congestion_threshold: u32,
pub(crate) keep_alive_interval: Option<Duration>,
Expand Down Expand Up @@ -228,7 +228,7 @@ impl TransportConfig {
/// Defaults to `None`, which disables receiving acknowledgement timestamps from the sender.
/// If `Some`, TransportParameters are sent to the peer to enable acknowledgement timestamps
/// if supported.
pub fn ack_timestamp_config(&mut self, value: Option<AckTimestampsConfig>) -> &mut Self {
pub fn ack_timestamp_config(&mut self, value: AckTimestampsConfig) -> &mut Self {
self.ack_timestamp_config = value;
self
}
Expand Down Expand Up @@ -371,7 +371,7 @@ impl Default for TransportConfig {

enable_segmentation_offload: true,

ack_timestamp_config: None,
ack_timestamp_config: AckTimestampsConfig::default(),
}
}
}
Expand Down Expand Up @@ -435,16 +435,17 @@ impl fmt::Debug for TransportConfig {
}

/// Parameters for controlling the peer's acknowledgements with receiver timestamps.
#[derive(Clone, Debug)]
#[derive(Clone, Debug, PartialEq, Eq, Copy)]
pub struct AckTimestampsConfig {
pub(crate) max_timestamps_per_ack: VarInt,
/// If max_timestamp_per_ack is None, this feature is disabled.
pub(crate) max_timestamps_per_ack: Option<VarInt>,
pub(crate) exponent: VarInt,
}

impl AckTimestampsConfig {
/// Sets the maximum number of timestamp entries per ACK frame.
pub fn max_timestamps_per_ack(&mut self, value: VarInt) -> &mut Self {
self.max_timestamps_per_ack = value;
self.max_timestamps_per_ack = Some(value);
self
}

Expand All @@ -459,7 +460,8 @@ impl AckTimestampsConfig {
impl Default for AckTimestampsConfig {
fn default() -> Self {
Self {
max_timestamps_per_ack: 10u32.into(),
max_timestamps_per_ack: None,
// Default to 0 as per draft.
exponent: 0u32.into(),
}
}
Expand Down
66 changes: 32 additions & 34 deletions quinn-proto/src/connection/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -231,7 +231,7 @@ pub struct Connection {

// Ack Receive Timestamps
// The timestamp config of the peer.
ack_timestamp_cfg: Option<AckTimestampsConfig>,
ack_timestamp_cfg: AckTimestampsConfig,

streams: StreamsState,
/// Surplus remote CIDs for future use on new paths
Expand Down Expand Up @@ -345,7 +345,7 @@ impl Connection {
&TransportParameters::default(),
)),

ack_timestamp_cfg: None,
ack_timestamp_cfg: AckTimestampsConfig::default(),

pto_count: 0,

Expand Down Expand Up @@ -828,7 +828,7 @@ impl Connection {
&mut self.spaces[space_id],
buf,
&mut self.stats,
None,
self.ack_timestamp_cfg,
self.epoch,
);
}
Expand Down Expand Up @@ -1357,7 +1357,13 @@ impl Connection {
return Err(TransportError::PROTOCOL_VIOLATION("unsent packet acked"));
}

if ack.timestamps.is_some() != self.config.ack_timestamp_config.is_some() {
if ack.timestamps.is_some()
!= self
.config
.ack_timestamp_config
.max_timestamps_per_ack
.is_some()
{
return Err(TransportError::PROTOCOL_VIOLATION(
"ack with timestamps expectation mismatched",
));
Expand Down Expand Up @@ -1391,13 +1397,18 @@ impl Connection {
}
}

let mut timestamp_iter = self.config.ack_timestamp_config.as_ref().map(|cfg| {
let decoder = ack.timestamp_iter(self.epoch, cfg.exponent.0).unwrap();
let mut v: tinyvec::TinyVec<[PacketTimestamp; 10]> = tinyvec::TinyVec::new();
decoder.for_each(|elt| v.push(elt));
v.reverse();
v.into_iter().peekable()
});
let mut timestamp_iter =
if let Some(_) = self.config.ack_timestamp_config.max_timestamps_per_ack {
let decoder = ack
.timestamp_iter(self.epoch, self.config.ack_timestamp_config.exponent.0)
.unwrap();
let mut v: tinyvec::TinyVec<[PacketTimestamp; 10]> = tinyvec::TinyVec::new();
decoder.for_each(|elt| v.push(elt));
v.reverse();
Some(v.into_iter().peekable())
} else {
None
};

if newly_acked.is_empty() {
return Ok(());
Expand Down Expand Up @@ -3290,7 +3301,7 @@ impl Connection {
space: &mut PacketSpace,
buf: &mut Vec<u8>,
stats: &mut ConnectionStats,
peer_timestamp_config: Option<AckTimestampsConfig>,
ack_timestamps_config: AckTimestampsConfig,
epoch: Instant,
) {
debug_assert!(!space.pending_acks.ranges().is_empty());
Expand Down Expand Up @@ -3320,13 +3331,13 @@ impl Connection {
delay as _,
space.pending_acks.ranges(),
ecn,
peer_timestamp_config.map(|cfg| {
ack_timestamps_config.max_timestamps_per_ack.map(|max| {
(
// Safety: If peer_timestamp_config is set, receiver_timestamps must be set.
space.pending_acks.receiver_timestamps_as_ref().unwrap(),
epoch,
cfg.exponent.0,
cfg.max_timestamps_per_ack.0,
ack_timestamps_config.exponent.0,
max.0,
)
}),
buf,
Expand Down Expand Up @@ -3385,25 +3396,12 @@ impl Connection {
self.path.mtud.on_peer_max_udp_payload_size_received(
u16::try_from(self.peer_params.max_udp_payload_size.into_inner()).unwrap_or(u16::MAX),
);

{
self.ack_timestamp_cfg = if let (Some(max_timestamps_per_ack), Some(exponent)) = (
params.max_recv_timestamps_per_ack,
params.receive_timestamps_exponent,
) {
for space in self.spaces.iter_mut() {
space
.pending_acks
.set_receiver_timestamp(max_timestamps_per_ack.0 as usize);
}
Some(AckTimestampsConfig {
exponent,
max_timestamps_per_ack,
})
} else {
None
};
}
self.ack_timestamp_cfg = params.ack_timestamps_cfg;
if let Some(max) = params.ack_timestamps_cfg.max_timestamps_per_ack {
for space in self.spaces.iter_mut() {
space.pending_acks.set_receiver_timestamp(max.0 as usize);
}
};
}

fn decrypt_packet(
Expand Down
53 changes: 21 additions & 32 deletions quinn-proto/src/transport_parameters.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ use crate::{
cid_generator::ConnectionIdGenerator,
cid_queue::CidQueue,
coding::{BufExt, BufMutExt, UnexpectedEnd},
config::{EndpointConfig, ServerConfig, TransportConfig},
config::{AckTimestampsConfig, EndpointConfig, ServerConfig, TransportConfig},
shared::ConnectionId,
ResetToken, Side, TransportError, VarInt, LOC_CID_COUNT, MAX_CID_SIZE, MAX_STREAM_COUNT,
RESET_TOKEN_SIZE, TIMER_GRANULARITY,
Expand Down Expand Up @@ -101,8 +101,7 @@ macro_rules! make_struct {
pub(crate) preferred_address: Option<PreferredAddress>,


pub(crate) receive_timestamps_exponent: Option<VarInt>,
pub(crate) max_recv_timestamps_per_ack: Option<VarInt>,
pub(crate) ack_timestamps_cfg: AckTimestampsConfig,
}

// We deliberately don't implement the `Default` trait, since that would be public, and
Expand All @@ -124,8 +123,7 @@ macro_rules! make_struct {
retry_src_cid: None,
stateless_reset_token: None,
preferred_address: None,
receive_timestamps_exponent: None,
max_recv_timestamps_per_ack: None,
ack_timestamps_cfg: AckTimestampsConfig::default(),
}
}
}
Expand Down Expand Up @@ -167,15 +165,7 @@ impl TransportParameters {
VarInt::from_u64(u64::try_from(TIMER_GRANULARITY.as_micros()).unwrap()).unwrap(),
),

receive_timestamps_exponent: config
.ack_timestamp_config
.as_ref()
.map(|cfg| cfg.exponent),

max_recv_timestamps_per_ack: config
.ack_timestamp_config
.as_ref()
.map(|cfg| cfg.max_timestamps_per_ack),
ack_timestamps_cfg: config.ack_timestamp_config,

..Self::default()
}
Expand Down Expand Up @@ -370,16 +360,15 @@ impl TransportParameters {
// The below 2 fields are for the implementation of
// https://www.ietf.org/archive/id/draft-smith-quic-receive-ts-00.html#name-extension-negotiation
// Values of 0x00f0 and 0x00f1 arbitrarily chosen.
if let Some(x) = self.max_recv_timestamps_per_ack {
if let Some(max) = self.ack_timestamps_cfg.max_timestamps_per_ack {
w.write_var(0x00f0);
w.write_var(x.size() as u64);
w.write(x);
}
w.write_var(max.size() as u64);
w.write(max);

if let Some(x) = self.receive_timestamps_exponent {
let exponent = self.ack_timestamps_cfg.exponent;
w.write_var(0x00f1);
w.write_var(x.size() as u64);
w.write(x);
w.write_var(exponent.size() as u64);
w.write(exponent);
}
}

Expand Down Expand Up @@ -447,16 +436,18 @@ impl TransportParameters {
0xff04de1b => params.min_ack_delay = Some(r.get().unwrap()),

0x00f0 => {
if len > 8 || params.max_recv_timestamps_per_ack.is_some() {
if len > 8 || params.ack_timestamps_cfg.max_timestamps_per_ack.is_some() {
return Err(Error::Malformed);
}
params.max_recv_timestamps_per_ack = Some(r.get().unwrap());
params
.ack_timestamps_cfg
.max_timestamps_per_ack(r.get().unwrap());
}
0x00f1 => {
if len > 8 || params.receive_timestamps_exponent.is_some() {
if len > 8 {
return Err(Error::Malformed);
}
params.receive_timestamps_exponent = Some(r.get().unwrap());
params.ack_timestamps_cfg.exponent(r.get().unwrap());
}

_ => {
Expand Down Expand Up @@ -511,10 +502,7 @@ impl TransportParameters {
}

// https://www.ietf.org/archive/id/draft-smith-quic-receive-ts-00.html#name-extension-negotiation
if params
.receive_timestamps_exponent
.map_or(false, |x| x.0 > 20)
{
if params.ack_timestamps_cfg.exponent.0 > 20 {
return Err(Error::IllegalValue);
}

Expand Down Expand Up @@ -554,9 +542,10 @@ mod test {
grease_quic_bit: true,
min_ack_delay: Some(2_000u32.into()),

receive_timestamps_exponent: Some(3u32.into()),
max_recv_timestamps_per_ack: Some(5u32.into()),

ack_timestamps_cfg: AckTimestampsConfig {
max_timestamps_per_ack: Some(10u32.into()),
exponent: 2u32.into(),
},
..TransportParameters::default()
};
params.write(&mut buf);
Expand Down

0 comments on commit 67af041

Please sign in to comment.