Skip to content

Commit

Permalink
Don't send IMMEDIATE_ACK frames every RTT
Browse files Browse the repository at this point in the history 
This behavior was redundant to intelligent transmission of
ACK_FREQUENCY frames when RTT change is observed.
  • Loading branch information
@Ralith
Ralith committed Sep 9, 2023
1 parent b83d5c4 commit 0af891e
Show file tree
Hide file tree
Showing 3 changed files with 25 additions and 142 deletions.
40 changes: 1 addition & 39 deletions quinn-proto/src/connection/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -790,22 +790,6 @@ impl Connection {
break;
}

if builder.ack_eliciting
&& builder.space == SpaceId::Data
&& self.peer_supports_ack_frequency()
&& self.config.ack_frequency_config.is_some()
&& self.timers.get(Timer::Rtt).is_none()
{
// Request an immediate ACK to ensure we receive one within RTT, as recommended by
// the acknowledgement frequency draft
self.spaces[SpaceId::Data].immediate_ack_pending = true;
tracing::trace!(
"setting RTT timer with RTT = {} ms",
self.path.rtt.get().as_millis()
);
self.timers.set(Timer::Rtt, now + self.path.rtt.get());
}

let sent = self.populate_packet(
now,
space_id,
Expand Down Expand Up @@ -1091,28 +1075,6 @@ impl Connection {
.pending_acks
.on_max_ack_delay_timeout()
}
Timer::Rtt => {
// This timer is only armed when ACK frequency has been enabled, the peer
// supports it, and we are in the Data space
let space = &mut self.spaces[SpaceId::Data];
trace!(
in_flight_ack_eliciting = space.sent_packets.len(),
"rtt timer"
);

// It is recommended to request an immediate ACK once per RTT if there are
// unacked packets. Packets with timer-triggered `IMMEDIATE_ACK` frames count
// towards unacked packets, but we don't want to take them into account here.
// For that reason, we only send an `IMMEDIATE_ACK` frame when there
// are at least _two_ unacked packets. This is an approximation, but should work
// well in practice (see
// [this comment](https://github.com/quinn-rs/quinn/pull/1553#discussion_r1251268689)
// for more details)
if space.sent_packets.len() > 1 {
space.immediate_ack_pending = true;
self.timers.set(Timer::Rtt, now + self.path.rtt.get());
}
}
}
}
}
Expand Down Expand Up @@ -3374,7 +3336,7 @@ impl Connection {
pub(crate) fn is_idle(&self) -> bool {
Timer::VALUES
.iter()
.filter(|&&t| t != Timer::KeepAlive && t != Timer::PushNewCid && t != Timer::Rtt)
.filter(|&&t| t != Timer::KeepAlive && t != Timer::PushNewCid)
.filter_map(|&t| Some((t, self.timers.get(t)?)))
.min_by_key(|&(_, time)| time)
.map_or(true, |(timer, _)| timer == Timer::Idle)
Expand Down
6 changes: 1 addition & 5 deletions quinn-proto/src/connection/timer.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,13 +20,10 @@ pub(crate) enum Timer {
PushNewCid = 7,
/// When to send an immediate ACK if there are unacked ack-eliciting packets of the peer
MaxAckDelay = 8,
/// When the RTT is elapsed (used to request an immediate ack if there are local unacked
/// ack-eliciting packets)
Rtt = 9,
}

impl Timer {
pub(crate) const VALUES: [Self; 10] = [
pub(crate) const VALUES: [Self; 9] = [
Self::LossDetection,
Self::Idle,
Self::Close,
Expand All @@ -36,7 +33,6 @@ impl Timer {
Self::Pacing,
Self::PushNewCid,
Self::MaxAckDelay,
Self::Rtt,
];
}

Expand Down
121 changes: 23 additions & 98 deletions quinn-proto/src/tests/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2416,17 +2416,16 @@ fn setup_ack_frequency_test(max_ack_delay: Duration) -> (Pair, ConnectionHandle,
(pair, client_ch, server_ch)
}

/// Verify that max ACK delay is counted from the first ACK-eliciting packet
#[test]
fn ack_frequency_start_of_ack_eliciting_sequence() {
fn ack_frequency_ack_delayed_from_first_of_flight() {
let _guard = subscribe();
let (mut pair, client_ch, server_ch) = setup_ack_frequency_test(Duration::from_millis(30));

// The client sends the following frames:
//
// * 0 ms: ping + IMMEDIATE_ACK
// * 5 ms: ping
// * 5 ms: ping
// * 10 ms: IMMEDIATE_ACK
// * 0 ms: ping
// * 5 ms: ping x2
pair.client_conn_mut(client_ch).ping();
pair.drive_client();

Expand All @@ -2436,54 +2435,39 @@ fn ack_frequency_start_of_ack_eliciting_sequence() {
pair.drive_client();
}

pair.time += Duration::from_millis(15);
pair.drive_client();

// Server: receive the first ping and send an ACK
pair.time -= Duration::from_millis(10);
pair.time += Duration::from_millis(5);
// Server: receive the first ping and send no ACK
let server_stats_before = pair.server_conn_mut(server_ch).stats();
pair.drive_server();
let server_stats_after = pair.server_conn_mut(server_ch).stats();
assert_eq!(
server_stats_after.frame_rx.ping - server_stats_before.frame_rx.ping,
1
);
assert_eq!(
server_stats_after.frame_rx.immediate_ack - server_stats_before.frame_rx.immediate_ack,
1
);
assert_eq!(
server_stats_after.frame_tx.acks - server_stats_before.frame_tx.acks,
1
0
);

// Server: receive the second and third ping and send no ACK
pair.time += Duration::from_millis(5);
// Server: receive the second and third pings and send no ACK
pair.time += Duration::from_millis(10);
let server_stats_before = pair.server_conn_mut(server_ch).stats();
pair.drive_server();
let server_stats_after = pair.server_conn_mut(server_ch).stats();
assert_eq!(
server_stats_after.frame_rx.ping - server_stats_before.frame_rx.ping,
2
);
assert_eq!(
server_stats_after.frame_rx.immediate_ack - server_stats_before.frame_rx.immediate_ack,
0
);
assert_eq!(
server_stats_after.frame_tx.acks - server_stats_before.frame_tx.acks,
0
);

// Server: receive the IMMEDIATE_ACK and reply to it
pair.time += Duration::from_millis(15);
// Server: Send an ACK after ACK delay expires
pair.time += Duration::from_millis(20);
let server_stats_before = pair.server_conn_mut(server_ch).stats();
pair.drive_server();
let server_stats_after = pair.server_conn_mut(server_ch).stats();
assert_eq!(
server_stats_after.frame_rx.immediate_ack - server_stats_before.frame_rx.immediate_ack,
1
);
assert_eq!(
server_stats_after.frame_tx.acks - server_stats_before.frame_tx.acks,
1
Expand All @@ -2496,47 +2480,19 @@ fn ack_frequency_ack_sent_after_max_ack_delay() {
let max_ack_delay = Duration::from_millis(30);
let (mut pair, client_ch, server_ch) = setup_ack_frequency_test(max_ack_delay);

// The client sends the following frames:
//
// * 0 ms: ping + IMMEDIATE_ACK
// * 5 ms: ping
pair.client_conn_mut(client_ch).ping();
pair.drive_client();
pair.time += Duration::from_millis(5);
// Client sends a ping
pair.client_conn_mut(client_ch).ping();
pair.drive_client();

// Server: receive the first IMMEDIATE_ACK and reply to it
pair.time += Duration::from_millis(5);
// Server: receive the ping, send no ACK
pair.time += pair.latency;
let server_stats_before = pair.server_conn_mut(server_ch).stats();
pair.drive_server();
let server_stats_after = pair.server_conn_mut(server_ch).stats();
assert_eq!(
server_stats_after.frame_rx.ping - server_stats_before.frame_rx.ping,
1
);
assert_eq!(
server_stats_after.frame_rx.immediate_ack - server_stats_before.frame_rx.immediate_ack,
1
);
assert_eq!(
server_stats_after.frame_tx.acks - server_stats_before.frame_tx.acks,
1
);

// Server: receive the second ping, send no ACK
pair.time += Duration::from_millis(5);
let server_stats_before = pair.server_conn_mut(server_ch).stats();
pair.drive_server();
let server_stats_after = pair.server_conn_mut(server_ch).stats();
assert_eq!(
server_stats_after.frame_rx.ping - server_stats_before.frame_rx.ping,
1
);
assert_eq!(
server_stats_after.frame_rx.immediate_ack - server_stats_before.frame_rx.immediate_ack,
0
);
assert_eq!(
server_stats_after.frame_tx.acks - server_stats_before.frame_tx.acks,
0
Expand All @@ -2551,10 +2507,6 @@ fn ack_frequency_ack_sent_after_max_ack_delay() {
server_stats_after.frame_rx.ping - server_stats_before.frame_rx.ping,
0
);
assert_eq!(
server_stats_after.frame_rx.immediate_ack - server_stats_before.frame_rx.immediate_ack,
0
);
assert_eq!(
server_stats_after.frame_tx.acks - server_stats_before.frame_tx.acks,
1
Expand All @@ -2569,7 +2521,7 @@ fn ack_frequency_ack_sent_after_packets_above_threshold() {

// The client sends the following frames:
//
// * 0 ms: ping + IMMEDIATE_ACK
// * 0 ms: ping
// * 5 ms: ping (11x)
pair.client_conn_mut(client_ch).ping();
pair.drive_client();
Expand All @@ -2580,7 +2532,7 @@ fn ack_frequency_ack_sent_after_packets_above_threshold() {
pair.drive_client();
}

// Server: receive the first ping, send ACK
// Server: receive the first ping, send no ACK
pair.time += Duration::from_millis(5);
let server_stats_before = pair.server_conn_mut(server_ch).stats();
pair.drive_server();
Expand All @@ -2589,13 +2541,9 @@ fn ack_frequency_ack_sent_after_packets_above_threshold() {
server_stats_after.frame_rx.ping - server_stats_before.frame_rx.ping,
1
);
assert_eq!(
server_stats_after.frame_rx.immediate_ack - server_stats_before.frame_rx.immediate_ack,
1
);
assert_eq!(
server_stats_after.frame_tx.acks - server_stats_before.frame_tx.acks,
1
0
);

// Server: receive the remaining pings, send ACK
Expand All @@ -2607,10 +2555,6 @@ fn ack_frequency_ack_sent_after_packets_above_threshold() {
server_stats_after.frame_rx.ping - server_stats_before.frame_rx.ping,
11
);
assert_eq!(
server_stats_after.frame_rx.immediate_ack - server_stats_before.frame_rx.immediate_ack,
0
);
assert_eq!(
server_stats_after.frame_tx.acks - server_stats_before.frame_tx.acks,
1
Expand All @@ -2625,7 +2569,7 @@ fn ack_frequency_ack_sent_after_reordered_packets_below_threshold() {

// The client sends the following frames:
//
// * 0 ms: ping + IMMEDIATE_ACK
// * 0 ms: ping
// * 5 ms: ping (lost)
// * 5 ms: ping
pair.client_conn_mut(client_ch).ping();
Expand All @@ -2643,7 +2587,7 @@ fn ack_frequency_ack_sent_after_reordered_packets_below_threshold() {
pair.client_conn_mut(client_ch).ping();
pair.drive_client();

// Server: receive first ping, send ACK
// Server: receive first ping, send no ACK
pair.time += Duration::from_millis(5);
let server_stats_before = pair.server_conn_mut(server_ch).stats();
pair.drive_server();
Expand All @@ -2652,13 +2596,9 @@ fn ack_frequency_ack_sent_after_reordered_packets_below_threshold() {
server_stats_after.frame_rx.ping - server_stats_before.frame_rx.ping,
1
);
assert_eq!(
server_stats_after.frame_rx.immediate_ack - server_stats_before.frame_rx.immediate_ack,
1
);
assert_eq!(
server_stats_after.frame_tx.acks - server_stats_before.frame_tx.acks,
1
0
);

// Server: receive second ping, send no ACK
Expand All @@ -2670,10 +2610,6 @@ fn ack_frequency_ack_sent_after_reordered_packets_below_threshold() {
server_stats_after.frame_rx.ping - server_stats_before.frame_rx.ping,
1
);
assert_eq!(
server_stats_after.frame_rx.immediate_ack - server_stats_before.frame_rx.immediate_ack,
0
);
assert_eq!(
server_stats_after.frame_tx.acks - server_stats_before.frame_tx.acks,
0
Expand All @@ -2686,10 +2622,7 @@ fn ack_frequency_ack_sent_after_reordered_packets_above_threshold() {
let max_ack_delay = Duration::from_millis(30);
let (mut pair, client_ch, server_ch) = setup_ack_frequency_test(max_ack_delay);

// The client sends the following frames:
//
// * 0 ms: ping + IMMEDIATE_ACK
// * 5 ms: ping
// Send a ping
pair.client_conn_mut(client_ch).ping();
pair.drive_client();

Expand All @@ -2706,7 +2639,7 @@ fn ack_frequency_ack_sent_after_reordered_packets_above_threshold() {
pair.client_conn_mut(client_ch).ping();
pair.drive_client();

// Server: receive first ping, send ACK
// Server: receive first ping, send no ACK
pair.time += Duration::from_millis(5);
let server_stats_before = pair.server_conn_mut(server_ch).stats();
pair.drive_server();
Expand All @@ -2715,13 +2648,9 @@ fn ack_frequency_ack_sent_after_reordered_packets_above_threshold() {
server_stats_after.frame_rx.ping - server_stats_before.frame_rx.ping,
1
);
assert_eq!(
server_stats_after.frame_rx.immediate_ack - server_stats_before.frame_rx.immediate_ack,
1
);
assert_eq!(
server_stats_after.frame_tx.acks - server_stats_before.frame_tx.acks,
1
0
);

// Server: receive remaining ping, send ACK
Expand All @@ -2733,10 +2662,6 @@ fn ack_frequency_ack_sent_after_reordered_packets_above_threshold() {
server_stats_after.frame_rx.ping - server_stats_before.frame_rx.ping,
1
);
assert_eq!(
server_stats_after.frame_rx.immediate_ack - server_stats_before.frame_rx.immediate_ack,
0
);
assert_eq!(
server_stats_after.frame_tx.acks - server_stats_before.frame_tx.acks,
1
Expand Down

0 comments on commit 0af891e

Please sign in to comment.