qemu: Recreate NUMA caps when creating connect capabilities

[PATCH] qemu: Recreate NUMA caps when creating connect capabilities

Posted by Michal Privoznik 3 years, 5 months ago

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).

When rebuilding virCaps (e.g. for 'virsh capabilities') we have
to dropped the cached info and rebuild it from scratch so that
the correct runtime info is reported.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1819058
Fixes: 1a1d848694f6c2f1d98a371124928375bc3bb4a3
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---

After this, we are still caching CPU caps, but I'm not sure if that is a
problem. Perhaps if CPU was hotplugged/unplugged? I don't know.

 src/qemu/qemu_conf.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index d6615ca0dd..89a16349bf 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -1380,6 +1380,12 @@ virCapsPtr virQEMUDriverCreateCapabilities(virQEMUDriverPtr driver)
                   "DOI \"%s\"", model, doi);
     }
 
+    /* Forcibly recreate NUMA caps. They are not static
+     * (e.g. size of hugepages pools can change). */
+    qemuDriverLock(driver);
+    g_clear_pointer(&driver->hostnuma, virCapabilitiesHostNUMAUnref);
+    qemuDriverUnlock(driver);
+
     caps->host.numa = virQEMUDriverGetHostNUMACaps(driver);
     caps->host.cpu = virQEMUDriverGetHostCPU(driver);
     return g_steal_pointer(&caps);
-- 
2.26.2

Re: [PATCH] qemu: Recreate NUMA caps when creating connect capabilities

Posted by Peter Krempa 3 years, 5 months ago

On Tue, Dec 01, 2020 at 13:54:29 +0100, 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).
> 
> When rebuilding virCaps (e.g. for 'virsh capabilities') we have
> to dropped the cached info and rebuild it from scratch so that
> the correct runtime info is reported.
> 
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1819058
> Fixes: 1a1d848694f6c2f1d98a371124928375bc3bb4a3
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
> 
> After this, we are still caching CPU caps, but I'm not sure if that is a
> problem. Perhaps if CPU was hotplugged/unplugged? I don't know.
> 
>  src/qemu/qemu_conf.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
> index d6615ca0dd..89a16349bf 100644
> --- a/src/qemu/qemu_conf.c
> +++ b/src/qemu/qemu_conf.c
> @@ -1380,6 +1380,12 @@ virCapsPtr virQEMUDriverCreateCapabilities(virQEMUDriverPtr driver)
>                    "DOI \"%s\"", model, doi);
>      }
>  
> +    /* Forcibly recreate NUMA caps. They are not static
> +     * (e.g. size of hugepages pools can change). */
> +    qemuDriverLock(driver);
> +    g_clear_pointer(&driver->hostnuma, virCapabilitiesHostNUMAUnref);
> +    qemuDriverUnlock(driver);


The documentation for 'driver->hostnuma' says:

    /* Lazy initialized on first use, immutable thereafter.
     * Require lock to get the pointer & do optional initialization
     */
    virCapsHostNUMAPtr hostnuma;

This patch invalidates that. A very very quick glance at the code
suggests that it's okay, since the accessor returns a reference, so the
docs must be adjusted.

Re: [PATCH] qemu: Recreate NUMA caps when creating connect capabilities

Posted by Peter Krempa 3 years, 5 months ago

On Tue, Dec 01, 2020 at 13:54:29 +0100, 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).
> 
> When rebuilding virCaps (e.g. for 'virsh capabilities') we have
> to dropped the cached info and rebuild it from scratch so that
> the correct runtime info is reported.
> 
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1819058
> Fixes: 1a1d848694f6c2f1d98a371124928375bc3bb4a3
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
> 
> After this, we are still caching CPU caps, but I'm not sure if that is a
> problem. Perhaps if CPU was hotplugged/unplugged? I don't know.
> 
>  src/qemu/qemu_conf.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
> index d6615ca0dd..89a16349bf 100644
> --- a/src/qemu/qemu_conf.c
> +++ b/src/qemu/qemu_conf.c
> @@ -1380,6 +1380,12 @@ virCapsPtr virQEMUDriverCreateCapabilities(virQEMUDriverPtr driver)
>                    "DOI \"%s\"", model, doi);
>      }
>  
> +    /* Forcibly recreate NUMA caps. They are not static
> +     * (e.g. size of hugepages pools can change). */
> +    qemuDriverLock(driver);
> +    g_clear_pointer(&driver->hostnuma, virCapabilitiesHostNUMAUnref);
> +    qemuDriverUnlock(driver);

On a second look, this looks really fishy here.

virQEMUDriverCreateCapabilities is meant to convert 'driver' internals
into a virCaps. It has no business modifying 'driver'.


> +
>      caps->host.numa = virQEMUDriverGetHostNUMACaps(driver);

If we always require new numa data, why don't we fetch it here instead
of having the driver code generate it? Also if it needs to be refreshed
on use, maybe caching it is not the best approach in the first place.

Re: [PATCH] qemu: Recreate NUMA caps when creating connect capabilities

Posted by Michal Privoznik 3 years, 5 months ago

On 12/1/20 3:17 PM, Peter Krempa wrote:
> On Tue, Dec 01, 2020 at 13:54:29 +0100, 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).
>>
>> When rebuilding virCaps (e.g. for 'virsh capabilities') we have
>> to dropped the cached info and rebuild it from scratch so that
>> the correct runtime info is reported.
>>
>> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1819058
>> Fixes: 1a1d848694f6c2f1d98a371124928375bc3bb4a3
>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>> ---
>>
>> After this, we are still caching CPU caps, but I'm not sure if that is a
>> problem. Perhaps if CPU was hotplugged/unplugged? I don't know.
>>
>>   src/qemu/qemu_conf.c | 6 ++++++
>>   1 file changed, 6 insertions(+)
>>
>> diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
>> index d6615ca0dd..89a16349bf 100644
>> --- a/src/qemu/qemu_conf.c
>> +++ b/src/qemu/qemu_conf.c
>> @@ -1380,6 +1380,12 @@ virCapsPtr virQEMUDriverCreateCapabilities(virQEMUDriverPtr driver)
>>                     "DOI \"%s\"", model, doi);
>>       }
>>   
>> +    /* Forcibly recreate NUMA caps. They are not static
>> +     * (e.g. size of hugepages pools can change). */
>> +    qemuDriverLock(driver);
>> +    g_clear_pointer(&driver->hostnuma, virCapabilitiesHostNUMAUnref);
>> +    qemuDriverUnlock(driver);
> 
> On a second look, this looks really fishy here.
> 
> virQEMUDriverCreateCapabilities is meant to convert 'driver' internals
> into a virCaps. It has no business modifying 'driver'.

Except it already does that. Like you pointed out in the previous 
e-mail, the driver->hostnuma is lazily initialized on the first use.

I can move this hunk into virQEMUDriverGetCapabilities() under 
refresh=true branch, if that's more acceptable.

> 
> 
>> +
>>       caps->host.numa = virQEMUDriverGetHostNUMACaps(driver);
> 
> If we always require new numa data, why don't we fetch it here instead
> of having the driver code generate it? Also if it needs to be refreshed
> on use, maybe caching it is not the best approach in the first place.
> 

We are using NUMA part when starting up a guest to construct a list of 
CPUs belonging to the nodes on which numad wants to place the guest. And 
I guess that part is static, so caching it makes sense.

I see two ways out:
1) drop caching completely,
2) keep caching for guest startup sake, and in 
virQEMUDriverCreateCapabilities() don't overwrite driver->hostnuma, but 
call virCapabilitiesHostNUMANewHost() to create fresh NUMA caps.

Michal