[PATCH 08/10] qemu: Recalculate balloon on MEMORY_DEVICE_SIZE_CHANGE event and reconnect

[Michal Privoznik]

On 2/2/21 2:32 PM, Peter Krempa wrote:
> On Fri, Jan 22, 2021 at 13:50:30 +0100, Michal Privoznik wrote:
>> Just like we are recalculating the amount of guest memory on
>> BALLOON_CHANGE and on reconnect to the monitor, we should include
>> the actual size of virtio-mem too.
>>
>> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
>> ---
>>   src/qemu/qemu_driver.c  |  3 +++
>>   src/qemu/qemu_process.c | 57 +++++++++++++++++++++++++++++++++--------
>>   2 files changed, 50 insertions(+), 10 deletions(-)
>>
>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>> index d64eb4d399..2fd4429ba8 100644
>> --- a/src/qemu/qemu_driver.c
>> +++ b/src/qemu/qemu_driver.c
>> @@ -4298,6 +4298,9 @@ processMemoryDeviceSizeChange(virQEMUDriverPtr driver,
>>       mem = vm->def->mems[idx];
>>       mem->actualsize = VIR_DIV_UP(info->size, 1024);
>>   
>> +    /* fix the balloon size */
>> +    ignore_value(qemuProcessRefreshBalloonState(driver, vm, QEMU_ASYNC_JOB_NONE));
> 
> During VM lifetime, qemu is actively notifying us about balloon changes
> via an event, so explicit refresh is wrong here.
> 
> You probably want to do the calculation here depending on the old and
> new value instead.
> 
>> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
>> index 8d41f947af..01d261d538 100644
>> --- a/src/qemu/qemu_process.c
>> +++ b/src/qemu/qemu_process.c
>> @@ -1250,10 +1250,31 @@ qemuProcessHandleBalloonChange(qemuMonitorPtr mon G_GNUC_UNUSED,
>>       virQEMUDriverPtr driver = opaque;
>>       virObjectEventPtr event = NULL;
>>       g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
>> +    size_t i;
>>   
>>       virObjectLock(vm);
>>       event = virDomainEventBalloonChangeNewFromObj(vm, actual);
>>   
>> +    VIR_DEBUG("New balloon size before fixup: %lld", actual);
>> +
>> +    for (i = 0; i < vm->def->nmems; i++) {
>> +        virDomainMemoryDefPtr mem = vm->def->mems[i];
>> +
>> +        switch (mem->model) {
>> +        case VIR_DOMAIN_MEMORY_MODEL_VIRTIO_MEM:
>> +            actual += mem->actualsize;
>> +            break;
> 
> So this means that the balloon driver never counts the memory provided
> by any virtio-mem device? If that is so then this is okay, but in that
> case it would be worth explaining it here.
> 

No, balloon does not report virtio-mem memory.

> If it's not the case it's wrong to add the size of the additional
> devices here since you'd count them twice.
> 
> [...]
> 
>> @@ -2451,21 +2472,37 @@ qemuProcessRefreshBalloonState(virQEMUDriverPtr driver,
>>                                  int asyncJob)
>>   {
>>       unsigned long long balloon;
>> +    size_t i;
>>       int rc;
>>   
>> -    /* if no ballooning is available, the current size equals to the current
>> -     * full memory size */
>> -    if (!virDomainDefHasMemballoon(vm->def)) {
>> -        vm->def->mem.cur_balloon = virDomainDefGetMemoryTotal(vm->def);
>> -        return 0;
>> +    if (virDomainDefHasMemballoon(vm->def)) {
>> +        if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0)
>> +            return -1;
>> +
>> +        rc = qemuMonitorGetBalloonInfo(qemuDomainGetMonitor(vm), &balloon);
>> +        if (qemuDomainObjExitMonitor(driver, vm) < 0 || rc < 0)
>> +            return -1;
>> +    } else {
>> +        balloon = virDomainDefGetMemoryTotal(vm->def);
> 
> So the following code is wrong at least in case when this branch is
> taken:
> 
> virDomainDefGetMemoryTotal:
> 
>   * Returns the current maximum memory size usable by the domain described by
>   * @def. This size includes possible additional memory devices.
> 
> The total_memory size is updated in virDomainDefPostParseMemory by
> adding up sizes of all 'mem' devices ...
> 
> 
>>       }
>>   
>> -    if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0)
>> -        return -1;
>> +    for (i = 0; i < vm->def->nmems; i++) {
>> +        virDomainMemoryDefPtr mem = vm->def->mems[i];
>> +
>> +        switch (mem->model) {
>> +        case VIR_DOMAIN_MEMORY_MODEL_VIRTIO_MEM:
>> +            balloon += mem->actualsize;
>> +            break;
> 
> ... and you then add the 'actualsize' component which is a fraction of
> the 'size' of a mem counted previously here.
> 
> Thus when reconnecting to a VM which doesn't have ballon, but uses
> virtio-mem, the virtio-mem is counted twice. (This doesn't happen on
> refresh when starting a new VM since no guest code executed yet)
> 

I've spent some time thinking about this patch. Do we really need it? I 
mean, if we did fake balloon size how do users learn the actual size of 
the balloon? On one hand I understand that <currentMemory/> (= 
def->mem.cur_balloon) should reflect the actual size that guest sees 
(balloon + actual size of virtio mem), but on the other - as Jing showed 
there might be quite a few places where we might report just balloon 
size (virsh dommemstat).

Somehow it suddenly feels wrong to mangle with reported balloon size.

Michal