mirage / mirage-tcpip

TCP/IP networking stack in pure OCaml, using the Mirage platform libraries. Includes IPv4/6, ICMP, and UDP/TCP support.
https://mirage.io
ISC License
339 stars 87 forks source link

TCP: avoid stall with larger MSS #493

Open djs55 opened 2 years ago

djs55 commented 2 years ago

(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:

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 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 to send and recv 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 dave@recoil.org

avsm commented 1 year ago

I'll revisit thinking about this one when:

I've not got a 64k MTU/MSS working end-to-end yet

... just to be able to look at a whole feature patchset. Mainly the interactions with TSO, as you note.

dinosaure commented 1 year ago

I would like a double check from someone else about this PR. Therefore, I would like to merge (or not) and cut a release 👍 .