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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
