-
Notifications
You must be signed in to change notification settings - Fork 15
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
In MirageOS / mirage-net, the MTU is defined as the Ethernet payload #98
Conversation
after sleeping over this, I'd be in favour of removing the code in frontend and backend that deals with "numneeded > 1", since I don't understand how it works, neither where/when this code path is visited (as outlined above, the MirageOS network stack will never send data > mtu + Ethernet_packet.header_size -- and other backends (solo5, unix) do not handle "jumbo frames"...). //cc @talex5 @avsm @djs55 |
I think the way it used to work was that the caller could send pretty large chunks, and netfront would distribute the data over as many blocks as were needed. But this block system won't work with the new fill callbacks, because the callback expects a contiguous block of memory to write to. Originally, blocks were pages, but I divided them in half because that's more efficient for ethernet frames. I guess the network layer is supposed to be independent of ethernet. Perhaps it should be reporting 2048 as its MTU and leave it up to the ethernet layer to limit it to 1500? But mirage-net does indeed define it as the ethernet MTU excluding the header, so this PR looks correct to me. |
thanks @talex5 -- I applied your suggestion to use Cstruct.sub with the size argument. imho this is good to go and fixes concrete issues. the reported issue was in qubes-mirage-firewall, which uses Nat_packet.into_cstruct -- that takes the length of the given Cstruct.t value as mtu: Ethernet.write asked for a 1514 (or 1528) byte cstruct, and intended to pass on a 1500 (or 1514) byte buffer, it actually passed on a 2034 byte buffer to the ip layer (which was then filled by nat_packet). the code as proposed here is now less surprising. (NB: the multi-page code path is not visited, and does a manual copy -- this should be revised if we have such a use case / setup -- i.e. where the mtu is 9000. for the time being, it should work fine (as before)). I also tried to figure out whether |
the travis CI failure looks scary, may it be that the used OS / container has a not-really-working OCaml compiler / toolchain (//cc @avsm due to recent changes for the ocaml base compilers)? #=== ERROR while compiling topkg.1.0.1 ========================================#
# context 2.0.7 | linux/x86_64 | ocaml-base-compiler.4.08.1 | git+file:///home/opam/opam-repository
# path ~/.opam/4.08/.opam-switch/build/topkg.1.0.1
# command ~/.opam/4.08/bin/ocaml pkg/pkg.ml build --pkg-name topkg --dev-pkg false
# exit-code 1
# env-file ~/.opam/log/topkg-6-c0ceac.env
# output-file ~/.opam/log/topkg-6-c0ceac.out
### output ###
# [...]
# pkg.ml: [WARNING] OCaml host-os conf: key ext_obj: undefined, using ".o"
# pkg.ml: [WARNING] OCaml host-os conf: key ext_lib: undefined, using ".a"
# pkg.ml: [WARNING] OCaml host-os conf: key ext_dll: undefined, using ".so"
# pkg.ml: [WARNING] OCaml host-os conf: key ext_exe: undefined and
# no C toolchain detected, using ""
# ocamlfind ocamldep -modules src/topkg.mli > src/topkg.mli.depends
# + ocamlfind ocamlc -where > /home/opam/.opam/4.08/.opam-switch/build/topkg.1.0.1/_build/ocamlc.where
# ocamlfind: Package `threads' not found
# Command exited with code 2.
# pkg.ml: [ERROR] cmd ['ocamlbuild' '-use-ocamlfind' '-classic-display' '-j' '4' '-tag' 'debug'
# '-build-dir' '_build' 'CHANGES.md' 'LICENSE.md' 'README.md' 'pkg/META'
# 'topkg.opam' 'src/topkg.cma' 'src/topkg.cmi' 'src/topkg.mli']: exited with 10 |
This is a bit problematic, since the MTU is to-be-agreed on to both sides of the link. What I can see in QubesOS is that all ethernet interfaces in the Linux VMs use a MTU of 1500 bytes, thus I'd stick with this as the default value. My Internet search for how to lookup the vif mtu / modify it lead to no good results (it looks like xe has a way to modify it -- but the value does not show up in xenstore (in contrast to the comment in the thread) -- http://xen.1045712.n5.nabble.com/Changing-MTU-of-vif-td2569886.html). |
I've enabled ocaml-ci here now. We could just disable Travis. |
CHANGES: * MirageOS (mirage-net) defines the MTU as the link-level payload size, adjust from 1514 to 1500 (@hannesm, mirage/mirage-net-xen#98) * Only pass the sub-buffer of requested size to the fill function (solves mirage/qubes-mirage-firewall#111, @hannesm, mirage/mirage-net-xen#98) * listen: do not catch out of memory exception (@hannesm, mirage/mirage-net-xen#97)
so in xenstore.ml, the
return 1514
was wrong (and fixed by 1500).but I'm still not sure how networking on xen is supposed to work. what I understand from the code is that it takes full pages (4096 bytes), splits them into halves, and fills these as buffers. now, how is this related to the mtu?
the mirage stack, in contrast, respects whatever the mtu returns, and only ever transmits ethernet packets which are smaller than mtu (+ 14 byte ethernet header).
I'm not confident about the
numneeded
etc. code in frontend and backend, especially if it is > 1. what I can reason and understand about is ethernet packets up to 1514 bytes including the ethernet header. what I do not know is what should happen with bigger packets. any hints are welcome. how does xen handle them? should they be fragmented or not?This should fix the immediate issue mirage/qubes-mirage-firewall#111 if
netchannel
is pinned to this branch.