Re: [PATCH v2] qemu: Don't cache NUMA caps

[Daniel Henrique Barboza <danielhb413@xxxxxxxxx>]

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@xxxxxxxxxx>
> ---


Reviewed-by: Daniel Henrique Barboza <danielhb413@xxxxxxxxx>

> 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;
>       }
>