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

[Michal Privoznik]

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 at 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