[PATCH 0/2] qemu_domin: Account for NVMe disks when calculating memlock limit on hotplug
*** BLURB HERE *** Michal Prívozník (2): qemu_domin: Account for NVMe disks when calculating memlock limit on hotplug qemu: Drop @forceVFIO argument of qemuDomainGetMemLockLimitBytes() and qemuDomainAdjustMaxMemLock() src/qemu/qemu_domain.c | 75 +++++++++++++++++++++++++++-------------- src/qemu/qemu_domain.h | 9 ++--- src/qemu/qemu_hotplug.c | 16 ++++----- src/qemu/qemu_process.c | 2 +- tests/qemumemlocktest.c | 2 +- 5 files changed, 64 insertions(+), 40 deletions(-) -- 2.39.3
During hotplug of a NVMe disk we need to adjust the memlock limit. The computation of the limit is handled by qemuDomainGetMemLockLimitBytes() which looks at given domain definition and accounts for various device types (as different types require different amounts). But during disk hotplug the disk is not added to domain definition until the very last moment. Therefore, qemuDomainGetMemLockLimitBytes() has this @forceVFIO argument which tells it to assume VFIO even if there are no signs of VFIO in domain definition. And this kind of works, until the amount needed for NVMe disks changed (in v9.3.0-rc1~52). What's missing in the commit is making @forceVFIO behave the same as if there was an NVMe disk present in the domain definition. But, we can do even better - just mimic whatever we're doing for hostdevs. IOW - introduce qemuDomainAdjustMaxMemLockNVMe() that behaves the same as qemuDomainAdjustMaxMemLockHostdev(). There are subtle differences though: 1) qemuDomainAdjustMaxMemLockHostdev() can afford placing hostdev right at the end of vm->def->hostdevs, because the array was already reallocated (at the beginning of qemuDomainAttachHostPCIDevice()). But qemuDomainAdjustMaxMemLockNVMe() doesn't have that luxury. 2) qemuDomainAdjustMaxMemLockHostdev() places a virDomainHostdevDef pointer into domain definition, while qemuDomainStorageSourceAccessModifyNVMe() (which calls qemuDomainAdjustMaxMemLock()) sees a virStorageSource pointer but domain definition contains virDomainDiskDef. But that okay, we can create a dummy disk definition and append it into the domain definition. After this, qemuDomainAdjustMaxMemLock() can be called with @forceVFIO = false, as the disk is now part of domain definition (when computing the new limit). Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2014030#c28 Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_domain.c | 35 ++++++++++++++++++++++++++++++++++- src/qemu/qemu_domain.h | 3 +++ 2 files changed, 37 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index d556e2186c..b5b4184782 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -8026,7 +8026,7 @@ qemuDomainStorageSourceAccessModifyNVMe(virQEMUDriver *driver, goto revoke; } - if (qemuDomainAdjustMaxMemLock(vm, true) < 0) + if (qemuDomainAdjustMaxMemLockNVMe(vm, src) < 0) goto revoke; revoke_maxmemlock = true; @@ -9779,6 +9779,39 @@ qemuDomainAdjustMaxMemLockHostdev(virDomainObj *vm, } +/** + * qemuDomainAdjustMaxMemLockNVMe: + * @vm: domain object + * @src: disk source + * + * Temporarily add the disk source to the domain definition, + * adjust the max memlock based in this new definition and + * restore the original definition. + * + * Returns: 0 on success, + * -1 on failure. + */ +int +qemuDomainAdjustMaxMemLockNVMe(virDomainObj *vm, + virStorageSource *src) +{ + g_autofree virDomainDiskDef *disk = NULL; + int ret = 0; + + disk = g_new0(virDomainDiskDef, 1); + disk->src = src; + + VIR_APPEND_ELEMENT_COPY(vm->def->disks, vm->def->ndisks, disk); + + if (qemuDomainAdjustMaxMemLock(vm, false) < 0) + ret = -1; + + VIR_DELETE_ELEMENT_INPLACE(vm->def->disks, vm->def->ndisks - 1, vm->def->ndisks); + + return ret; +} + + /** * qemuDomainHasVcpuPids: * @vm: Domain object diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index eaa75de3e5..ee2ddda079 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -41,6 +41,7 @@ #include "virdomainmomentobjlist.h" #include "virenum.h" #include "vireventthread.h" +#include "storage_source_conf.h" #define QEMU_DOMAIN_FORMAT_LIVE_FLAGS \ (VIR_DOMAIN_XML_SECURE) @@ -859,6 +860,8 @@ int qemuDomainAdjustMaxMemLock(virDomainObj *vm, bool forceVFIO); int qemuDomainAdjustMaxMemLockHostdev(virDomainObj *vm, virDomainHostdevDef *hostdev); +int qemuDomainAdjustMaxMemLockNVMe(virDomainObj *vm, + virStorageSource *src); int qemuDomainSetMaxMemLock(virDomainObj *vm, unsigned long long limit, unsigned long long *origPtr); -- 2.39.3
s/domin/domain/ in $SUBJ On Tue, May 09, 2023 at 04:38:52PM +0200, Michal Privoznik wrote:
During hotplug of a NVMe disk we need to adjust the memlock limit. The computation of the limit is handled by qemuDomainGetMemLockLimitBytes() which looks at given domain definition and accounts for various device types (as different types require different amounts). But during disk hotplug the disk is not added to domain definition until the very last moment. Therefore, qemuDomainGetMemLockLimitBytes() has this @forceVFIO argument which tells it to assume VFIO even if there are no signs of VFIO in domain definition. And this kind of works, until the amount needed for NVMe disks changed (in v9.3.0-rc1~52). What's missing in the commit is making @forceVFIO behave the same as if there was an NVMe disk present in the domain definition.
But, we can do even better - just mimic whatever we're doing for hostdevs. IOW - introduce qemuDomainAdjustMaxMemLockNVMe() that behaves the same as qemuDomainAdjustMaxMemLockHostdev().
This paragraph sums it up nicely I think, the rest threw me off.
There are subtle differences though:
1) qemuDomainAdjustMaxMemLockHostdev() can afford placing hostdev right at the end of vm->def->hostdevs, because the array was already reallocated (at the beginning of qemuDomainAttachHostPCIDevice()). But qemuDomainAdjustMaxMemLockNVMe() doesn't have that luxury.
2) qemuDomainAdjustMaxMemLockHostdev() places a virDomainHostdevDef pointer into domain definition, while qemuDomainStorageSourceAccessModifyNVMe() (which calls qemuDomainAdjustMaxMemLock()) sees a virStorageSource pointer but domain definition contains virDomainDiskDef. But that
s/that/that's/ maybe?
okay, we can create a dummy disk definition and append it into the domain definition.
After this, qemuDomainAdjustMaxMemLock() can be called with @forceVFIO = false, as the disk is now part of domain definition (when computing the new limit).
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2014030#c28 Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
After previous cleanup, there's not a single caller that would call either qemuDomainGetMemLockLimitBytes() or qemuDomainAdjustMaxMemLock() with @forceVFIO set. All callers pass false. Drop the unneeded argument from both functions. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_domain.c | 42 ++++++++++++++++------------------------- src/qemu/qemu_domain.h | 6 ++---- src/qemu/qemu_hotplug.c | 16 ++++++++-------- src/qemu/qemu_process.c | 2 +- tests/qemumemlocktest.c | 2 +- 5 files changed, 28 insertions(+), 40 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index b5b4184782..fac611d920 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -8040,7 +8040,7 @@ qemuDomainStorageSourceAccessModifyNVMe(virQEMUDriver *driver, revoke: if (revoke_maxmemlock) { - if (qemuDomainAdjustMaxMemLock(vm, false) < 0) + if (qemuDomainAdjustMaxMemLock(vm) < 0) VIR_WARN("Unable to change max memlock limit"); } @@ -9403,14 +9403,12 @@ ppc64VFIODeviceIsNV2Bridge(const char *device) /** * getPPC64MemLockLimitBytes: * @def: domain definition - * @forceVFIO: force VFIO usage * * A PPC64 helper that calculates the memory locking limit in order for * the guest to operate properly. */ static unsigned long long -getPPC64MemLockLimitBytes(virDomainDef *def, - bool forceVFIO) +getPPC64MemLockLimitBytes(virDomainDef *def) { unsigned long long memKB = 0; unsigned long long baseLimit = 0; @@ -9472,10 +9470,10 @@ getPPC64MemLockLimitBytes(virDomainDef *def, 8192; /* NVLink2 support in QEMU is a special case of the passthrough - * mechanics explained in the forceVFIO case below. The GPU RAM - * is placed with a gap after maxMemory. The current QEMU - * implementation puts the NVIDIA RAM above the PCI MMIO, which - * starts at 32TiB and is the MMIO reserved for the guest main RAM. + * mechanics explained below. The GPU RAM is placed with a gap after + * maxMemory. The current QEMU implementation puts the NVIDIA RAM + * above the PCI MMIO, which starts at 32TiB and is the MMIO + * reserved for the guest main RAM. * * This window ends at 64TiB, and this is where the GPUs are being * placed. The next available window size is at 128TiB, and @@ -9496,7 +9494,7 @@ getPPC64MemLockLimitBytes(virDomainDef *def, passthroughLimit = maxMemory + 128 * (1ULL<<30) / 512 * nPCIHostBridges + 8192; - } else if (forceVFIO || qemuDomainNeedsVFIO(def) || virDomainDefHasVDPANet(def)) { + } else if (qemuDomainNeedsVFIO(def) || virDomainDefHasVDPANet(def)) { /* For regular (non-NVLink2 present) VFIO passthrough, the value * of passthroughLimit is: * @@ -9580,20 +9578,16 @@ qemuDomainGetNumVDPANetDevices(const virDomainDef *def) /** * qemuDomainGetMemLockLimitBytes: * @def: domain definition - * @forceVFIO: force VFIO calculation * * Calculate the memory locking limit that needs to be set in order for * the guest to operate properly. The limit depends on a number of factors, * including certain configuration options and less immediately apparent ones * such as the guest architecture or the use of certain devices. - * The @forceVFIO argument can be used to tell this function will use VFIO even - * though @def doesn't indicates so right now. * * Returns: the memory locking limit, or 0 if setting the limit is not needed */ unsigned long long -qemuDomainGetMemLockLimitBytes(virDomainDef *def, - bool forceVFIO) +qemuDomainGetMemLockLimitBytes(virDomainDef *def) { unsigned long long memKB = 0; int nvfio; @@ -9615,7 +9609,7 @@ qemuDomainGetMemLockLimitBytes(virDomainDef *def, return VIR_DOMAIN_MEMORY_PARAM_UNLIMITED; if (ARCH_IS_PPC64(def->os.arch) && def->virtType == VIR_DOMAIN_VIRT_KVM) - return getPPC64MemLockLimitBytes(def, forceVFIO); + return getPPC64MemLockLimitBytes(def); nvfio = qemuDomainGetNumVFIOHostdevs(def); nnvme = qemuDomainGetNumNVMeDisks(def); @@ -9638,7 +9632,7 @@ qemuDomainGetMemLockLimitBytes(virDomainDef *def, * * Note that this may not be valid for all platforms. */ - if (forceVFIO || nvfio || nnvme || nvdpa) { + if (nvfio || nnvme || nvdpa) { /* At present, the full memory needs to be locked for each VFIO / VDPA * NVMe device. For VFIO devices, this only applies when there is a * vIOMMU present. Yes, this may result in a memory limit that is @@ -9650,7 +9644,7 @@ qemuDomainGetMemLockLimitBytes(virDomainDef *def, */ int factor = nvdpa + nnvme; - if (nvfio || forceVFIO) { + if (nvfio) { if (nvfio && def->iommu) factor += nvfio; else @@ -9726,12 +9720,9 @@ qemuDomainSetMaxMemLock(virDomainObj *vm, /** * qemuDomainAdjustMaxMemLock: * @vm: domain - * @forceVFIO: apply VFIO requirements even if vm's def doesn't require it * * Adjust the memory locking limit for the QEMU process associated to @vm, in - * order to comply with VFIO or architecture requirements. If @forceVFIO is - * true then the limit is changed even if nothing in @vm's definition indicates - * so. + * order to comply with VFIO or architecture requirements. * * The limit will not be changed unless doing so is needed; the first time * the limit is changed, the original (default) limit is stored in @vm and @@ -9741,11 +9732,10 @@ qemuDomainSetMaxMemLock(virDomainObj *vm, * Returns: 0 on success, <0 on failure */ int -qemuDomainAdjustMaxMemLock(virDomainObj *vm, - bool forceVFIO) +qemuDomainAdjustMaxMemLock(virDomainObj *vm) { return qemuDomainSetMaxMemLock(vm, - qemuDomainGetMemLockLimitBytes(vm->def, forceVFIO), + qemuDomainGetMemLockLimitBytes(vm->def), &QEMU_DOMAIN_PRIVATE(vm)->originalMemlock); } @@ -9770,7 +9760,7 @@ qemuDomainAdjustMaxMemLockHostdev(virDomainObj *vm, int ret = 0; vm->def->hostdevs[vm->def->nhostdevs++] = hostdev; - if (qemuDomainAdjustMaxMemLock(vm, false) < 0) + if (qemuDomainAdjustMaxMemLock(vm) < 0) ret = -1; vm->def->hostdevs[--(vm->def->nhostdevs)] = NULL; @@ -9803,7 +9793,7 @@ qemuDomainAdjustMaxMemLockNVMe(virDomainObj *vm, VIR_APPEND_ELEMENT_COPY(vm->def->disks, vm->def->ndisks, disk); - if (qemuDomainAdjustMaxMemLock(vm, false) < 0) + if (qemuDomainAdjustMaxMemLock(vm) < 0) ret = -1; VIR_DELETE_ELEMENT_INPLACE(vm->def->disks, vm->def->ndisks - 1, vm->def->ndisks); diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index ee2ddda079..ec9ae75bce 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -854,10 +854,8 @@ bool qemuDomainSupportsPCI(virDomainDef *def, void qemuDomainUpdateCurrentMemorySize(virDomainObj *vm); -unsigned long long qemuDomainGetMemLockLimitBytes(virDomainDef *def, - bool forceVFIO); -int qemuDomainAdjustMaxMemLock(virDomainObj *vm, - bool forceVFIO); +unsigned long long qemuDomainGetMemLockLimitBytes(virDomainDef *def); +int qemuDomainAdjustMaxMemLock(virDomainObj *vm); int qemuDomainAdjustMaxMemLockHostdev(virDomainObj *vm, virDomainHostdevDef *hostdev); int qemuDomainAdjustMaxMemLockNVMe(virDomainObj *vm, diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 54b5a2c2c9..d5148f5815 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1244,7 +1244,7 @@ qemuDomainAttachNetDevice(virQEMUDriver *driver, break; case VIR_DOMAIN_NET_TYPE_VDPA: - if (qemuDomainAdjustMaxMemLock(vm, false) < 0) + if (qemuDomainAdjustMaxMemLock(vm) < 0) goto cleanup; adjustmemlock = true; break; @@ -1417,7 +1417,7 @@ qemuDomainAttachNetDevice(virQEMUDriver *driver, * after all */ if (adjustmemlock) - qemuDomainAdjustMaxMemLock(vm, false); + qemuDomainAdjustMaxMemLock(vm); if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK) { if (conn) @@ -1564,7 +1564,7 @@ qemuDomainAttachHostPCIDevice(virQEMUDriver *driver, if (teardowndevice && qemuDomainNamespaceTeardownHostdev(vm, hostdev) < 0) VIR_WARN("Unable to remove host device from /dev"); - if (teardownmemlock && qemuDomainAdjustMaxMemLock(vm, false) < 0) + if (teardownmemlock && qemuDomainAdjustMaxMemLock(vm) < 0) VIR_WARN("Unable to reset maximum locked memory on hotplug fail"); if (releaseaddr) @@ -2291,7 +2291,7 @@ qemuDomainAttachMemory(virQEMUDriver *driver, if (virDomainMemoryInsert(vm->def, mem) < 0) goto cleanup; - if (qemuDomainAdjustMaxMemLock(vm, false) < 0) + if (qemuDomainAdjustMaxMemLock(vm) < 0) goto removedef; qemuDomainObjEnterMonitor(vm); @@ -2357,7 +2357,7 @@ qemuDomainAttachMemory(virQEMUDriver *driver, /* reset the mlock limit */ virErrorPreserveLast(&orig_err); - ignore_value(qemuDomainAdjustMaxMemLock(vm, false)); + ignore_value(qemuDomainAdjustMaxMemLock(vm)); virErrorRestore(&orig_err); goto audit; @@ -2720,7 +2720,7 @@ qemuDomainAttachMediatedDevice(virQEMUDriver *driver, ret = 0; cleanup: if (ret < 0) { - if (teardownmemlock && qemuDomainAdjustMaxMemLock(vm, false) < 0) + if (teardownmemlock && qemuDomainAdjustMaxMemLock(vm) < 0) VIR_WARN("Unable to reset maximum locked memory on hotplug fail"); if (teardowncgroup && qemuTeardownHostdevCgroup(vm, hostdev) < 0) VIR_WARN("Unable to remove host device cgroup ACL on hotplug fail"); @@ -4583,7 +4583,7 @@ qemuDomainRemoveMemoryDevice(virQEMUDriver *driver, ignore_value(qemuProcessRefreshBalloonState(vm, VIR_ASYNC_JOB_NONE)); /* decrease the mlock limit after memory unplug if necessary */ - ignore_value(qemuDomainAdjustMaxMemLock(vm, false)); + ignore_value(qemuDomainAdjustMaxMemLock(vm)); return 0; } @@ -4690,7 +4690,7 @@ qemuDomainRemoveHostDevice(virQEMUDriver *driver, qemuDomainRemovePCIHostDevice(driver, vm, hostdev); /* QEMU might no longer need to lock as much memory, eg. we just * detached the last VFIO device, so adjust the limit here */ - if (qemuDomainAdjustMaxMemLock(vm, false) < 0) + if (qemuDomainAdjustMaxMemLock(vm) < 0) VIR_WARN("Failed to adjust locked memory limit"); break; case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB: diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 952814d663..721b379381 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -7656,7 +7656,7 @@ qemuProcessLaunch(virConnectPtr conn, /* In some situations, eg. VFIO passthrough, QEMU might need to lock a * significant amount of memory, so we need to set the limit accordingly */ - maxMemLock = qemuDomainGetMemLockLimitBytes(vm->def, false); + maxMemLock = qemuDomainGetMemLockLimitBytes(vm->def); /* For all these settings, zero indicates that the limit should * not be set explicitly and the default/inherited limit should diff --git a/tests/qemumemlocktest.c b/tests/qemumemlocktest.c index c53905a7dd..7d219fcc40 100644 --- a/tests/qemumemlocktest.c +++ b/tests/qemumemlocktest.c @@ -39,7 +39,7 @@ testCompareMemLock(const void *data) return -1; } - return virTestCompareToULL(info->memlock, qemuDomainGetMemLockLimitBytes(def, false)); + return virTestCompareToULL(info->memlock, qemuDomainGetMemLockLimitBytes(def)); } static int -- 2.39.3
On Tue, May 09, 2023 at 04:38:53PM +0200, Michal Privoznik wrote:
After previous cleanup, there's not a single caller that would call either qemuDomainGetMemLockLimitBytes() or qemuDomainAdjustMaxMemLock() with @forceVFIO set. All callers pass false.
Drop the unneeded argument from both functions.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
--- src/qemu/qemu_domain.c | 42 ++++++++++++++++------------------------- src/qemu/qemu_domain.h | 6 ++---- src/qemu/qemu_hotplug.c | 16 ++++++++-------- src/qemu/qemu_process.c | 2 +- tests/qemumemlocktest.c | 2 +- 5 files changed, 28 insertions(+), 40 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index b5b4184782..fac611d920 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -9650,7 +9644,7 @@ qemuDomainGetMemLockLimitBytes(virDomainDef *def, */ int factor = nvdpa + nnvme;
- if (nvfio || forceVFIO) { + if (nvfio) { if (nvfio && def->iommu)
You can drop the 'nvfio && ' part from here too.
participants (2)
-
Martin Kletzander -
Michal Privoznik