[libvirt] [PATCH v2 4/4] qemu: propagate bridge MTU into qemu "host_mtu" option

[Michal Privoznik]

On 03.02.2017 18:35, Laine Stump wrote:
> libvirt was able to set the host_mtu option when an MTU was explicitly
> given in the interface config (with <mtu size='n'/>), set the MTU of a
> libvirt network in the network config (with the same named
> subelement), and would automatically set the MTU of any tap device to
> the MTU of the network.
> 
> This patch ties that all together (for networks based on tap devices
> and either Linux host bridges or OVS bridges) by learning the MTU of
> the network (i.e. the bridge) during qemuInterfaceBridgeConnect(), and
> returning that value so that it can be passed to qemuBuildNicDevStr();
> qemuBuildNicDevStr() then sets host_mtu in the interface's commandline
> options.
> 
> The result is that a higher MTU for all guests connecting to a
> particular network will be plumbed top to bottom by simply changing
> the MTU of the network (in libvirt's config for libvirt-managed
> networks, or directly on the bridge device for simple host bridges or
> OVS bridges managed outside of libvirt).
> 
> One question I have about this - it occurred to me that in the case of
> migrating a guest from a host with an older libvirt to one with a
> newer libvirt, the guest may have *not* had the host_mtu option on the
> older machine, but *will* have it on the newer machine. I'm curious if
> this could lead to incompatibilities between source and destination (I
> guess it all depends on whether or not the setting of host_mtu has a
> practical effect on a guest that is already running - Maxime?) (I hope
> we don't have to add a "<mtu auto='yes'/>" and only set host_mtu when
> that is present :-/)

This is a question for qemu folks. I know nothing about qemu internals.

> 
> Likewise, we could run into problems when migrating from a newer
> libvirt to older libvirt - The guest would have been told of the
> higher MTU on the newer libvirt, then migrated to a host that didn't
> understand <mtu size='blah'/>. (If this really is a problem, it would
> be a problem with or without the current patch).


Well, if it turns out to be a problem there's yet another variation of it: users can supply new domain XML upon migration and thus change MTU. But that should be easy to check (not that we are doing that now).

> ---
> 
> New in V2.
> 
>  src/qemu/qemu_command.c   | 32 ++++++++++++++++++++++----------
>  src/qemu/qemu_command.h   |  3 ++-
>  src/qemu/qemu_hotplug.c   |  5 +++--
>  src/qemu/qemu_interface.c |  5 +++--
>  src/qemu/qemu_interface.h |  3 ++-
>  5 files changed, 32 insertions(+), 16 deletions(-)
> 
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 6d65872..522152d 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -3555,7 +3555,8 @@ qemuBuildNicDevStr(virDomainDefPtr def,
>                     int vlan,
>                     unsigned int bootindex,
>                     size_t vhostfdSize,
> -                   virQEMUCapsPtr qemuCaps)
> +                   virQEMUCapsPtr qemuCaps,
> +                   unsigned int mtu)
>  {
>      virBuffer buf = VIR_BUFFER_INITIALIZER;
>      const char *nic = net->model;
> @@ -3679,13 +3680,23 @@ qemuBuildNicDevStr(virDomainDefPtr def,
>          virBufferAsprintf(&buf, ",rx_queue_size=%u", net->driver.virtio.rx_queue_size);
>      }
>  
> -    if (usingVirtio && net->mtu) {
> -        if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_NET_HOST_MTU)) {
> -            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> -                           _("setting MTU is not supported with this QEMU binary"));
> -            goto error;
> +    if (usingVirtio && mtu) {
> +        if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_NET_HOST_MTU)) {
> +
> +            virBufferAsprintf(&buf, ",host_mtu=%u", mtu);
> +
> +        } else {
> +            /* log an error if mtu was requested specifically for this
> +             * interface, otherwise, if it's just what was reported by
> +             * the attached network, ignore it.
> +             */
> +            if (net->mtu) {
> +                virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                               _("setting MTU is not supported with "
> +                                 "this QEMU binary"));
> +                goto error;
> +            }
>          }

This requires users to pass net->mtu even though net is already passed into this function. I wonder whether we should alter meaning of @mtu argument slightly. Something like you're going with in 1/4:
- a nonzero value means caller is requesting that particular MTU size
- a zero value means stick with net->mtu value.

Although, now that I've tried and changed code accordingly the difference is just one line changed (apart from these line above):

index 0767c6649..a556dc60a 100644
--- i/src/qemu/qemu_command.c
+++ w/src/qemu/qemu_command.c
@@ -3680,10 +3680,10 @@ qemuBuildNicDevStr(virDomainDefPtr def,
         virBufferAsprintf(&buf, ",rx_queue_size=%u", net->driver.virtio.rx_queue_size);
     }
 
-    if (usingVirtio && mtu) {
+    if (usingVirtio && (net->mtu || mtu)) {
         if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_NET_HOST_MTU)) {
 
-            virBufferAsprintf(&buf, ",host_mtu=%u", mtu);
+            virBufferAsprintf(&buf, ",host_mtu=%u", net->mtu ? net->mtu : mtu);
 
         } else {
             /* log an error if mtu was requested specifically for this
@@ -8053,7 +8053,7 @@ qemuBuildVhostuserCommandLine(virQEMUDriverPtr driver,
     VIR_FREE(netdev);
 
     if (!(nic = qemuBuildNicDevStr(def, net, -1, bootindex,
-                                   queues, qemuCaps, net->mtu))) {
+                                   queues, qemuCaps, 0))) {
         virReportError(VIR_ERR_INTERNAL_ERROR,
                        "%s", _("Error generating NIC -device string"));
         goto error;


This is because we need one bit as well:

@@ -8273,8 +8273,8 @@ qemuBuildInterfaceCommandLine(virQEMUDriverPtr driver,
         }
     }
 
-    if (net->mtu &&
-        virNetDevSetMTU(net->ifname, net->mtu) < 0)
+    if (mtu &&
+        virNetDevSetMTU(net->ifname, mtu) < 0)
         goto cleanup;
 
     if ((actualType == VIR_DOMAIN_NET_TYPE_NETWORK ||
diff --git i/src/qemu/qemu_hotplug.c w/src/qemu/qemu_hotplug.c
index 7784dad3c..a083b2a3f 100644
--- i/src/qemu/qemu_hotplug.c
+++ w/src/qemu/qemu_hotplug.c
@@ -1116,6 +1116,9 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver,
         goto cleanup;
     }
 
+    if (mtu &&
+        virNetDevSetMTU(net->ifname, mtu) < 0)
+
     /* Set device online immediately */
     if (qemuInterfaceStartDevice(net) < 0)
         goto cleanup;

It just feels better than ternary operator. So ACK to whatever version you choose.

Michal