-
Notifications
You must be signed in to change notification settings - Fork 204
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
Remove unnecessary pacing text and reword RTO text #977
Conversation
Was the text about the pacing rate just unnecessary or was it wrong? I recently implemented the pacing rate previously described here in quic-go, and the results so far look promising. |
Not wrong -- it is what GQUIC does -- but those constants don't add much value, IMO. We do 2x during slow start because that's what the Linux fq qdisc does. I'm wary of specifying in an RFC something that's done by one implementation of pacing. You could choose to use 1.25x for the entire thing and I suspect you'll still be quite fine. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This approach does work fairly well, so it's a bit unfortunate to get rid of it. The fact we take these constants from Linux doesn't seem like a major problem to me, given we do that in quite a few places.
I'd like to get a read from the WG about whether they'd like this included, but I'm also happy to chat later today.
Happy to chat, and happy to open an issue for WG consideration. The spec currently does not specify a full pacer, which is appropriately out of scope. Linux's fq is not defined anywhere except in Linux code, and these constants apply to that flavor of pacer. (In contrast, the other constants we use apply to documented mechanisms, but we are borrowing constants only from Linux.) Simply stating these constants is incomplete, which is why I wanted to remove this entirely. I could change the text to have some sort of more generic language about GQUIC and Linux fq implementations; let me give that a shot. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One suggestion unrelated to your changes, LGTM.
draft-ietf-quic-recovery.md
Outdated
acknowledged, then the congestion window remains the same. If no packets prior | ||
to the first retransmission timeout are acknowledged, the retransmission timeout | ||
has been validated and the congestion window must be reduced to the minimum | ||
congestion window and slow start is begun. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: begun is weird in retrospect. Maybe re-entered?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reworded paragraph
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion on making the wording slightly more active
draft-ietf-quic-recovery.md
Outdated
has been validated and the congestion window must be reduced to the minimum | ||
congestion window and slow start is begun. | ||
is made to the congestion window until the next acknowledgement arrives. The | ||
retransmission timeout is considered spurious when an ack is received that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: when the next ack acknowleges?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
draft-ietf-quic-recovery.md
Outdated
is made to the congestion window until the next acknowledgement arrives. The | ||
retransmission timeout is considered spurious when an ack is received that | ||
acknowledges packets sent prior to the first retransmission timeout. The | ||
retransmission timeout is considered valid when an ack is received that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same nit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
Merging. Please add a comment if you want to change something post-merge, happy to do it. |
Nope, LGTM, thanks for the improved text. |
Closes #967