[PATCH 06/10] qemu: Wire up MEMORY_DEVICE_SIZE_CHANGE event

[Peter Krempa]

On Fri, Jan 22, 2021 at 13:50:28 +0100, Michal Privoznik wrote:
> As advertised in previous commit, this event is delivered to us
> when virtio-mem module changes the allocation inside the guest.
> It comes with one attribute - size - which holds the new size of
> the virtio-mem (well, allocated size), in bytes.
> Mind you, this is not necessarily the same number as 'requested
> size'. It almost certainly will be when sizing the memory up, but
> it might not be when sizing the memory down - the guest kernel
> might be unable to free some blocks.
> 
> This actual size is reported in the domain XML as an output
> element only.
> 
> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> ---

[...]

> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index b6fe5e4436..74c897b53e 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -17320,6 +17320,21 @@ virDomainMemoryFindByDeviceInfo(virDomainDefPtr def,
>  }
>  
>  
> +ssize_t
> +virDomainMemoryFindByDeviceAlias(virDomainDefPtr def,
> +                                 const char *alias)

This is finding the memory index rather than the memory object itself
but the name seems to suggest otherwise.

> +{
> +    size_t i;
> +
> +    for (i = 0; i < def->nmems; i++) {
> +        if (STREQ_NULLABLE(def->mems[i]->info.alias, alias))
> +            return i;
> +    }
> +
> +    return -1;
> +}
> +
> +
>  /**
>   * virDomainMemoryInsert:
>   *

[...]

> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index fadf0240fc..d64eb4d399 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -4273,6 +4273,36 @@ processGuestCrashloadedEvent(virQEMUDriverPtr driver,
>  }
>  
>  
> +static void
> +processMemoryDeviceSizeChange(virQEMUDriverPtr driver,
> +                              virDomainObjPtr vm,
> +                              qemuMonitorMemoryDeviceSizeChangePtr info)
> +{
> +    virDomainMemoryDefPtr mem = NULL;
> +    ssize_t idx;
> +
> +    if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0)
> +        return;
> +
> +    if (!virDomainObjIsActive(vm)) {
> +        VIR_DEBUG("Domain is not running");
> +        goto endjob;
> +    }
> +
> +    idx = virDomainMemoryFindByDeviceAlias(vm->def, info->devAlias);
> +    if (idx < 0) {
> +        VIR_DEBUG("Memory device '%s' not found", info->devAlias);
> +        goto endjob;
> +    }
> +
> +    mem = vm->def->mems[idx];

Why didn't you add a helper which doesn't require doing this weird
lookup instead?

> +    mem->actualsize = VIR_DIV_UP(info->size, 1024);
> +
> + endjob:
> +    qemuDomainObjEndJob(driver, vm);
> +}
> +
> +
>  static void qemuProcessEventHandler(void *data, void *opaque)
>  {
>      struct qemuProcessEvent *processEvent = data;

[...]

> diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
> index 6b3c1c2f5e..59f0265804 100644
> --- a/src/qemu/qemu_monitor.c
> +++ b/src/qemu/qemu_monitor.c

[...]

> @@ -4400,6 +4414,16 @@ qemuMonitorEventRdmaGidStatusFree(qemuMonitorRdmaGidStatusPtr info)
>  }
>  
>  
> +void
> +qemuMonitorMemoryDeviceSizeChangeFree(qemuMonitorMemoryDeviceSizeChangePtr info)
> +{
> +    if (!info)
> +        return;
> +
> +    VIR_FREE(info->devAlias);

Missing free of the whole struct? Also preferably use g_free nowadays.


[...]

> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index f87a3c0f60..37cc6a28e5 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -1937,6 +1937,46 @@ qemuProcessHandleMemoryFailure(qemuMonitorPtr mon G_GNUC_UNUSED,
>  }
>  
>  
> +static int
> +qemuProcessHandleMemoryDeviceSizeChange(qemuMonitorPtr mon G_GNUC_UNUSED,
> +                                        virDomainObjPtr vm,
> +                                        const char *devAlias,
> +                                        unsigned long long size,
> +                                        void *opaque)
> +{
> +    virQEMUDriverPtr driver = opaque;
> +    struct qemuProcessEvent *processEvent = NULL;
> +    qemuMonitorMemoryDeviceSizeChangePtr info = NULL;
> +    int ret = -1;
> +
> +    virObjectLock(vm);
> +
> +    VIR_DEBUG("Memory device '%s' changed size to '%llu' in domain '%s'",
> +              devAlias, size, vm->def->name);
> +
> +    info = g_new0(qemuMonitorMemoryDeviceSizeChange, 1);
> +    info->devAlias = g_strdup(devAlias);
> +    info->size = size;
> +
> +    processEvent = g_new0(struct qemuProcessEvent, 1);
> +    processEvent->eventType = QEMU_PROCESS_EVENT_MEMORY_DEVICE_SIZE_CHANGE;
> +    processEvent->vm = virObjectRef(vm);
> +    processEvent->data = g_steal_pointer(&info);
> +
> +    if (virThreadPoolSendJob(driver->workerPool, 0, processEvent) < 0) {
> +        qemuProcessEventFree(processEvent);
> +        virObjectUnref(vm);
> +        goto cleanup;
> +    }
> +
> +    ret = 0;
> + cleanup:
> +    qemuMonitorMemoryDeviceSizeChangeFree(info);

As noted above, info is leaked since the function doesn't fre it.


> +    virObjectUnlock(vm);
> +    return ret;
> +}

A side note, do we get this event e.g. on VM reset? If no we need to
wire up the reset of 'actual' size in such case as it would wrongly
suggest that the VM is using it and it may not even get to loading the
driver.