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

Send all packets with the same DSCP field #21

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

remip2
Copy link

@remip2 remip2 commented Sep 2, 2019

irtt --dscp=N sends the first packet with DSCP 0 which can cause issues with stateful network devices on the path which register dscp=0 for the udp pseudo-connection. This fix sets the same DSCP field on the first and following packets.

Copy link
Owner

@heistp heistp left a comment

Choose a reason for hiding this comment

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

Thanks for the pull request. I'll think about taking in this change, but one thing to note is that in the case where the server doesn't support DSCP (or disables it with --no-dscp), the open packet will contain a non-zero DSCP, and each additional packet will contain a 0 DSCP, so this will also confuse any stateful middleboxes which are classifying traffic in this way.

I'm not sure that middleboxes should be using the DSCP value of only the first packet as a class identifier. However, I see that at a minimum, pfSense is doing so (see note under Caveats section: https://docs.netgate.com/pfsense/en/latest/trafficshaper/diffserv.html). The problem with this is that the DSCP value can change at any time as the flow goes through different routes, which may treat packets differently, and may even wipe the dscp field entirely.

So at a minimum, we'd need to stop the client fallback in the case where the server doesn't support DSCP, but I'm still thinking through what the right overall solution is here...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants