-
Notifications
You must be signed in to change notification settings - Fork 6
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
Clarify DSCP #174
Clarify DSCP #174
Conversation
draft-ietf-masque-connect-ip.md
Outdated
When a connection is congestion-controlled, marking packets with different DSCP | ||
can lead to reordering between them, and that can in turn lead the congestion | ||
controller to perform poorly. If tunneled packets are subject to congestion | ||
control by the outer connection, they need to avoid carrying different DSCP |
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.
Do we want to clarify that the possible solutions to this are multiple HTTP/3 connections with independent CONNECT-IP sessions?
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.
Isn't that already there further down in that 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.
Yes, you're right, but I had to read it 3 times to notice. I think it gets missed with the note to scope on particular DSCP values. Maybe let's add a newline and separate that out into its own paragraph, since it's kind of a separate thought?
draft-ietf-masque-connect-ip.md
Outdated
markings to not disrupt the congestion controller. In this scenario, the IP | ||
When a connection is congestion-controlled, marking packets with different DSCP | ||
can lead to reordering between them, and that can in turn lead the congestion | ||
controller to perform poorly. If tunneled packets are subject to congestion |
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.
I would suggest to clarify that it is the "... in turn lead the HTTP connection's congestion controller to perform poorly."
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-masque-connect-ip.md
Outdated
can lead to reordering between them, and that can in turn lead the congestion | ||
controller to perform poorly. If tunneled packets are subject to congestion | ||
control by the outer connection, they need to avoid carrying different DSCP | ||
markings to prevent this situation. In this scenario, the IP |
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.
"carrying different DSCP markings" is over simplifying it. It is carrying DSCP that are not equivalent in forwarding behaviour that is the issue.
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
the tunneled packets need to be treated equally regardless of their DSCP | ||
markings to not disrupt the congestion controller. In this scenario, the IP | ||
When an HTTP connection is congestion-controlled, marking packets with | ||
different DSCP can lead to reordering between them, and that can in turn lead |
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.
It not only reordering. Also getting different delay samples can break delay based cc.
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.
Do we need to go that much into detail?
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.
I think you simplification works fine here. As the relevant aspect cut out appear to covered in later sentences.
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.
I know this has been merged now but this comment of mine was addressed. reordering alone is not the problem.
Fixes #173