recovery: retransmit frames from 2 oldest unacked packets on PTO
In 39905083 we implemented support for retransmitting unacked data in
PTO probes if no new data is available.
However after discussion it seems that prioritizing new data is not
actually necessary, and simply retransmitting old data first might be
the better strategy (if anything it's easier to implement).
This reverts the changes from the previous commit, and simply implements
logic to reschedule the frames from the 2 oldest sent packets when the
PTO expires.
Besides having a much simpler implementation, this also has the
advantage of allowing other frames to be sent in the probe packet (e.g.
ACKs), without risking that the original data frame wouldn't fit in the
output packet, since we won't retransmit CRYPTO and STREAM frames as-is
but will go through the normal packetization procedure (including e.g.
respecting STREAM priorities).
This also means that we won't retransmit the same data in both probe
packets. Though if there aren't enough packets with data frames
available we might still end-up sending a PING frame only in the second
probe packet.
diff --git a/src/lib.rs b/src/lib.rs
index d21cbee..196cca6 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -2080,6 +2080,7 @@
let mut ack_eliciting = false;
let mut in_flight = false;
+ let mut has_data = false;
let mut payload_len = 0;
@@ -2284,6 +2285,7 @@
if push_frame_to_pkt!(frames, frame, payload_len, left) {
ack_eliciting = true;
in_flight = true;
+ has_data = true;
}
}
@@ -2329,6 +2331,7 @@
if push_frame_to_pkt!(frames, frame, payload_len, left) {
ack_eliciting = true;
in_flight = true;
+ has_data = true;
}
// If the stream is still flushable, push it to the back of the
@@ -2350,45 +2353,6 @@
}
}
- // Try to retransmit old frames on PTO, if there is space left.
- if self.recovery.loss_probes[epoch] > 0 &&
- left > cmp::min(
- frame::MAX_CRYPTO_OVERHEAD,
- frame::MAX_STREAM_OVERHEAD,
- ) &&
- !is_closing
- {
- let unacked_iter = self.recovery.sent[epoch]
- .iter_mut()
- // Skip packets that have already been acked or lost and packets
- // that are not in-flight.
- .filter(|p| p.in_flight && p.time_acked.is_none() && p.time_lost.is_none());
-
- 'unacked_iter: for unacked in unacked_iter {
- for frame in &unacked.frames {
- // Only retransmit CRYPTO and STREAM frames.
- match frame {
- frame::Frame::Crypto { .. } |
- frame::Frame::Stream { .. } => (),
-
- _ => continue,
- }
-
- // Skip frame if it's too big.
- if left < frame.wire_len() {
- break 'unacked_iter;
- }
-
- push_frame_to_pkt!(frames, frame.clone(), payload_len, left);
-
- ack_eliciting = true;
- in_flight = true;
-
- break 'unacked_iter;
- }
- }
- }
-
// Create PING for PTO probe if no other ack-elicitng frame is sent.
if self.recovery.loss_probes[epoch] > 0 &&
!ack_eliciting &&
@@ -2525,6 +2489,7 @@
delivered_time: now,
recent_delivered_packet_sent_time: now,
is_app_limited: false,
+ has_data,
};
self.recovery.on_packet_sent(
diff --git a/src/recovery/cubic.rs b/src/recovery/cubic.rs
index 11e29c9..4f3888d 100644
--- a/src/recovery/cubic.rs
+++ b/src/recovery/cubic.rs
@@ -319,6 +319,7 @@
delivered_time: now,
recent_delivered_packet_sent_time: now,
is_app_limited: false,
+ has_data: false,
};
// Send 5k x 4 = 20k, higher than default cwnd(~15k)
@@ -456,6 +457,7 @@
delivered_time: now,
recent_delivered_packet_sent_time: now,
is_app_limited: false,
+ has_data: false,
};
// 1st round.
diff --git a/src/recovery/delivery_rate.rs b/src/recovery/delivery_rate.rs
index bd54d0e..77fd248 100644
--- a/src/recovery/delivery_rate.rs
+++ b/src/recovery/delivery_rate.rs
@@ -204,6 +204,7 @@
delivered_time: Instant::now(),
recent_delivered_packet_sent_time: Instant::now(),
is_app_limited: false,
+ has_data: false,
};
recovery
@@ -227,6 +228,7 @@
delivered_time: Instant::now(),
recent_delivered_packet_sent_time: Instant::now(),
is_app_limited: false,
+ has_data: false,
};
recovery
@@ -259,6 +261,7 @@
delivered_time: Instant::now(),
recent_delivered_packet_sent_time: Instant::now(),
is_app_limited: false,
+ has_data: false,
};
recvry
@@ -280,6 +283,7 @@
delivered_time: Instant::now(),
recent_delivered_packet_sent_time: Instant::now(),
is_app_limited: false,
+ has_data: false,
};
recvry.app_limited = true;
diff --git a/src/recovery/mod.rs b/src/recovery/mod.rs
index 6988394..1f9e6ae 100644
--- a/src/recovery/mod.rs
+++ b/src/recovery/mod.rs
@@ -55,6 +55,8 @@
const RTT_WINDOW: Duration = Duration::from_secs(300);
+const PTO_PROBES_COUNT: usize = 2;
+
// Congestion Control
const INITIAL_WINDOW_PACKETS: usize = 10;
@@ -91,7 +93,7 @@
loss_time: [Option<Instant>; packet::EPOCH_COUNT],
- pub sent: [VecDeque<Sent>; packet::EPOCH_COUNT],
+ sent: [VecDeque<Sent>; packet::EPOCH_COUNT],
pub lost: [Vec<frame::Frame>; packet::EPOCH_COUNT],
@@ -354,7 +356,27 @@
handshake_completed,
);
- self.loss_probes[epoch] = 2;
+ let unacked_iter = self.sent[epoch]
+ .iter_mut()
+ // Skip packets that have already been acked or lost, and packets
+ // that don't contain either CRYPTO or STREAM frames.
+ .filter(|p| p.has_data && p.time_acked.is_none() && p.time_lost.is_none())
+ // Only return as many packets as the number of probe packets that
+ // will be sent.
+ .take(PTO_PROBES_COUNT);
+
+ // Retransmit the frames from the oldest sent packets on PTO. However
+ // the packets are not actually declared lost (so there is no effect to
+ // congestion control), we just reschedule the data they carried.
+ //
+ // This will also trigger sending an ACK and retransmitting frames like
+ // HANDSHAKE_DONE and MAX_DATA / MAX_STREAM_DATA as well, in addition
+ // to CRYPTO and STREAM, if the original packet carried them.
+ for unacked in unacked_iter {
+ self.lost[epoch].extend_from_slice(&unacked.frames);
+ }
+
+ self.loss_probes[epoch] = PTO_PROBES_COUNT;
self.pto_count += 1;
@@ -823,6 +845,8 @@
pub recent_delivered_packet_sent_time: Instant,
pub is_app_limited: bool,
+
+ pub has_data: bool,
}
impl std::fmt::Debug for Sent {
@@ -831,13 +855,14 @@
write!(f, "pkt_sent_time={:?} ", self.time_sent.elapsed())?;
write!(f, "pkt_size={:?} ", self.size)?;
write!(f, "delivered={:?} ", self.delivered)?;
- write!(f, "delivered_time ={:?} ", self.delivered_time.elapsed())?;
+ write!(f, "delivered_time={:?} ", self.delivered_time.elapsed())?;
write!(
f,
"recent_delivered_packet_sent_time={:?} ",
self.recent_delivered_packet_sent_time.elapsed()
)?;
- write!(f, "is_app_limited={:?} ", self.is_app_limited)?;
+ write!(f, "is_app_limited={} ", self.is_app_limited)?;
+ write!(f, "has_data={} ", self.has_data)?;
Ok(())
}
@@ -915,6 +940,7 @@
delivered_time: now,
recent_delivered_packet_sent_time: now,
is_app_limited: false,
+ has_data: false,
};
r.on_packet_sent(p, packet::EPOCH_APPLICATION, true, now, "");
@@ -934,6 +960,7 @@
delivered_time: now,
recent_delivered_packet_sent_time: now,
is_app_limited: false,
+ has_data: false,
};
r.on_packet_sent(p, packet::EPOCH_APPLICATION, true, now, "");
@@ -953,6 +980,7 @@
delivered_time: now,
recent_delivered_packet_sent_time: now,
is_app_limited: false,
+ has_data: false,
};
r.on_packet_sent(p, packet::EPOCH_APPLICATION, true, now, "");
@@ -972,6 +1000,7 @@
delivered_time: now,
recent_delivered_packet_sent_time: now,
is_app_limited: false,
+ has_data: false,
};
r.on_packet_sent(p, packet::EPOCH_APPLICATION, true, now, "");
@@ -1023,6 +1052,7 @@
delivered_time: now,
recent_delivered_packet_sent_time: now,
is_app_limited: false,
+ has_data: false,
};
r.on_packet_sent(p, packet::EPOCH_APPLICATION, true, now, "");
@@ -1042,6 +1072,7 @@
delivered_time: now,
recent_delivered_packet_sent_time: now,
is_app_limited: false,
+ has_data: false,
};
r.on_packet_sent(p, packet::EPOCH_APPLICATION, true, now, "");
@@ -1099,6 +1130,7 @@
delivered_time: now,
recent_delivered_packet_sent_time: now,
is_app_limited: false,
+ has_data: false,
};
r.on_packet_sent(p, packet::EPOCH_APPLICATION, true, now, "");
@@ -1118,6 +1150,7 @@
delivered_time: now,
recent_delivered_packet_sent_time: now,
is_app_limited: false,
+ has_data: false,
};
r.on_packet_sent(p, packet::EPOCH_APPLICATION, true, now, "");
@@ -1137,6 +1170,7 @@
delivered_time: now,
recent_delivered_packet_sent_time: now,
is_app_limited: false,
+ has_data: false,
};
r.on_packet_sent(p, packet::EPOCH_APPLICATION, true, now, "");
@@ -1156,6 +1190,7 @@
delivered_time: now,
recent_delivered_packet_sent_time: now,
is_app_limited: false,
+ has_data: false,
};
r.on_packet_sent(p, packet::EPOCH_APPLICATION, true, now, "");
@@ -1224,6 +1259,7 @@
delivered_time: now,
recent_delivered_packet_sent_time: now,
is_app_limited: false,
+ has_data: false,
};
r.on_packet_sent(p, packet::EPOCH_APPLICATION, true, now, "");
@@ -1243,6 +1279,7 @@
delivered_time: now,
recent_delivered_packet_sent_time: now,
is_app_limited: false,
+ has_data: false,
};
r.on_packet_sent(p, packet::EPOCH_APPLICATION, true, now, "");
@@ -1262,6 +1299,7 @@
delivered_time: now,
recent_delivered_packet_sent_time: now,
is_app_limited: false,
+ has_data: false,
};
r.on_packet_sent(p, packet::EPOCH_APPLICATION, true, now, "");
@@ -1281,6 +1319,7 @@
delivered_time: now,
recent_delivered_packet_sent_time: now,
is_app_limited: false,
+ has_data: false,
};
r.on_packet_sent(p, packet::EPOCH_APPLICATION, true, now, "");
diff --git a/src/recovery/reno.rs b/src/recovery/reno.rs
index 9bcfcd7..2a1628e 100644
--- a/src/recovery/reno.rs
+++ b/src/recovery/reno.rs
@@ -171,6 +171,7 @@
delivered_time: std::time::Instant::now(),
recent_delivered_packet_sent_time: std::time::Instant::now(),
is_app_limited: false,
+ has_data: false,
};
// Send 5k x 4 = 20k, higher than default cwnd(~15k)
