virtio/net: Advertise MTU to the guest via VIRTIO_NET_F_MTU - #5828

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 82.89%. Comparing base (8efac0b) to head (0f3cf71).

@@            Coverage Diff             @@
##             main    #5828      +/-   ##
==========================================
+ Coverage   82.88%   82.89%   +0.01%     
==========================================
  Files         277      277              
  Lines       30053    30072      +19     
==========================================
+ Hits        24908    24928      +20     
+ Misses       5145     5144       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Hi Anastassios,

Thank you for your PR.

We'll need some integration test for this. Perhaps a test where different TAPs are configured with different MTUs in the host and then checking that the guest sees the MTU we expect for each of them? Also, we should probably mention this change in the CHANGELOG.

Add MTU negotiation support to the virtio-net

This seems to purely be advertisement rather than negotiation, right?

Best,
Ilias

Hi Anastassios. Thanks for sending the new version. Could you rebase on top of main and resolve the conflicts so that we can run the full testing pipeline and make sure everything passes?

Hi Anastassios. Thanks for sending the new version. Could you rebase on top of main and resolve the conflicts so that we can run the full testing pipeline and make sure everything passes?

running the tests locally ATM, trying to resolve some linting issues - will let you know when I'm ready to kick off the CI.

Hi Anastassios. Thanks for sending the new version. Could you rebase on top of main and resolve the conflicts so that we can run the full testing pipeline and make sure everything passes?

running the tests locally ATM, trying to resolve some linting issues - will let you know when I'm ready to kick off the CI.

ok, linters fail with a timeout, but I'm not sure if its this PR changes or some glitch. Can you kick-off the CI and we can iterate?

thanks!

Thinking further about this. I had a quick look at what QEMU does and it doesn't seem to try and detect the MTU. It rather lets the user optionally specify it in the command line. This also works when the backend is something like vhost-user-net which we might want to support in the future. Shall we consider doing the same, i.e. adding an optional 'mtu' setting to the network interface configuration and advertise VIRTIO_NET_F_MTU only when that's set? Would that work for your use-case?

Also a practical difference I noticed is that with VIRTIO_NET_F_MTU set the guest kernel will (rightly) not allow you to configure an MTU higher than what was advertised by the virtio backend. But when VIRTIO_NET_F_MTU is not set there is no limit to what you can configure inside the guest.

Thinking further about this. I had a quick look at what QEMU does and it doesn't seem to try and detect the MTU. It rather lets the user optionally specify it in the command line. This also works when the backend is something like vhost-user-net which we might want to support in the future. Shall we consider doing the same, i.e. adding an optional 'mtu' setting to the network interface configuration and advertise VIRTIO_NET_F_MTU only when that's set? Would that work for your use-case?

totally agree, happy to include this as well as part of this PR.

Also a practical difference I noticed is that with VIRTIO_NET_F_MTU set the guest kernel will (rightly) not allow you to configure an MTU higher than what was advertised by the virtio backend. But when VIRTIO_NET_F_MTU is not set there is no limit to what you can configure inside the guest.

are you concerned about not being able to use larger MTU sizes even when the host is bound to 1500, when(/if) FC virtio-net supports this kind of functionality (LRO/TSO etc.)?

I'm assuming the config option you suggested resolves this issue. If the config is set, then we take into account the MTU set by the host interface. If not, we ignore it (not advertise it via the driver) and the guest can set whatever they want.

totally agree, happy to include this as well as part of this PR.

Sounds good.

I'm assuming the config option you suggested resolves this issue.

Yep.

are you concerned about not being able to use larger MTU sizes even when the host is bound to 1500, when(/if) FC virtio-net supports this kind of functionality (LRO/TSO etc.)?

I guess the current behaviour allows you to do weird things such as increasing both the host TAP MTU and the guest interface MTU after starting the VM. I don't know if anyone does this, but if we tried to set VIRTIO_NET_F_MTU unconditionally then we would break things like that.

If the config is set, then we take into account the MTU set by the host interface.

To be clear, the config option will carry the MTU value, so it's on the administrator/operator to specify the MTU value (rather than Firecracker trying to determine it via sysfs or otherwise).

If not, we ignore it (not advertise it via the driver) and the guest can set whatever they want.

Yes, thus preserving the current behaviour by default.

made an attempt -- have a look at your convenience and let me know if you think the implementation makes sense.

Hi @ilstam! thanks for your comments - did another pass, have a look and let me know!

thanks!

Hey Anastassios. Thanks for the new revision. There are a few test failures in the CI but we should be able to merge this once these are fixed:

test device_manager::pci_mngr::tests::test_device_manager_persistence ... FAILED
test device_manager::persist::tests::test_device_manager_persistence ... FAILED
test rpc_interface::tests::test_runtime_disallowed ... FAILED

[gw0] [ 17%] FAILED integration_tests/functional/test_api.py::test_get_full_config_after_restoring_snapshot[vmlinux-5.10.252-PCI_ON]
[gw0] [ 20%] FAILED integration_tests/functional/test_api.py::test_get_full_config[vmlinux-5.10.252-PCI_ON]

For the integration tests I think this diff should help:

  --- a/tests/integration_tests/functional/test_api.py
  +++ b/tests/integration_tests/functional/test_api.py
  @@ -1347,6 +1347,7 @@
               "guest_mac": net_tools.mac_from_ip(net_iface.guest_ip),
               "iface_id": net_iface.dev_name,
               "host_dev_name": net_iface.tap_name,
  +            "mtu": None,
               "rx_rate_limiter": None,
               "tx_rate_limiter": tx_rl,
           }
  @@ -1479,6 +1480,7 @@
               "iface_id": iface_id,
               "host_dev_name": tap1.name,
               "guest_mac": "06:00:00:00:00:01",
  +            "mtu": None,
               "rx_rate_limiter": None,
               "tx_rate_limiter": tx_rl,
           }

And a similar diff is needed for the Rust tests.

I think you should be able to run these tests locally, but let me know if you need any help.

All tests pass now, just tools/devtool checkstyle is failing:

E   Diff in /firecracker/src/vmm/src/devices/virtio/net/device.rs:1128:
E        };
E        use crate::devices::virtio::net::test_utils::test::TestHelper;
E        use crate::devices::virtio::net::test_utils::{
E   -        NetEvent, NetQueue, TapTrafficSimulator, default_net, enable, if_index, inject_tap_tx_frame,
E   -        set_mac,
E   +        NetEvent, NetQueue, TapTrafficSimulator, default_net, enable, if_index,
E   +        inject_tap_tx_frame, set_mac,
E        };
E        use crate::devices::virtio::queue::VIRTQ_DESC_F_WRITE;
E        use crate::devices::virtio::test_utils::VirtQueue;

All tests pass now, just tools/devtool checkstyle is failing:

I think I was looking at an older CI build, the last one passes. I just left one more comment, otherwise it looks good to me.

thanks @ilstam! Added the extra check for mtu < 68 + a unit test. Also changed the swagger description since it wouldn't make sense to keep a value that would never be used.

I suspect the CI failures are not related to the PR, right?

I suspect the CI failures are not related to the PR, right?

That's correct, there's an issue in our CI pipeline and tests are failing on a certain EC2 instance size. Nothing to do with your PR.

I'm waiting for a second reviewer to approve and then we'll merge this.

Could you permit maintainers to edit this pull request? This will allow us to do the final rebase on top of mainline rather than waiting for you to do it (which will always be racy as there are things merged to mainline constantly).

Could you permit maintainers to edit this pull request? This will allow us to do the final rebase on top of mainline rather than waiting for you to do it (which will always be racy as there are things merged to mainline constantly).

happy to do so, but I don't think it's possible, due to this: https://github.com/orgs/community/discussions/5634 ...

Merged, will be deployed with the next Firecracker release in a couple weeks. Thanks.