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

TCP: don't hardcode a maximum write of 4000 bytes #492

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

Conversation

djs55
Copy link
Member

@djs55 djs55 commented Oct 2, 2022

This allows larger MTUs to be used, which helps when there is large per-packet overhead. For example

both expose ethernet frames over SOCK_DGRAM file descriptors, which leads to one syscall per frame.

There doesn't seem to be any assumption that the MTU must fit inside a 4096 byte page, so remove the min.

Signed-off-by: David Scott dave@recoil.org

This allows larger MTUs to be used, which helps when there is large
per-packet overhead. For example

- qemu when using the socket network device
- Apple virtualization.framework

both expose ethernet frames over SOCK_DGRAM file descriptors, which
leads to one syscall per frame.

Signed-off-by: David Scott <dave@recoil.org>
@haesbaert
Copy link
Member

That's nice !
I can promise to have a look but I need some time, same for the second PR.

@avsm
Copy link
Member

avsm commented Oct 4, 2022

Looks write to me (geddit?). I originally set that 4000 as a placeholder to make sure we don't overflow an Io_page, but that shouldn't be an issue any more. Would be good to have one test case with the higher MTU though.

@djs55
Copy link
Member Author

djs55 commented Oct 10, 2022

It looks like this existing test sends a 7000 byte buffer and receives exactly a 7000 byte buffer. I added a pcap around it and it's correct even without this patch.

 % tcpdump -r _build/default/test/foo.pcap                     
reading from file _build/default/test/foo.pcap, link-type EN10MB (Ethernet)
10:40:28.722387 ARP, Request who-has 192.168.1.254 (Broadcast) tell 192.168.1.10, length 28
10:40:28.722429 ARP, Reply 192.168.1.254 is-at 02:50:00:00:00:02 (oui Unknown), length 28
10:40:28.722506 IP 192.168.1.10.dc > 192.168.1.254.echo: Flags [S], seq 3648279901, win 5840, options [mss 8960,wscale 2,eol], length 0
10:40:28.722668 ARP, Reply 192.168.1.10 is-at 02:50:00:00:00:01 (oui Unknown), length 28
10:40:28.722677 ARP, Request who-has 192.168.1.10 (Broadcast) tell 192.168.1.254, length 28
10:40:28.722811 IP 192.168.1.10.dc > 192.168.1.254.echo: Flags [.], ack 3280248939, win 65535, length 0
10:40:28.722901 IP 192.168.1.254.echo > 192.168.1.10.dc: Flags [S.], seq 3280248938, ack 3648279902, win 65535, options [mss 8960,wscale 2,eol], length 0
10:40:28.722980 IP 192.168.1.254.echo > 192.168.1.10.dc: Flags [P.], seq 1:7001, ack 1, win 65535, length 7000

I think what's happening is that write internally loops to fragment the buffer, but it's then being coalesced somewhere else (compactbufs?). In my vpnkit code I'm somehow able to transmit the fragments as they accumulate, so the compaction isn't effective? I'm not sure.

(As an aside it's a pity compactbufs is a Cstruct.concat. I wonder if we could pass around lists of fragments down to writev-style APIs)

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 <dave@recoil.org>
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 <dave@recoil.org>
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 <dave@recoil.org>
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 <dave@recoil.org>
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.

3 participants