-
Notifications
You must be signed in to change notification settings - Fork 87
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
TCP: avoid stall with larger MSS #493
Open
djs55
wants to merge
2
commits into
mirage:main
Choose a base branch
from
djs55:avoid-stall-larger-mss
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
If the connection requests a large MSS > 16k, for example to exploit a large MTU, then writes block because available space is believed to be 0. Consider UTX.available: ``` let available t = let a = Int32.sub t.max_size t.bufbytes in match a < (Int32.of_int (Window.tx_mss t.wnd)) with | true -> 0l | false -> a ``` Initially max_size = 16k (hardcoded in flow.ml) and bufbytes = 0, so a = 16k (meaning 16k space is free in the buffer). If the free space (a) is less than an MSS, we return 0 and the connection stalls. I think the assumption is that the UTX can buffer at least 2*MSS worth of data so that when the free space (a) is less than an MSS, the buffer already contains an MSS worth of data to transmit to make progress. Bump the user buffer size to 128k which is 2x a 64k MSS (where 64k is the max value of the 16-bit MSS option). This might need rethinking if we support segmentation offload because we might see even bigger segments. Signed-off-by: David Scott <[email protected]>
avsm
reviewed
Oct 4, 2022
I'll revisit thinking about this one when:
... just to be able to look at a whole feature patchset. Mainly the interactions with TSO, as you note. |
djs55
added a commit
to djs55/vpnkit
that referenced
this pull request
Oct 13, 2022
- mirage/mirage-tcpip#492 : remove 4000 byte maximum - mirage/mirage-tcpip#493 : avoid stall with large MSS Signed-off-by: David Scott <[email protected]>
djs55
added a commit
to djs55/vpnkit
that referenced
this pull request
Oct 13, 2022
- mirage/mirage-tcpip#492 : remove 4000 byte maximum - mirage/mirage-tcpip#493 : avoid stall with large MSS Signed-off-by: David Scott <[email protected]>
djs55
added a commit
to djs55/vpnkit
that referenced
this pull request
Oct 13, 2022
- mirage/mirage-tcpip#492 : remove 4000 byte maximum - mirage/mirage-tcpip#493 : avoid stall with large MSS Signed-off-by: David Scott <[email protected]>
djs55
added a commit
to djs55/vpnkit
that referenced
this pull request
Oct 26, 2022
- mirage/mirage-tcpip#492 : remove 4000 byte maximum - mirage/mirage-tcpip#493 : avoid stall with large MSS Signed-off-by: David Scott <[email protected]>
I would like a double check from someone else about this PR. Therefore, I would like to merge (or not) and cut a release 👍 . |
dinosaure
force-pushed
the
avoid-stall-larger-mss
branch
from
February 10, 2023 10:21
97793f2
to
657b7c8
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
(I'm not an expert on this so I might have misunderstood something!)
If the connection requests a large MSS > 16k, for example to exploit a large MTU, then writes block because available space is believed to be 0.
Consider UTX.available:
Initially max_size = 16k (hardcoded in flow.ml) and bufbytes = 0, so a = 16k (meaning 16k space is free in the buffer).
If the free space (a) is less than an MSS, we return 0 and the connection stalls.
I think the assumption is that the UTX can buffer at least an MSS worth of data so that when the free space (a) is less than an MSS, the buffer already contains an MSS worth of data to transmit to make progress (does this make sense? Or does it only need to contain 1 byte, so it could be MSS + 1?)
Bump the user buffer size to 128k which is 2x a 64k MSS (where 64k is the max value of the 16-bit MSS option). (Does this need to be configurable somewhere? Linux has
ip route add 192.168.1.0/24 dev eth0 advmss 65536
and socket options.)My hope is that for https://github.com/moby/vpnkit which will use a
SOCK_DGRAM
interface tosend
andrecv
ethernet frames via Apple virtualization.framework and qemu, I'll be able to bump the ethernet MTU and TCP MSS to 64k to reduce the number of packets, since there's a syscall overhead per-packet.I'm not completely sure this change is correct and I've not got a 64k MTU/MSS working end-to-end yet. I thought it was worth making the PR for discussion.
This might need rethinking if we support segmentation offload because we might see segments >> MTUs.
Signed-off-by: David Scott [email protected]