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.

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

unclear definition of persistent congestion #3937

Closed
marten-seemann opened this issue Jul 22, 2020 · 5 comments · Fixed by #3961
Closed

unclear definition of persistent congestion #3937

marten-seemann opened this issue Jul 22, 2020 · 5 comments · Fixed by #3961
Labels
-recovery editorial An issue that does not affect the design of the protocol; does not require consensus.

Comments

@marten-seemann
Copy link
Contributor

A sender sends a tail of 10 packets, packet number 1 to 10. It now receives an acknowledgement for packets 5 and 10, which establishes the loss of all other packets.

It's not clear how to perform the check for persistent congestion now. Several options come to mind:

  1. check if the period between 6 and 9 is longer than 3*PTO
  2. check if either the period between packet 1 and 4 or the period between packet 6 and 9 is longer than 3*PTO
  3. The ACK doesn't establish loss of all tail packets, so there's no persistent congestion.

In an offline discussion, both @mjoras and @goelvidhi expressed preferences for different options here. I'm not sure what the right thing to do is here, but at least the draft should be clearer on what exactly establishes persistent congestion and what doesn't.

@kazuho
Copy link
Member

kazuho commented Jul 22, 2020

Consider the case where the sender receives an acknowledgement for packets from 5 to 10, and the period between 1 and 4 was longer than 3*PTO. IIUC, this is clearly a persistent congestion.

The original example is strictly worse than that, and I think it would make sense to declare persistent congestion in the case too. In other words, I think that 3 is incorrect.

That said, I do not have a strong opinion regarding if we should specifically recommend either of 1 or 2, considering that the logic (or the time threshold) being used to detect persistent congestion is merely a recommendation.

@kazuho
Copy link
Member

kazuho commented Jul 22, 2020

FWIW, we had some discussion regarding the design rationale behind persistent congestion in #3875, and it seemed to me that it was mostly about the amount of time the sender cannot make any progress.

@larseggert
Copy link
Member

#3939 seems also related

@larseggert larseggert added this to Triage in Late Stage Processing via automation Jul 22, 2020
@goelvidhi
Copy link

I think we should keep it simple, which IMO is do nothing.
Or if we want to add a recommendation, we can say that you start with the largest lost packet and try to detect Persistent congestion until either you have found 1 occurrence or reached the end of list.

facebook-github-bot pushed a commit to facebook/mvfst that referenced this issue Jul 24, 2020
Summary:
There's a lot of discussion about this in the WG. See:

quicwg/base-drafts#3937

quicwg/base-drafts#3939

quicwg/base-drafts#3875

Our approach is very conservative, which may mean we are declaring persistent congestion mor frequently than we need to. This seems to work okay, but put a TODO to remind us to experiment with this in the future.

Reviewed By: yangchi

Differential Revision: D22699664

fbshipit-source-id: c0b73fc48a9f3dd141cada95eeb8493d17322702
@janaiyengar
Copy link
Contributor

This is fixed in #3961, and is an editorial fix.

@janaiyengar janaiyengar added the editorial An issue that does not affect the design of the protocol; does not require consensus. label Jul 28, 2020
@project-bot project-bot bot moved this from Triage to Editorial Issues in Late Stage Processing Jul 28, 2020
Late Stage Processing automation moved this from Editorial Issues to Issue Handled Sep 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
-recovery editorial An issue that does not affect the design of the protocol; does not require consensus.
Projects
Late Stage Processing
  
Issue Handled
Development

Successfully merging a pull request may close this issue.

5 participants