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

use IP_PMTUDISC_PROBE for setting the DF bit on Linux & Windows, add support for darwin & FreeBSD #3378

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

tobyxdd
Copy link
Contributor

@tobyxdd tobyxdd commented Apr 15, 2022

Thank @database64128

I can confirm that this (still) works on Linux & Windows. But we probably need to have someone test it under macOS and FreeBSD.

@codecov
Copy link

codecov bot commented Apr 15, 2022

Codecov Report

Merging #3378 (c3007f8) into master (3324736) will decrease coverage by 0.20%.
The diff coverage is 50.00%.

@@            Coverage Diff             @@
##           master    #3378      +/-   ##
==========================================
- Coverage   85.58%   85.37%   -0.20%     
==========================================
  Files         135      135              
  Lines        9872     9928      +56     
==========================================
+ Hits         8448     8476      +28     
- Misses       1047     1067      +20     
- Partials      377      385       +8     
Impacted Files Coverage Δ
sys_conn_df_windows.go 52.94% <0.00%> (ø)
sys_conn_df_linux.go 52.94% <66.67%> (ø)
http3/client.go 82.26% <0.00%> (-7.03%) ⬇️
http3/body.go 95.56% <0.00%> (-4.44%) ⬇️
http3/response_writer.go 76.47% <0.00%> (-2.70%) ⬇️
http3/request.go 91.55% <0.00%> (-2.20%) ⬇️
http3/server.go 71.15% <0.00%> (-2.09%) ⬇️
client.go 78.91% <0.00%> (-0.46%) ⬇️
packet_handler_map.go 74.10% <0.00%> (-0.30%) ⬇️
send_stream.go 93.31% <0.00%> (+0.08%) ⬆️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3324736...c3007f8. Read the comment docs.

Copy link

@database64128 database64128 left a comment

Choose a reason for hiding this comment

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

LGTM.

Copy link
Member

@marten-seemann marten-seemann left a comment

Choose a reason for hiding this comment

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

But we probably need to have someone test it under macOS and FreeBSD.

What exactly do we need to do to test this? Is it sufficient to run a localhost <-> localhost transfer and run Wireshark to see if the DF bit is set?

sys_conn_df_windows.go Outdated Show resolved Hide resolved
// https://microsoft.github.io/windows-docs-rs/doc/windows/Win32/Networking/WinSock/constant.IP_MTU_DISCOVER.html
// https://microsoft.github.io/windows-docs-rs/doc/windows/Win32/Networking/WinSock/constant.IPV6_MTU_DISCOVER.html
IP_MTU_DISCOVER = 14
IPV6_MTU_DISCOVER = 14

Choose a reason for hiding this comment

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

From ws2ipdef.h:

#define IP_MTU_DISCOVER           71 // Set/get path MTU discover state.
#define IPV6_MTU_DISCOVER           71 // Set/get path MTU discover state.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oof. Sorry for the mistake

(cherry picked from commit b6bf9c8)
@tobyxdd
Copy link
Contributor Author

tobyxdd commented Apr 16, 2022

But we probably need to have someone test it under macOS and FreeBSD.

What exactly do we need to do to test this? Is it sufficient to run a localhost <-> localhost transfer and run Wireshark to see if the DF bit is set?

I'm not sure if localhost works the same as real ones. I think the best way is to set up 2 machines (at least one of them should have a path MTU < MaxPacketBufferSize) and run some transfers. Use Wireshark to see the DF bit & packet size.

@database64128
Copy link

I'm not sure if localhost works the same as real ones. I think the best way is to set up 2 machines (at least one of them should have a path MTU < MaxPacketBufferSize) and run some transfers. Use Wireshark to see the DF bit & packet size.

Or we can just assume that it works, and deal with it in the future if someone found a problem and opened an issue. After all, the worst case scenario for macOS and FreeBSD is only like: These new socket options didn't work, and quic-go behaves exactly the same as before.

@marten-seemann marten-seemann self-requested a review April 16, 2022 22:12
@marten-seemann
Copy link
Member

Just tested this PR between a OSX and an Ubuntu. None of the QUIC packets originating from OSX hat DF set, only the packets from Ubuntu did.

@marten-seemann
Copy link
Member

@database64128 @tobyxdd What shall we do with this PR?

@database64128
Copy link

I think it's better than nothing. At least merging this PR makes it easier for someone with macOS or FreeBSD experience to fix this in the future.

@marten-seemann
Copy link
Member

I think it's better than nothing. At least merging this PR makes it easier for someone with macOS or FreeBSD experience to fix this in the future.

According to my test on OSX, it doesn't work on OSX. What does this PR achieve then?

@database64128
Copy link

What does this PR achieve then?

It also fixes the behavior on Linux and Windows by setting the correct IP_MTU_DISCOVER value.

@tobyxdd
Copy link
Contributor Author

tobyxdd commented May 13, 2022

@database64128 @tobyxdd What shall we do with this PR?

I'll try to fix it on macOS. If it's not possible, I can drop macOS support, and at least we still added it for FreeBSD.

don't treat setDF failed as error

(cherry picked from commit 503ec23)
@database64128
Copy link

return to IP_DONTFRAGMENT on Windows for compatibility

IP_PMTUDISC_PROBE allows you to send UDP packets exceeding the system cached PMTU without returning -EMSGSIZE, which is the optimal behavior we want here. This is not the case with IP_DONTFRAGMENT. If you want compatibility with unsupported Windows versions, maybe simply fallback to IP_DONTFRAGMENT when setting IP(V6)_MTU_DISCOVER fails?

@marten-seemann
Copy link
Member

@tobyxdd Any updates?

@tobyxdd
Copy link
Contributor Author

tobyxdd commented Jun 9, 2022

@tobyxdd Any updates?

I'm still alive. Will test it on a Mac within a few days

@tobyxdd
Copy link
Contributor Author

tobyxdd commented Jun 10, 2022

return to IP_DONTFRAGMENT on Windows for compatibility

IP_PMTUDISC_PROBE allows you to send UDP packets exceeding the system cached PMTU without returning -EMSGSIZE, which is the optimal behavior we want here. This is not the case with IP_DONTFRAGMENT. If you want compatibility with unsupported Windows versions, maybe simply fallback to IP_DONTFRAGMENT when setting IP(V6)_MTU_DISCOVER fails?

I don't think the return value matters that much when we have isMsgSizeErr to check and ignore the error anyway.

Compatibility is a real concern as I got some feedback immediately after switching to IP_MTU_DISCOVER in my fork.

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.

None yet

3 participants