Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix fast retransmit #233

Closed
wants to merge 2 commits into from
Closed

Fix fast retransmit #233

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3a10cc0
Reset retrasmission delay after fast retrasmit
pothos Jun 12, 2018
277e7a3
Only trigger fast retransmit for data to send
pothos Jun 12, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
66 changes: 61 additions & 5 deletions src/socket/tcp.rs
View file
Open in desktop
Expand Up @@ -143,7 +143,7 @@ impl Timer {

fn set_for_retransmit(&mut self, timestamp: Instant) {
match *self {
Timer::Idle { .. } => {
Timer::Idle { .. } | Timer::FastRetransmit { .. } => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you explain this decision?

If we're already doing a fast retransmit, why would we change it to a retransmit with a timeout?

Is it related to exiting from the fast retransmit state?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, set_for_retransmit is called after all was send out, so this leaves the fast retransmit mode and should wait now instead of looping in poll.

*self = Timer::Retransmit {
expires_at: timestamp + RETRANSMIT_DELAY,
delay: RETRANSMIT_DELAY,
Expand All @@ -157,7 +157,6 @@ impl Timer {
}
}
Timer::Retransmit { .. } => (),
Timer::FastRetransmit { .. } => (),
Timer::Close { .. } => ()
}
}
Expand Down Expand Up @@ -1158,9 +1157,10 @@ impl<'a> TcpSocket<'a> {
// Duplicate ACK if payload empty and ACK doesn't move send window ->
// Increment duplicate ACK count and set for retransmit if we just recived
// the third duplicate ACK
Some(ref mut last_rx_ack) if
Some(ref last_rx_ack) if
repr.payload.len() == 0 &&
*last_rx_ack == ack_number => {
*last_rx_ack == ack_number &&
ack_number < self.remote_last_seq => {
// Increment duplicate ACK count
self.local_rx_dup_acks = self.local_rx_dup_acks.saturating_add(1);

Expand Down Expand Up @@ -3289,6 +3289,32 @@ mod test {
..RECV_TEMPL
}));

recv!(s, time 1105, Ok(TcpRepr {
seq_number: LOCAL_SEQ + 1 + 6,
ack_number: Some(REMOTE_SEQ + 1),
payload: &b"yyyyyy"[..],
..RECV_TEMPL
}));
recv!(s, time 1110, Ok(TcpRepr {
seq_number: LOCAL_SEQ + 1 + (6 * 2),
ack_number: Some(REMOTE_SEQ + 1),
payload: &b"wwwwww"[..],
..RECV_TEMPL
}));
recv!(s, time 1115, Ok(TcpRepr {
seq_number: LOCAL_SEQ + 1 + (6 * 3),
ack_number: Some(REMOTE_SEQ + 1),
payload: &b"zzzzzz"[..],
..RECV_TEMPL
}));

// After all was send out, enter *normal* retransmission,
// don't stay in fast retransmission.
assert!(match s.timer {
Timer::Retransmit { expires_at, .. } => expires_at > Instant::from_millis(1115),
_ => false,
});

// ACK all recived segments
send!(s, time 1120, TcpRepr {
seq_number: REMOTE_SEQ + 1,
Expand All @@ -3301,6 +3327,14 @@ mod test {
fn test_fast_retransmit_duplicate_detection_with_data() {
let mut s = socket_established();

s.send_slice(b"abc").unwrap(); // This is lost
recv!(s, time 1000, Ok(TcpRepr {
seq_number: LOCAL_SEQ + 1,
ack_number: Some(REMOTE_SEQ + 1),
payload: &b"abc"[..],
..RECV_TEMPL
}));

// Normal ACK of previously recieved segment
send!(s, TcpRepr {
seq_number: REMOTE_SEQ + 1,
Expand All @@ -3320,6 +3354,9 @@ mod test {
..SEND_TEMPL
});

assert_eq!(s.local_rx_dup_acks, 2,
"duplicate ACK counter is not set");

// This packet has content, hence should not be detected
// as a duplicate ACK and should reset the duplicate ACK count
send!(s, TcpRepr {
Expand All @@ -3330,7 +3367,7 @@ mod test {
});

recv!(s, [TcpRepr {
seq_number: LOCAL_SEQ + 1,
seq_number: LOCAL_SEQ + 1 + 3,
ack_number: Some(REMOTE_SEQ + 1 + 6),
window_len: 58,
..RECV_TEMPL
Expand All @@ -3352,6 +3389,17 @@ mod test {
..SEND_TEMPL
});

// First duplicate, should not be counted as there is nothing to resend
send!(s, time 0, TcpRepr {
seq_number: REMOTE_SEQ + 1,
ack_number: Some(LOCAL_SEQ + 1),
window_len: 6,
..SEND_TEMPL
});

assert_eq!(s.local_rx_dup_acks, 0,
"duplicate ACK counter is set but wound not transmit data");

// Send a long string of text divided into several packets
// because of previously recieved "window_len"
s.send_slice(b"xxxxxxyyyyyywwwwwwzzzzzz").unwrap();
Expand Down Expand Up @@ -3419,6 +3467,14 @@ mod test {
fn test_fast_retransmit_dup_acks_counter() {
let mut s = socket_established();

s.send_slice(b"abc").unwrap(); // This is lost
recv!(s, time 0, Ok(TcpRepr {
seq_number: LOCAL_SEQ + 1,
ack_number: Some(REMOTE_SEQ + 1),
payload: &b"abc"[..],
..RECV_TEMPL
}));

send!(s, time 0, TcpRepr {
seq_number: REMOTE_SEQ + 1,
ack_number: Some(LOCAL_SEQ + 1),
Expand Down