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