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

Rework section on persistent congestion #3961

Merged
merged 11 commits into from Sep 1, 2020
Merged

Rework section on persistent congestion #3961

Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
999e179
sync
janaiyengar Jul 28, 2020
fd11e9b
Rework section on persistent congestion
janaiyengar Jul 28, 2020
c750cc2
martin and ian comments
janaiyengar Jul 29, 2020
fa24f98
Apply suggestions from code review
janaiyengar Jul 28, 2020
8c4f77d
mjoras comment
janaiyengar Jul 29, 2020
2b0dd56
kazuho comment
janaiyengar Jul 29, 2020
e7e1b02
merge changes from 3889
janaiyengar Jul 29, 2020
310832d
editorial
janaiyengar Jul 29, 2020
b38fb27
ian comments
janaiyengar Aug 1, 2020
275d9bc
PTO expiration
janaiyengar Aug 2, 2020
f3baa0e
mike and ian comments
janaiyengar Aug 11, 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
13 changes: 6 additions & 7 deletions draft-ietf-quic-recovery.md
View file
Open in desktop
Expand Up @@ -839,10 +839,9 @@ The persistent congestion duration is computed as follows:
Unlike the PTO computation in {{pto}}, this duration includes the max_ack_delay
irrespective of the packet number spaces in which losses are established.

This duration allows a sender to send as many packets, including some in
response to PTO expiration, as TCP does with Tail Loss Probe ({{RACK}}), before
establishing persistent congestion, as TCP does with a Retransmission Timeout
({{?RFC5681}}).
This duration allows a sender to send as many packets before establishing
persistent congestion, including some in response to PTO expiration, as TCP does
with Tail Loss Probes ({{RACK}}) and a Retransmission Timeout ({{?RFC5681}}).

The RECOMMENDED value for kPersistentCongestionThreshold is 3, which is
approximately equivalent to two TLPs before an RTO in TCP.
Expand All @@ -860,13 +859,13 @@ congestion without depending on PTO expiration.
A sender establishes persistent congestion on receiving an acknowledgement if at
least two ack-eliciting packets are declared lost, and:

* a prior RTT sample existed when both packets were sent;
* all packets, across all packet number spaces, sent between these two send
times are declared lost;

* the duration between the send times of these two packets exceeds the
janaiyengar marked this conversation as resolved.
Show resolved Hide resolved
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 this is still ambiguous if more than 2 packets are lost, so I'm reusing language below.

Suggested change
* the duration between the send times of these two packets exceeds the
* the duration between the send times of the oldest and newest lost packet exceeds the

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's still not correct. The point here is that there's a contiguous period that is long enough, and the duration has to be between two packets that are the edges of a loss period, not necessarily the oldest and the newest packets. For example, consider that

  • packets 1-10 were sent,
  • all odd packets were marked lost: 1, 3, 5, 7, 9.
  • the duration between 1 and 9 was greater than the persistent congestion duration.

According to the existing design, this should not be considered persistent congestion, but with your proposed text, it will be.

Copy link
Contributor

Choose a reason for hiding this comment

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

There's a requirement that all packets between the largest and smallest are marked lost as well, so my suggested change would still not declare this as persistent congestion(assuming 2, 4, 6 or 8 were acknowledged).

I still think this is unclear if more than 2 packets are lost, so maybe you have a suggestion?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can't require all packets between largest and smallest be acked; see my comment: #3961 (comment).

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry for the confusion, I now have what you have in mind.

If you put this bullet after the next one, I think this will be clearer.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, I might move that third bullet to be first. Conceptually, it frames the idea that they're the first and last packets of a 2+ packet sequence.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, suggestions taken.

persistent congestion duration ({{pc-duration}}); and

* all packets, across all packet number spaces, sent between those two send
times are declared lost.
* a prior RTT sample existed when both packets were sent.

The persistent congestion period SHOULD NOT start until there is at least one
RTT sample. Before the first RTT sample, a sender arms its PTO timer based on
Expand Down