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

MUST pace or limit bursts to 10 packets #3106

Merged
merged 3 commits into from
Oct 17, 2019
Merged

Conversation

ianswett
Copy link
Contributor

@ianswett ianswett commented Oct 17, 2019

Came from the discussion of #3094

Copy link
Member

@nibanks nibanks left a comment

Choose a reason for hiding this comment

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

LGTM

Remove the old paragraph
@janaiyengar
Copy link
Contributor

@ianswett : I realized after we did this PR yesterday that this does not fix #3094. I It reduces burstiness in general, which is still a good thing, but it doesn't change slow start increase. I'll send a PR or ask Vidhi if she wants to.

Copy link
Contributor

@janaiyengar janaiyengar left a comment

Choose a reason for hiding this comment

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

Let's get this in -- it addresses some burstiness anyway.

@janaiyengar janaiyengar merged commit 8bd39d9 into master Oct 17, 2019
@janaiyengar janaiyengar deleted the ianswett-limit-bursts branch October 17, 2019 21:57
Implementations MUST either use pacing or limit such bursts to minimum
of 10 * kMaxDatagramSize and max(2* kMaxDatagramSize, 14720)), the same
as the recommended initial congestion window.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry again late here... To we recommend a fixed value of 10 packets or do we recommend to have the burst limit the same as the initial window? E.g. if we decide in future to increase our recommendation for the initial window, should that also increase the recommendation for the burst limit? I guess to be really smart about this, you would need to say that you set the burst but initial window but decrease if you see tail loss in the initial burst (but that might be an overkill).

Copy link
Member

Choose a reason for hiding this comment

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

Or you could say that there are different initial window and burst limits, which is probably OK. I don't think that we need to be that smart though.

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 a good point. We could swap the sentence and say "same as the initial congestion window, which is recommended to be ..." if you think it's preferable. But I'm also not sure it matters much.

Copy link
Member

Choose a reason for hiding this comment

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

I think that explicitly saying "Initial congestion window" here as the primary target of the MUST requirement would be better.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fine for me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PR created, PTAL: #3160

ianswett added a commit that referenced this pull request Oct 28, 2019
ianswett added a commit that referenced this pull request Oct 29, 2019
* MUST limit bursts to the initial congestion window

From discussion on #3106 
#3106 (comment)

* Update draft-ietf-quic-recovery.md

* Update draft-ietf-quic-recovery.md

Co-Authored-By: Martin Thomson <mt@lowentropy.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants