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

No RTT samples, no persistent congestion #3889

Merged
merged 10 commits into from Jul 29, 2020
Merged

No RTT samples, no persistent congestion #3889

Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
2d92d2c
No RTT samples, no persistent congestion
ianswett Jul 9, 2020
414498f
Update draft-ietf-quic-recovery.md
ianswett Jul 14, 2020
5a790e1
Update draft-ietf-quic-recovery.md
ianswett Jul 20, 2020
eabdc5c
Update draft-ietf-quic-recovery.md
ianswett Jul 20, 2020
a5cfbdc
Update draft-ietf-quic-recovery.md
ianswett Jul 20, 2020
89e99d1
Update draft-ietf-quic-recovery.md
ianswett Jul 21, 2020
4139dce
Update draft-ietf-quic-recovery.md
ianswett Jul 21, 2020
1acc648
Update draft-ietf-quic-recovery.md
ianswett Jul 21, 2020
3ddca85
Kazuho's suggestion
ianswett Jul 21, 2020
e4601fe
Update draft-ietf-quic-recovery.md
ianswett Jul 22, 2020
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
2 changes: 1 addition & 1 deletion draft-ietf-quic-recovery.md
View file
Open in desktop
Expand Up @@ -1589,7 +1589,7 @@ Invoked when DetectAndRemoveLostPackets deems packets lost.
InPersistentCongestion(largest_lost):
// Persistent congestion cannot be declared on the
// first RTT sample.
if (is first rtt sample):
if (is first RTT sample):
return false
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if that's correct. What matters is that the packet you use for the start of the persistent congestion period was sent after you already had an RTT sample.

Copy link
Member

Choose a reason for hiding this comment

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

I agree with @marten-seemann here.

Consider the case where an endpoint receives the first ACK 10 seconds after sending the first packet (with an RTT of 10ms), then receives the next ACK in 20ms (with RTT of 10ms).

When the second ACK is being processed, the execution would pass through this if, and invoke AreAllPacketsLost. Because all the packets that were sent during the first few seconds are not acked, persistent congestion would be declared.

I think we can simply remove these lines, and rely on the fact that the comment in the pseudo-code talking about "edges." We can clarify what "edge" means, if necessary.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can simply remove these lines, and rely on the fact that the comment in the pseudo-code talking about "edges." We can clarify what "edge" means, if necessary.

I'm not sure if that's sufficient. I think I'd prefer to make the pseudo-code more closely resemble what you'd have to implement. Specifically, to implement this, you'll need a new global variable time_of_first_rtt_measurement, or alternative first_packet_sent_with_measured_rtt, and then you'll have to implement a comparison with the time of the packets in lost_packets.

Copy link
Member

@kazuho kazuho Jul 22, 2020 •
edited

Choose a reason for hiding this comment

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

Specifically, to implement this, you'll need a new global variable time_of_first_rtt_measurement, or alternative first_packet_sent_with_measured_rtt, and then you'll have to implement a comparison with the time of the packets in lost_packets.

Can you implement that way? Persistent congestion is declared across all packet number spaces. That means that you'd have to consult if any acks were received in other packet number spaces during the loss period observed in lost_packets. But you cannot remember all the moments when acks were received, because the state would be unbounded.

Copy link

Choose a reason for hiding this comment

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

@kazuho can you explain your reasoning around "edges" being sufficient?

Copy link
Member

Choose a reason for hiding this comment

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

@mjoras If we define "edge" as a lost packet next to a packet that has been acked, having both edges means that a series of packets being lost are surrounded by acks. As there would have been RTT samples obtained for each of the surrounding acks, there is no need for a separate condition that checks if an RTT sample has been previously obtained.

Copy link

Choose a reason for hiding this comment

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

That's very cute. I would be okay with that too if we cleaned up what "edge" means and also clarify the notion of which contiguous segment should be considered for the persistent period.

Copy link
Member

Choose a reason for hiding this comment

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

Discussed offline with @janaiyengar, it has turned out that the approach of tweaking the definion of edges does not work well, because the edges (from current time) can be any packet, while RTT can only be established with an ack-eliciting packet.

pto = smoothed_rtt + max(4 * rttvar, kGranularity) +
max_ack_delay
Expand Down