[libvirt PATCH 0/4] adjust max memlock limit when hotplugging a vDPA device
see patch 4 for the why. Found during testing of https://bugzilla.redhat.com/1939776 Laine Stump (4): qemu_hotplug.c: don't skip cleanup on failures of qemuDomainAttachNetDevice conf: new function virDomainNetRemoveByObj() qemu_hotplug.c: add net devices to the domain list earlier qemu: adjust the maxmemlock limit when hotplugging a vDPA device src/conf/domain_conf.c | 16 ++++++++++++++++ src/conf/domain_conf.h | 1 + src/libvirt_private.syms | 1 + src/qemu/qemu_hotplug.c | 41 +++++++++++++++++++++++++++++++--------- 4 files changed, 50 insertions(+), 9 deletions(-) -- 2.31.1
We have many places where the earliest error returns from a function skip any cleanup label at the bottom (the assumption being that it is so early in the function that there isn't yet anything that needs to be explicitly undone on failure). But in general it is a bad sign if there are any direct "return" statements in a function at any time after there has been a "goto cleanup" - that indicates someone thought that an earlier point in the code had done something needing cleanup, so we shouldn't be skipping it. There were two occurences of a "return -1" after "goto cleanup" in qemuDomainAttachDeviceNet(). The first of these has been around for a very long time (since 2013) and my assumption is that the earlier "goto cleanup" didn't exist at that time (so it was proper), and when the code further up in the function was added, the this return -1 was missed. The second was added during a mass change to check the return from qemuInterfacePrepareSlirp() in several places (commit 99a1cfc43889c6d); in this case it was erroneous from the start. Change both of these "return -1"s to "goto cleanup". Since we already have code paths earlier in the function that goto cleanup, this should not cause any new problem. Signed-off-by: Laine Stump <laine@redhat.com> --- src/qemu/qemu_hotplug.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index c6f275e11d..244cf65c87 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1212,7 +1212,7 @@ qemuDomainAttachNetDevice(virQEMUDriver *driver, /* final validation now that we have full info on the type */ if (qemuDomainValidateActualNetDef(net, priv->qemuCaps) < 0) - return -1; + goto cleanup; actualType = virDomainNetGetActualType(net); @@ -1330,7 +1330,7 @@ qemuDomainAttachNetDevice(virQEMUDriver *driver, int rv = qemuInterfacePrepareSlirp(driver, net, &slirp); if (rv == -1) - return -1; + goto cleanup; if (rv == 0) break; -- 2.31.1
virDomainNetRemove() requires the index of the net device you want to remove from the list, but in some cases you may not have the index handy, only the object itself (or the object may not have been added to the domain's list). virDomainNetRemoveByObj() first tries to find the given object in the nets list, and deletes that if it is found. As with virDomainNetRemove() it always unconditionally tries to remove the device from the hostdevs list (in case it is the ridiculous combined net+hostdev device created for <interface type='hostdev'>). Signed-off-by: Laine Stump <laine@redhat.com> --- src/conf/domain_conf.c | 16 ++++++++++++++++ src/conf/domain_conf.h | 1 + src/libvirt_private.syms | 1 + 3 files changed, 18 insertions(+) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 0c52efa816..22e4ab5661 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -15742,6 +15742,22 @@ virDomainNetRemove(virDomainDef *def, size_t i) } +virDomainNetDef * +virDomainNetRemoveByObj(virDomainDef *def, virDomainNetDef *net) +{ + size_t i; + + /* the device might have been added to hostdevs but not nets */ + virDomainNetRemoveHostdev(def, net); + + for (i = 0; i < def->nnets; i++) { + if (def->nets[i] == net) + VIR_DELETE_ELEMENT(def->nets, i, def->nnets); + } + return net; +} + + int virDomainNetUpdate(virDomainDef *def, size_t netidx, diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 8d64d4a1e2..8b4ed6d7d2 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -3581,6 +3581,7 @@ int virDomainNetUpdate(virDomainDef *def, size_t netidx, virDomainNetDef *newnet int virDomainNetDHCPInterfaces(virDomainDef *def, virDomainInterfacePtr **ifaces); int virDomainNetARPInterfaces(virDomainDef *def, virDomainInterfacePtr **ifaces); virDomainNetDef *virDomainNetRemove(virDomainDef *def, size_t i); +virDomainNetDef *virDomainNetRemoveByObj(virDomainDef *def, virDomainNetDef *net); void virDomainNetRemoveHostdev(virDomainDef *def, virDomainNetDef *net); int virDomainHostdevInsert(virDomainDef *def, virDomainHostdevDef *hostdev); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 7c7627052b..c327098f68 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -544,6 +544,7 @@ virDomainNetModelTypeToString; virDomainNetNotifyActualDevice; virDomainNetReleaseActualDevice; virDomainNetRemove; +virDomainNetRemoveByObj; virDomainNetRemoveHostdev; virDomainNetResolveActualType; virDomainNetSetModelString; -- 2.31.1
An upcoming patch will be checking if the addition of a new net device requires adjusting the domain locked memory limit, which must be done prior to sending the command to qemu to add the new device. But qemuDomainAdjustMaxMemLock() checks all (and only) the devices that are currently in the domain definition, and currently we are adding new net devices to the domain definition only at the very end of the hotplug operation, after qemu has already executed the device_add command. In order for the upcoming patch to work, this patch changes qemuDomainAttachNetDevice() to add the device to the domain nets list at an earlier time. It can't be added until after PCI address and alias name have been determined (because both of those examine existing devices in the domain to figure out a unique value for the new device), but must be done before making the qemu monitor call. Since the device has been added to the list earlier, we need to potentially remove it on failure. This is done by replacing the existing call to virDomainNetRemoveHostdev() (which checks if this is a hostdev net device, and if so removes it from the hostdevs list, since it could have already been added to that list) with a call to the new virDomainNetRemoveByObj(), which looks for the device on both nets and hostdevs lists, and removes it where it finds it. Signed-off-by: Laine Stump <laine@redhat.com> --- src/qemu/qemu_hotplug.c | 25 ++++++++++++++++++------- 1 file changed, 18 insertions(+), 7 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 244cf65c87..dff31666f7 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1196,9 +1196,6 @@ qemuDomainAttachNetDevice(virQEMUDriver *driver, g_autoptr(virConnect) conn = NULL; virErrorPtr save_err = NULL; - /* preallocate new slot for device */ - VIR_REALLOC_N(vm->def->nets, vm->def->nnets + 1); - /* If appropriate, grab a physical device from the configured * network's pool of devices, or resolve bridge device name * to the one defined in the network definition. @@ -1249,6 +1246,18 @@ qemuDomainAttachNetDevice(virQEMUDriver *driver, releaseaddr = true; + /* We've completed all examinations of the full domain definition + * that require the new device to *not* be present (e.g. PCI + * address allocation and alias name assignment) so it is now safe + * to add the new device to the domain's nets list (in order for + * it to be in place for checks that *do* need it present in the + * domain definition, e.g. checking if we need to adjust the + * locked memory limit). This means we will need to remove it if + * there is a failure. + */ + if (VIR_APPEND_ELEMENT_COPY(vm->def->nets, vm->def->nnets, net) < 0) + goto cleanup; + switch (actualType) { case VIR_DOMAIN_NET_TYPE_BRIDGE: case VIR_DOMAIN_NET_TYPE_NETWORK: @@ -1505,9 +1514,7 @@ qemuDomainAttachNetDevice(virQEMUDriver *driver, ret = 0; cleanup: - if (!ret) { - vm->def->nets[vm->def->nnets++] = net; - } else { + if (ret < 0) { virErrorPreserveLast(&save_err); if (releaseaddr) qemuDomainReleaseDeviceAddress(vm, &net->info); @@ -1529,7 +1536,11 @@ qemuDomainAttachNetDevice(virQEMUDriver *driver, qemuDomainNetDeviceVportRemove(net); } - virDomainNetRemoveHostdev(vm->def, net); + /* we had potentially pre-added the device to the domain + * device lists, if so we need to remove it (from def->nets + * and/or def->hostdevs) on failure + */ + virDomainNetRemoveByObj(vm->def, net); if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK) { if (conn) -- 2.31.1
and re-adjust if the hotplug fails. This fixes a bug found during testing of https://bugzilla.redhat.com/1939776, which was supposed to be resolved by commit 98e22ff749, but failed to account for the case of device hotplug. Signed-off-by: Laine Stump <laine@redhat.com> --- src/qemu/qemu_hotplug.c | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index dff31666f7..794c80444f 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1184,6 +1184,7 @@ qemuDomainAttachNetDevice(virQEMUDriver *driver, int ret = -1; bool releaseaddr = false; bool iface_connected = false; + bool adjustmemlock = false; virDomainNetType actualType; const virNetDevBandwidth *actualBandwidth; g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); @@ -1362,6 +1363,10 @@ qemuDomainAttachNetDevice(virQEMUDriver *driver, break; case VIR_DOMAIN_NET_TYPE_VDPA: + if (qemuDomainAdjustMaxMemLock(vm, false) < 0) + goto cleanup; + adjustmemlock = true; + if ((vdpafd = qemuInterfaceVDPAConnect(net)) < 0) goto cleanup; break; @@ -1542,6 +1547,13 @@ qemuDomainAttachNetDevice(virQEMUDriver *driver, */ virDomainNetRemoveByObj(vm->def, net); + /* if we adjusted the memlock limit (for a vDPA device) then + * we need to re-adjust since we won't be using the device + * after all + */ + if (adjustmemlock) + qemuDomainAdjustMaxMemLock(vm, false); + if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK) { if (conn) virDomainNetReleaseActualDevice(conn, vm->def, net); -- 2.31.1
On 5/21/21 9:11 PM, Laine Stump wrote:
see patch 4 for the why.
Found during testing of https://bugzilla.redhat.com/1939776
Laine Stump (4): qemu_hotplug.c: don't skip cleanup on failures of qemuDomainAttachNetDevice conf: new function virDomainNetRemoveByObj() qemu_hotplug.c: add net devices to the domain list earlier qemu: adjust the maxmemlock limit when hotplugging a vDPA device
src/conf/domain_conf.c | 16 ++++++++++++++++ src/conf/domain_conf.h | 1 + src/libvirt_private.syms | 1 + src/qemu/qemu_hotplug.c | 41 +++++++++++++++++++++++++++++++--------- 4 files changed, 50 insertions(+), 9 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
participants (2)
-
Laine Stump -
Michal Prívozník