Skip to content

Commit

Permalink
fix: Resurrect mozilla#1662
Browse files Browse the repository at this point in the history 
Botched the "merge from main" on mozilla#1662. Reverted the merge. This PS
has the changes in mozilla#1662, rebased to main. Sorry for the mess.
  • Loading branch information
@larseggert
larseggert committed Mar 19, 2024
1 parent 7028479 commit b663879
Show file tree
Hide file tree
Showing 12 changed files with 596 additions and 302 deletions.
60 changes: 30 additions & 30 deletions neqo-transport/src/cc/classic_cc.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,9 @@ use crate::{
cc::MAX_DATAGRAM_SIZE,
packet::PacketNumber,
qlog::{self, QlogMetric},
recovery::SentPacket,
rtt::RttEstimate,
sender::PACING_BURST_SIZE,
tracking::SentPacket,
};
#[rustfmt::skip] // to keep `::` and thus prevent conflict with `crate::qlog`
use ::qlog::events::{quic::CongestionStateUpdated, EventData};
Expand Down Expand Up @@ -167,20 +167,20 @@ impl<T: WindowAdjustment> CongestionControl for ClassicCongestionControl<T> {
qinfo!(
"packet_acked this={:p}, pn={}, ps={}, ignored={}, lost={}, rtt_est={:?}",
self,
pkt.pn,
pkt.size,
pkt.pn(),
pkt.len(),
i32::from(!pkt.cc_outstanding()),
i32::from(pkt.lost()),
rtt_est,
);
if !pkt.cc_outstanding() {
continue;
}
if pkt.pn < self.first_app_limited {
if pkt.pn() < self.first_app_limited {
is_app_limited = false;
}
assert!(self.bytes_in_flight >= pkt.size);
self.bytes_in_flight -= pkt.size;
assert!(self.bytes_in_flight >= pkt.len());
self.bytes_in_flight -= pkt.len();

if !self.after_recovery_start(pkt) {
// Do not increase congestion window for packets sent before
Expand All @@ -193,7 +193,7 @@ impl<T: WindowAdjustment> CongestionControl for ClassicCongestionControl<T> {
qlog::metrics_updated(&mut self.qlog, &[QlogMetric::InRecovery(false)]);
}

new_acked += pkt.size;
new_acked += pkt.len();
}

if is_app_limited {
Expand Down Expand Up @@ -268,11 +268,11 @@ impl<T: WindowAdjustment> CongestionControl for ClassicCongestionControl<T> {
qinfo!(
"packet_lost this={:p}, pn={}, ps={}",
self,
pkt.pn,
pkt.size
pkt.pn(),
pkt.len()
);
assert!(self.bytes_in_flight >= pkt.size);
self.bytes_in_flight -= pkt.size;
assert!(self.bytes_in_flight >= pkt.len());
self.bytes_in_flight -= pkt.len();
}
qlog::metrics_updated(
&mut self.qlog,
Expand All @@ -298,13 +298,13 @@ impl<T: WindowAdjustment> CongestionControl for ClassicCongestionControl<T> {

fn discard(&mut self, pkt: &SentPacket) {
if pkt.cc_outstanding() {
assert!(self.bytes_in_flight >= pkt.size);
self.bytes_in_flight -= pkt.size;
assert!(self.bytes_in_flight >= pkt.len());
self.bytes_in_flight -= pkt.len();
qlog::metrics_updated(
&mut self.qlog,
&[QlogMetric::BytesInFlight(self.bytes_in_flight)],
);
qtrace!([self], "Ignore pkt with size {}", pkt.size);
qtrace!([self], "Ignore pkt with size {}", pkt.len());
}
}

Expand All @@ -319,7 +319,7 @@ impl<T: WindowAdjustment> CongestionControl for ClassicCongestionControl<T> {
fn on_packet_sent(&mut self, pkt: &SentPacket) {
// Record the recovery time and exit any transient state.
if self.state.transient() {
self.recovery_start = Some(pkt.pn);
self.recovery_start = Some(pkt.pn());
self.state.update();
}

Expand All @@ -331,15 +331,15 @@ impl<T: WindowAdjustment> CongestionControl for ClassicCongestionControl<T> {
// window. Assume that all in-flight packets up to this one are NOT app-limited.
// However, subsequent packets might be app-limited. Set `first_app_limited` to the
// next packet number.
self.first_app_limited = pkt.pn + 1;
self.first_app_limited = pkt.pn() + 1;
}

self.bytes_in_flight += pkt.size;
self.bytes_in_flight += pkt.len();
qinfo!(
"packet_sent this={:p}, pn={}, ps={}",
self,
pkt.pn,
pkt.size
pkt.pn(),
pkt.len()
);
qlog::metrics_updated(
&mut self.qlog,
Expand Down Expand Up @@ -438,20 +438,20 @@ impl<T: WindowAdjustment> ClassicCongestionControl<T> {
let cutoff = max(first_rtt_sample_time, prev_largest_acked_sent);
for p in lost_packets
.iter()
.skip_while(|p| Some(p.time_sent) < cutoff)
.skip_while(|p| Some(p.time_sent()) < cutoff)
{
if p.pn != last_pn + 1 {
if p.pn() != last_pn + 1 {
// Not a contiguous range of lost packets, start over.
start = None;
}
last_pn = p.pn;
last_pn = p.pn();
if !p.cc_in_flight() {
// Not interesting, keep looking.
continue;
}
if let Some(t) = start {
let elapsed = p
.time_sent
.time_sent()
.checked_duration_since(t)
.expect("time is monotonic");
if elapsed > pc_period {
Expand All @@ -466,7 +466,7 @@ impl<T: WindowAdjustment> ClassicCongestionControl<T> {
return true;
}
} else {
start = Some(p.time_sent);
start = Some(p.time_sent());
}
}
false
Expand All @@ -480,7 +480,7 @@ impl<T: WindowAdjustment> ClassicCongestionControl<T> {
// state and update the variable `self.recovery_start`. Before the
// first recovery, all packets were sent after the recovery event,
// allowing to reduce the cwnd on congestion events.
!self.state.transient() && self.recovery_start.map_or(true, |pn| packet.pn >= pn)
!self.state.transient() && self.recovery_start.map_or(true, |pn| packet.pn() >= pn)
}

/// Handle a congestion event.
Expand Down Expand Up @@ -551,8 +551,8 @@ mod tests {
CongestionControl, CongestionControlAlgorithm, CWND_INITIAL_PKTS, MAX_DATAGRAM_SIZE,
},
packet::{PacketNumber, PacketType},
recovery::SentPacket,
rtt::RttEstimate,
tracking::SentPacket,
};

const PTO: Duration = Duration::from_millis(100);
Expand Down Expand Up @@ -912,12 +912,12 @@ mod tests {
fn persistent_congestion_ack_eliciting() {
let mut lost = make_lost(&[1, PERSISTENT_CONG_THRESH + 2]);
lost[0] = SentPacket::new(
lost[0].pt,
lost[0].pn,
lost[0].time_sent,
lost[0].packet_type(),
lost[0].pn(),
lost[0].time_sent(),
false,
Vec::new(),
lost[0].size,
lost[0].len(),
);
assert!(!persistent_congestion_by_pto(
ClassicCongestionControl::new(NewReno::default()),
Expand Down
2 changes: 1 addition & 1 deletion neqo-transport/src/cc/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ use std::{

use neqo_common::qlog::NeqoQlog;

use crate::{path::PATH_MTU_V6, rtt::RttEstimate, tracking::SentPacket, Error};
use crate::{path::PATH_MTU_V6, recovery::SentPacket, rtt::RttEstimate, Error};

mod classic_cc;
mod cubic;
Expand Down
2 changes: 1 addition & 1 deletion neqo-transport/src/cc/tests/cubic.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,8 @@ use crate::{
CongestionControl, MAX_DATAGRAM_SIZE, MAX_DATAGRAM_SIZE_F64,
},
packet::PacketType,
recovery::SentPacket,
rtt::RttEstimate,
tracking::SentPacket,
};

const RTT: Duration = Duration::from_millis(100);
Expand Down
6 changes: 3 additions & 3 deletions neqo-transport/src/cc/tests/new_reno.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,8 @@ use crate::{
MAX_DATAGRAM_SIZE,
},
packet::PacketType,
recovery::SentPacket,
rtt::RttEstimate,
tracking::SentPacket,
};

const PTO: Duration = Duration::from_millis(100);
Expand Down Expand Up @@ -125,14 +125,14 @@ fn issue_876() {

// and ack it. cwnd increases slightly
cc.on_packets_acked(&sent_packets[6..], &RTT_ESTIMATE, time_now);
assert_eq!(cc.acked_bytes(), sent_packets[6].size);
assert_eq!(cc.acked_bytes(), sent_packets[6].len());
cwnd_is_halved(&cc);
assert_eq!(cc.bytes_in_flight(), 5 * MAX_DATAGRAM_SIZE - 2);

// Packet from before is lost. Should not hurt cwnd.
cc.on_packets_lost(Some(time_now), None, PTO, &sent_packets[1..2]);
assert!(!cc.recovery_packet());
assert_eq!(cc.acked_bytes(), sent_packets[6].size);
assert_eq!(cc.acked_bytes(), sent_packets[6].len());
cwnd_is_halved(&cc);
assert_eq!(cc.bytes_in_flight(), 4 * MAX_DATAGRAM_SIZE);
}
Expand Down
14 changes: 7 additions & 7 deletions neqo-transport/src/connection/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ use crate::{
path::{Path, PathRef, Paths},
qlog,
quic_datagrams::{DatagramTracking, QuicDatagrams},
recovery::{LossRecovery, RecoveryToken, SendProfile},
recovery::{LossRecovery, RecoveryToken, SendProfile, SentPacket},
recv_stream::RecvStreamStats,
rtt::GRANULARITY,
send_stream::SendStream,
Expand All @@ -55,7 +55,7 @@ use crate::{
self, TransportParameter, TransportParameterId, TransportParameters,
TransportParametersHandler,
},
tracking::{AckTracker, PacketNumberSpace, SentPacket},
tracking::{AckTracker, PacketNumberSpace},
version::{Version, WireVersion},
AppError, ConnectionError, Error, Res, StreamId,
};
Expand Down Expand Up @@ -2336,7 +2336,7 @@ impl Connection {
packets.len(),
mtu
);
initial.size += mtu - packets.len();
initial.add_padding(mtu - packets.len());
packets.resize(mtu, 0);
}
self.loss_recovery.on_packet_sent(path, initial);
Expand Down Expand Up @@ -2855,7 +2855,7 @@ impl Connection {
/// to retransmit the frame as needed.
fn handle_lost_packets(&mut self, lost_packets: &[SentPacket]) {
for lost in lost_packets {
for token in &lost.tokens {
for token in lost.tokens() {
qdebug!([self], "Lost: {:?}", token);
match token {
RecoveryToken::Ack(_) => {}
Expand Down Expand Up @@ -2891,12 +2891,12 @@ impl Connection {
fn handle_ack<R>(
&mut self,
space: PacketNumberSpace,
largest_acknowledged: u64,
largest_acknowledged: PacketNumber,
ack_ranges: R,
ack_delay: u64,
now: Instant,
) where
R: IntoIterator<Item = RangeInclusive<u64>> + Debug,
R: IntoIterator<Item = RangeInclusive<PacketNumber>> + Debug,
R::IntoIter: ExactSizeIterator,
{
qinfo!([self], "Rx ACK space={}, ranges={:?}", space, ack_ranges);
Expand All @@ -2910,7 +2910,7 @@ impl Connection {
now,
);
for acked in acked_packets {
for token in &acked.tokens {
for token in acked.tokens() {
match token {
RecoveryToken::Stream(stream_token) => self.streams.acked(stream_token),
RecoveryToken::Ack(at) => self.acks.acked(at),
Expand Down
8 changes: 4 additions & 4 deletions neqo-transport/src/path.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,11 +24,11 @@ use crate::{
cid::{ConnectionId, ConnectionIdRef, ConnectionIdStore, RemoteConnectionIdEntry},
frame::{FRAME_TYPE_PATH_CHALLENGE, FRAME_TYPE_PATH_RESPONSE, FRAME_TYPE_RETIRE_CONNECTION_ID},
packet::PacketBuilder,
recovery::RecoveryToken,
recovery::{RecoveryToken, SentPacket},
rtt::RttEstimate,
sender::PacketSender,
stats::FrameStats,
tracking::{PacketNumberSpace, SentPacket},
tracking::PacketNumberSpace,
Stats,
};

Expand Down Expand Up @@ -943,12 +943,12 @@ impl Path {
qinfo!(
[self],
"discarding a packet without an RTT estimate; guessing RTT={:?}",
now - sent.time_sent
now - sent.time_sent()
);
stats.rtt_init_guess = true;
self.rtt.update(
&mut self.qlog,
now - sent.time_sent,
now - sent.time_sent(),
Duration::new(0, 0),
false,
now,
Expand Down
11 changes: 8 additions & 3 deletions neqo-transport/src/qlog.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,9 @@ use crate::{
frame::{CloseError, Frame},
packet::{DecryptedPacket, PacketNumber, PacketType, PublicPacket},
path::PathRef,
recovery::SentPacket,
stream_id::StreamType as NeqoStreamType,
tparams::{self, TransportParametersHandler},
tracking::SentPacket,
version::{Version, VersionConfig, WireVersion},
};

Expand Down Expand Up @@ -259,8 +259,13 @@ pub fn packet_dropped(qlog: &mut NeqoQlog, public_packet: &PublicPacket) {
pub fn packets_lost(qlog: &mut NeqoQlog, pkts: &[SentPacket]) {
qlog.add_event_with_stream(|stream| {
for pkt in pkts {
let header =
PacketHeader::with_type(to_qlog_pkt_type(pkt.pt), Some(pkt.pn), None, None, None);
let header = PacketHeader::with_type(
to_qlog_pkt_type(pkt.packet_type()),
Some(pkt.pn()),
None,
None,
None,
);

let ev_data = EventData::PacketLost(PacketLost {
header: Some(header),
Expand Down
Loading

0 comments on commit b663879

Please sign in to comment.