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

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(a)redhat.com&gt; > --- > 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.

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)