[PATCH v2] qemu: Don't cache NUMA caps
Daniel Henrique Barboza
danielhb413 at gmail.com
Wed Dec 2 14:04:44 UTC 2020
On 12/2/20 5:26 AM, Michal Privoznik wrote:
> In v6.0.0-rc1~439 (and friends) we tried to cache NUMA
> capabilities because we assumed they are immutable. And to some
> extent they are (NUMA hotplug is not a thing, is it). However,
> our capabilities contain also some runtime info that can change,
> e.g. hugepages pool allocation sizes or total amount of memory
> per node (host side memory hotplug might change the value).
>
> Because of the caching we might not be reporting the correct
> runtime info in 'virsh capabilities'.
>
> The NUMA caps are used in three places:
>
> 1) 'virsh capabilities'
> 2) domain startup, when parsing numad reply
> 3) parsing domain private data XML
>
> In cases 2) and 3) we need NUMA caps to construct list of
> physical CPUs that belong to NUMA nodes from numad reply. And
> while this may seem static, it's not really because of possible
> CPU hotplug on physical host.
>
> There are two possible approaches:
>
> 1) build a validation mechanism that would invalidate the
> cached NUMA caps, or
> 2) drop the caching and construct NUMA caps from scratch on
> each use.
>
> In this commit, the latter approach is implemented, because it's
> easier.
>
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1819058
> Fixes: 1a1d848694f6c2f1d98a371124928375bc3bb4a3
> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> ---
Reviewed-by: Daniel Henrique Barboza <danielhb413 at gmail.com>
>
> v2 of:
>
> https://www.redhat.com/archives/libvir-list/2020-December/msg00023.html
>
> diff to v1:
> - instead of fixing cached capabilities, drop caching completely
>
> src/qemu/qemu_conf.c | 23 +----------------------
> src/qemu/qemu_conf.h | 6 ------
> src/qemu/qemu_domain.c | 7 +++----
> src/qemu/qemu_driver.c | 1 -
> src/qemu/qemu_process.c | 7 +++----
> 5 files changed, 7 insertions(+), 37 deletions(-)
>
> diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
> index cbdde0c0dc..0ae86e5468 100644
> --- a/src/qemu/qemu_conf.c
> +++ b/src/qemu/qemu_conf.c
> @@ -1289,27 +1289,6 @@ virQEMUDriverCreateXMLConf(virQEMUDriverPtr driver,
> }
>
>
> -virCapsHostNUMAPtr
> -virQEMUDriverGetHostNUMACaps(virQEMUDriverPtr driver)
> -{
> - virCapsHostNUMAPtr hostnuma;
> -
> - qemuDriverLock(driver);
> -
> - if (!driver->hostnuma)
> - driver->hostnuma = virCapabilitiesHostNUMANewHost();
> -
> - hostnuma = driver->hostnuma;
> -
> - qemuDriverUnlock(driver);
> -
> - if (hostnuma)
> - virCapabilitiesHostNUMARef(hostnuma);
> -
> - return hostnuma;
> -}
> -
> -
> virCPUDefPtr
> virQEMUDriverGetHostCPU(virQEMUDriverPtr driver)
> {
> @@ -1381,7 +1360,7 @@ virCapsPtr virQEMUDriverCreateCapabilities(virQEMUDriverPtr driver)
> "DOI \"%s\"", model, doi);
> }
>
> - caps->host.numa = virQEMUDriverGetHostNUMACaps(driver);
> + caps->host.numa = virCapabilitiesHostNUMANewHost();
> caps->host.cpu = virQEMUDriverGetHostCPU(driver);
> return g_steal_pointer(&caps);
> }
> diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h
> index 411d08db36..7025b5222e 100644
> --- a/src/qemu/qemu_conf.h
> +++ b/src/qemu/qemu_conf.h
> @@ -270,11 +270,6 @@ struct _virQEMUDriver {
> */
> virCapsPtr caps;
>
> - /* Lazy initialized on first use, immutable thereafter.
> - * Require lock to get the pointer & do optional initialization
> - */
> - virCapsHostNUMAPtr hostnuma;
> -
> /* Lazy initialized on first use, immutable thereafter.
> * Require lock to get the pointer & do optional initialization
> */
> @@ -337,7 +332,6 @@ virQEMUDriverConfigSetDefaults(virQEMUDriverConfigPtr cfg);
>
> virQEMUDriverConfigPtr virQEMUDriverGetConfig(virQEMUDriverPtr driver);
>
> -virCapsHostNUMAPtr virQEMUDriverGetHostNUMACaps(virQEMUDriverPtr driver);
> virCPUDefPtr virQEMUDriverGetHostCPU(virQEMUDriverPtr driver);
> virCapsPtr virQEMUDriverCreateCapabilities(virQEMUDriverPtr driver);
> virCapsPtr virQEMUDriverGetCapabilities(virQEMUDriverPtr driver,
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 663c0af867..8e9c0224e4 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -2498,8 +2498,7 @@ qemuDomainObjPrivateXMLParseVcpu(xmlNodePtr node,
>
> static int
> qemuDomainObjPrivateXMLParseAutomaticPlacement(xmlXPathContextPtr ctxt,
> - qemuDomainObjPrivatePtr priv,
> - virQEMUDriverPtr driver)
> + qemuDomainObjPrivatePtr priv)
> {
> g_autoptr(virCapsHostNUMA) caps = NULL;
> g_autofree char *nodeset = NULL;
> @@ -2513,7 +2512,7 @@ qemuDomainObjPrivateXMLParseAutomaticPlacement(xmlXPathContextPtr ctxt,
> if (!nodeset && !cpuset)
> return 0;
>
> - if (!(caps = virQEMUDriverGetHostNUMACaps(driver)))
> + if (!(caps = virCapabilitiesHostNUMANewHost()))
> return -1;
>
> /* Figure out how big the nodeset bitmap needs to be.
> @@ -3114,7 +3113,7 @@ qemuDomainObjPrivateXMLParse(xmlXPathContextPtr ctxt,
> }
> VIR_FREE(nodes);
>
> - if (qemuDomainObjPrivateXMLParseAutomaticPlacement(ctxt, priv, driver) < 0)
> + if (qemuDomainObjPrivateXMLParseAutomaticPlacement(ctxt, priv) < 0)
> goto error;
>
> if ((tmp = virXPathString("string(./libDir/@path)", ctxt)))
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 8eaa3ce68f..d2077f8f16 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -1136,7 +1136,6 @@ qemuStateCleanup(void)
> virObjectUnref(qemu_driver->qemuCapsCache);
> virObjectUnref(qemu_driver->xmlopt);
> virCPUDefFree(qemu_driver->hostcpu);
> - virCapabilitiesHostNUMAUnref(qemu_driver->hostnuma);
> virObjectUnref(qemu_driver->caps);
> ebtablesContextFree(qemu_driver->ebtables);
> VIR_FREE(qemu_driver->qemuImgBinary);
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 579b3c3713..49528f3dab 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -6197,8 +6197,7 @@ qemuProcessUpdateGuestCPU(virDomainDefPtr def,
>
>
> static int
> -qemuProcessPrepareDomainNUMAPlacement(virQEMUDriverPtr driver,
> - virDomainObjPtr vm)
> +qemuProcessPrepareDomainNUMAPlacement(virDomainObjPtr vm)
> {
> qemuDomainObjPrivatePtr priv = vm->privateData;
> g_autofree char *nodeset = NULL;
> @@ -6226,7 +6225,7 @@ qemuProcessPrepareDomainNUMAPlacement(virQEMUDriverPtr driver,
> if (virBitmapParse(nodeset, &numadNodeset, VIR_DOMAIN_CPUMASK_LEN) < 0)
> return -1;
>
> - if (!(caps = virQEMUDriverGetHostNUMACaps(driver)))
> + if (!(caps = virCapabilitiesHostNUMANewHost()))
> return -1;
>
> /* numad may return a nodeset that only contains cpus but cgroups don't play
> @@ -6428,7 +6427,7 @@ qemuProcessPrepareDomain(virQEMUDriverPtr driver,
> }
> virDomainAuditSecurityLabel(vm, true);
>
> - if (qemuProcessPrepareDomainNUMAPlacement(driver, vm) < 0)
> + if (qemuProcessPrepareDomainNUMAPlacement(vm) < 0)
> return -1;
> }
>
>
More information about the libvir-list
mailing list