Re: [PATCH v1 23/26] qemu: Wire up MEMORY_DEVICE_SIZE_CHANGE event

[Daniel Henrique Barboza <danielhb413@xxxxxxxxx>]

On 11/27/20 12:03 PM, 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.
>
> TODO: Fix up total domain memory calculation.

I don't mind the 'TODO' here, but it would be good to clarify what
we can/can't expect while this isn't looked at.

E.g. I took the series for a spin in my x86 dev box (apparently pSeries
does no support virtio-pmem and virtio-mem, so here I am doing x86
work hehe) and I saw that  'virsh setmem --virtio' does not update the
'maxMemory' of the live XML. Is that the intended effect of this pending
'TODO' the commit is referring to?


Code LGTM.


Reviewed-by: Daniel Henrique Barboza <danielhb413@xxxxxxxxx>


> Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx>
> ---
>   docs/formatdomain.rst         |  7 ++++++
>   docs/schemas/domaincommon.rng |  5 +++++
>   src/conf/domain_conf.c        | 24 ++++++++++++++++++--
>   src/conf/domain_conf.h        |  7 ++++++
>   src/libvirt_private.syms      |  1 +
>   src/qemu/qemu_domain.c        |  3 +++
>   src/qemu/qemu_domain.h        |  1 +
>   src/qemu/qemu_driver.c        | 33 ++++++++++++++++++++++++++++
>   src/qemu/qemu_monitor.c       | 24 ++++++++++++++++++++
>   src/qemu/qemu_monitor.h       | 20 +++++++++++++++++
>   src/qemu/qemu_monitor_json.c  | 24 ++++++++++++++++++++
>   src/qemu/qemu_process.c       | 41 +++++++++++++++++++++++++++++++++++
>   12 files changed, 188 insertions(+), 2 deletions(-)
>
> diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
> index 3990728939..ac87d03b33 100644
> --- a/docs/formatdomain.rst
> +++ b/docs/formatdomain.rst
> @@ -7196,6 +7196,7 @@ Example: usage of the memory devices
>            <node>0</node>
>            <block unit='KiB'>2048</block>
>            <requested unit='KiB'>524288</requested>
> +         <actual unit='KiB'>524288</requested>
>          </target>
>        </memory>
>        <memory model='virtio' access='shared'>
> @@ -7322,6 +7323,12 @@ Example: usage of the memory devices
>        granularity. This is valid for ``virtio`` model only and mutually
>        exclusive with ``pmem``.
>   
> +   ``actual``
> +     The active XML for a ``virtio`` model may contain ``actual`` element that
> +     reflects the actual size of the corresponding virtio memory device. The
> +     element is formatted into live XML and never parsed, i.e. it is
> +     output-only element.
> +
>   :anchor:`<a id="elementsIommu"/>`
>   
>   IOMMU devices
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index d478b639fa..3b12902e04 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -6063,6 +6063,11 @@
>               <ref name="scaledInteger"/>
>             </element>
>           </optional>
> +        <optional>
> +          <element name="actual">
> +            <ref name="scaledInteger"/>
> +          </element>
> +        </optional>
>           <optional>
>             <element name="node">
>               <ref name="unsignedInt"/>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 5db1fee16b..05f5d70cee 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -18804,6 +18804,21 @@ virDomainMemoryFindByDeviceInfo(virDomainDefPtr def,
>   }
>   
>   
> +ssize_t
> +virDomainMemoryFindByDeviceAlias(virDomainDefPtr def,
> +                                 const char *alias)
> +{
> +    size_t i;
> +
> +    for (i = 0; i < def->nmems; i++) {
> +        if (STREQ_NULLABLE(def->mems[i]->info.alias, alias))
> +            return i;
> +    }
> +
> +    return -1;
> +}
> +
> +
>   /**
>    * virDomainMemoryInsert:
>    *
> @@ -28124,7 +28139,8 @@ virDomainMemorySourceDefFormat(virBufferPtr buf,
>   
>   static void
>   virDomainMemoryTargetDefFormat(virBufferPtr buf,
> -                               virDomainMemoryDefPtr def)
> +                               virDomainMemoryDefPtr def,
> +                               unsigned int flags)
>   {
>       g_auto(virBuffer) childBuf = VIR_BUFFER_INIT_CHILD(buf);
>   
> @@ -28146,6 +28162,10 @@ virDomainMemoryTargetDefFormat(virBufferPtr buf,
>   
>           virBufferAsprintf(&childBuf, "<requested unit='KiB'>%llu</requested>\n",
>                             def->requestedsize);
> +        if (!(flags & VIR_DOMAIN_DEF_FORMAT_INACTIVE)) {
> +            virBufferAsprintf(&childBuf, "<actual unit='KiB'>%llu</actual>\n",
> +                              def->actualsize);
> +        }
>       }
>   
>       virXMLFormatElement(buf, "target", NULL, &childBuf);
> @@ -28180,7 +28200,7 @@ virDomainMemoryDefFormat(virBufferPtr buf,
>       if (virDomainMemorySourceDefFormat(buf, def) < 0)
>           return -1;
>   
> -    virDomainMemoryTargetDefFormat(buf, def);
> +    virDomainMemoryTargetDefFormat(buf, def, flags);
>   
>       virDomainDeviceInfoFormat(buf, &def->info, flags);
>   
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 31892c4941..633c07b59c 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -2342,6 +2342,9 @@ struct _virDomainMemoryDef {
>       unsigned long long labelsize; /* kibibytes; valid only for NVDIMM */
>       unsigned long long blocksize; /* kibibytes, valid for virtio-mem only */
>       unsigned long long requestedsize; /* kibibytes, valid for virtio-mem only */
> +    unsigned long long actualsize; /* kibibytes, valid for virtio-mem and
> +                                      active domain only, only to report never
> +                                      parse */
>       bool readonly; /* valid only for NVDIMM */
>   
>       /* required for QEMU NVDIMM ppc64 support */
> @@ -3584,6 +3587,10 @@ ssize_t virDomainMemoryFindByDeviceInfo(virDomainDefPtr dev,
>                                           virDomainDeviceInfoPtr info)
>       ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) G_GNUC_WARN_UNUSED_RESULT;
>   
> +ssize_t virDomainMemoryFindByDeviceAlias(virDomainDefPtr def,
> +                                         const char *alias)
> +    ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) G_GNUC_WARN_UNUSED_RESULT;
> +
>   int virDomainShmemDefInsert(virDomainDefPtr def, virDomainShmemDefPtr shmem)
>       ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) G_GNUC_WARN_UNUSED_RESULT;
>   bool virDomainShmemDefEquals(virDomainShmemDefPtr src, virDomainShmemDefPtr dst)
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 1bed019aac..0fdec594ba 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -497,6 +497,7 @@ virDomainMemballoonModelTypeFromString;
>   virDomainMemballoonModelTypeToString;
>   virDomainMemoryDefFree;
>   virDomainMemoryFindByDef;
> +virDomainMemoryFindByDeviceAlias;
>   virDomainMemoryFindByDeviceInfo;
>   virDomainMemoryFindInactiveByDef;
>   virDomainMemoryInsert;
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index ab7938a355..fc994ec282 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -10554,6 +10554,9 @@ qemuProcessEventFree(struct qemuProcessEvent *event)
>       case QEMU_PROCESS_EVENT_JOB_STATUS_CHANGE:
>           virObjectUnref(event->data);
>           break;
> +    case QEMU_PROCESS_EVENT_MEMORY_DEVICE_SIZE_CHANGE:
> +        qemuMonitorMemoryDeviceSizeChangeFree(event->data);
> +        break;
>       case QEMU_PROCESS_EVENT_PR_DISCONNECT:
>       case QEMU_PROCESS_EVENT_LAST:
>           break;
> diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
> index 010bae285d..376679da77 100644
> --- a/src/qemu/qemu_domain.h
> +++ b/src/qemu/qemu_domain.h
> @@ -441,6 +441,7 @@ typedef enum {
>       QEMU_PROCESS_EVENT_PR_DISCONNECT,
>       QEMU_PROCESS_EVENT_RDMA_GID_STATUS_CHANGED,
>       QEMU_PROCESS_EVENT_GUEST_CRASHLOADED,
> +    QEMU_PROCESS_EVENT_MEMORY_DEVICE_SIZE_CHANGE,
>   
>       QEMU_PROCESS_EVENT_LAST
>   } qemuProcessEventType;
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 677f921920..0c5db18dff 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -4289,6 +4289,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];
> +    mem->actualsize = info->size / 1024;
> +
> + endjob:
> +    qemuDomainObjEndJob(driver, vm);
> +}
> +
> +
>   static void qemuProcessEventHandler(void *data, void *opaque)
>   {
>       struct qemuProcessEvent *processEvent = data;
> @@ -4338,6 +4368,9 @@ static void qemuProcessEventHandler(void *data, void *opaque)
>       case QEMU_PROCESS_EVENT_GUEST_CRASHLOADED:
>           processGuestCrashloadedEvent(driver, vm);
>           break;
> +    case QEMU_PROCESS_EVENT_MEMORY_DEVICE_SIZE_CHANGE:
> +        processMemoryDeviceSizeChange(driver, vm, processEvent->data);
> +        break;
>       case QEMU_PROCESS_EVENT_LAST:
>           break;
>       }
> diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
> index ace7c889d4..5f1cae9f48 100644
> --- a/src/qemu/qemu_monitor.c
> +++ b/src/qemu/qemu_monitor.c
> @@ -1435,6 +1435,20 @@ qemuMonitorEmitSpiceMigrated(qemuMonitorPtr mon)
>   }
>   
>   
> +int
> +qemuMonitorEmitMemoryDeviceSizeChange(qemuMonitorPtr mon,
> +                                      const char *devAlias,
> +                                      unsigned long long size)
> +{
> +    int ret = -1;
> +    VIR_DEBUG("mon=%p, devAlias='%s', size=%llu", mon, devAlias, size);
> +
> +    QEMU_MONITOR_CALLBACK(mon, ret, domainMemoryDeviceSizeChange, mon->vm, devAlias, size);
> +
> +    return ret;
> +}
> +
> +
>   int
>   qemuMonitorEmitMemoryFailure(qemuMonitorPtr mon,
>                                qemuMonitorEventMemoryFailurePtr mfp)
> @@ -4456,6 +4470,16 @@ qemuMonitorEventRdmaGidStatusFree(qemuMonitorRdmaGidStatusPtr info)
>   }
>   
>   
> +void
> +qemuMonitorMemoryDeviceSizeChangeFree(qemuMonitorMemoryDeviceSizeChangePtr info)
> +{
> +    if (!info)
> +        return;
> +
> +    VIR_FREE(info->devAlias);
> +}
> +
> +
>   int
>   qemuMonitorSetWatchdogAction(qemuMonitorPtr mon,
>                                const char *action)
> diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
> index c792c95c46..63c52ce6e8 100644
> --- a/src/qemu/qemu_monitor.h
> +++ b/src/qemu/qemu_monitor.h
> @@ -108,6 +108,14 @@ struct _qemuMonitorRdmaGidStatus {
>   };
>   
>   
> +typedef struct _qemuMonitorMemoryDeviceSizeChange qemuMonitorMemoryDeviceSizeChange;
> +typedef qemuMonitorMemoryDeviceSizeChange *qemuMonitorMemoryDeviceSizeChangePtr;
> +struct _qemuMonitorMemoryDeviceSizeChange {
> +    char *devAlias;
> +    unsigned long long size;
> +};
> +
> +
>   typedef enum {
>       QEMU_MONITOR_JOB_TYPE_UNKNOWN, /* internal value, not exposed by qemu */
>       QEMU_MONITOR_JOB_TYPE_COMMIT,
> @@ -153,6 +161,7 @@ struct _qemuMonitorJobInfo {
>   char *qemuMonitorGuestPanicEventInfoFormatMsg(qemuMonitorEventPanicInfoPtr info);
>   void qemuMonitorEventPanicInfoFree(qemuMonitorEventPanicInfoPtr info);
>   void qemuMonitorEventRdmaGidStatusFree(qemuMonitorRdmaGidStatusPtr info);
> +void qemuMonitorMemoryDeviceSizeChangeFree(qemuMonitorMemoryDeviceSizeChangePtr info);
>   
>   typedef void (*qemuMonitorDestroyCallback)(qemuMonitorPtr mon,
>                                              virDomainObjPtr vm,
> @@ -374,6 +383,12 @@ typedef int (*qemuMonitorDomainMemoryFailureCallback)(qemuMonitorPtr mon,
>                                                         qemuMonitorEventMemoryFailurePtr mfp,
>                                                         void *opaque);
>   
> +typedef int (*qemuMonitorDomainMemoryDeviceSizeChange)(qemuMonitorPtr mon,
> +                                                       virDomainObjPtr vm,
> +                                                       const char *alias,
> +                                                       unsigned long long size,
> +                                                       void *opaque);
> +
>   typedef struct _qemuMonitorCallbacks qemuMonitorCallbacks;
>   typedef qemuMonitorCallbacks *qemuMonitorCallbacksPtr;
>   struct _qemuMonitorCallbacks {
> @@ -411,6 +426,7 @@ struct _qemuMonitorCallbacks {
>       qemuMonitorDomainRdmaGidStatusChangedCallback domainRdmaGidStatusChanged;
>       qemuMonitorDomainGuestCrashloadedCallback domainGuestCrashloaded;
>       qemuMonitorDomainMemoryFailureCallback domainMemoryFailure;
> +    qemuMonitorDomainMemoryDeviceSizeChange domainMemoryDeviceSizeChange;
>   };
>   
>   qemuMonitorPtr qemuMonitorOpen(virDomainObjPtr vm,
> @@ -511,6 +527,10 @@ int qemuMonitorEmitSerialChange(qemuMonitorPtr mon,
>                                   bool connected);
>   int qemuMonitorEmitSpiceMigrated(qemuMonitorPtr mon);
>   
> +int qemuMonitorEmitMemoryDeviceSizeChange(qemuMonitorPtr mon,
> +                                          const char *devAlias,
> +                                          unsigned long long size);
> +
>   int qemuMonitorEmitMemoryFailure(qemuMonitorPtr mon,
>                                    qemuMonitorEventMemoryFailurePtr mfp);
>   
> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> index 3d94181afb..0c050b27b7 100644
> --- a/src/qemu/qemu_monitor_json.c
> +++ b/src/qemu/qemu_monitor_json.c
> @@ -113,6 +113,7 @@ static void qemuMonitorJSONHandleDumpCompleted(qemuMonitorPtr mon, virJSONValueP
>   static void qemuMonitorJSONHandlePRManagerStatusChanged(qemuMonitorPtr mon, virJSONValuePtr data);
>   static void qemuMonitorJSONHandleRdmaGidStatusChanged(qemuMonitorPtr mon, virJSONValuePtr data);
>   static void qemuMonitorJSONHandleMemoryFailure(qemuMonitorPtr mon, virJSONValuePtr data);
> +static void qemuMonitorJSONHandleMemoryDeviceSizeChange(qemuMonitorPtr mon, virJSONValuePtr data);
>   
>   typedef struct {
>       const char *type;
> @@ -133,6 +134,7 @@ static qemuEventHandler eventHandlers[] = {
>       { "GUEST_CRASHLOADED", qemuMonitorJSONHandleGuestCrashloaded, },
>       { "GUEST_PANICKED", qemuMonitorJSONHandleGuestPanic, },
>       { "JOB_STATUS_CHANGE", qemuMonitorJSONHandleJobStatusChange, },
> +    { "MEMORY_DEVICE_SIZE_CHANGE", qemuMonitorJSONHandleMemoryDeviceSizeChange, },
>       { "MEMORY_FAILURE", qemuMonitorJSONHandleMemoryFailure, },
>       { "MIGRATION", qemuMonitorJSONHandleMigrationStatus, },
>       { "MIGRATION_PASS", qemuMonitorJSONHandleMigrationPass, },
> @@ -1335,6 +1337,28 @@ qemuMonitorJSONHandleSpiceMigrated(qemuMonitorPtr mon,
>   }
>   
>   
> +static void
> +qemuMonitorJSONHandleMemoryDeviceSizeChange(qemuMonitorPtr mon,
> +                                            virJSONValuePtr data)
> +{
> +    const char *name;
> +    unsigned long long size;
> +
> +    if (!(name = virJSONValueObjectGetString(data, "id"))) {
> +        VIR_WARN("missing device alias in MEMORY_DEVICE_SIZE_CHANGE event");
> +        return;
> +    }
> +
> +    if (virJSONValueObjectGetNumberUlong(data, "size", &size) < 0) {
> +        VIR_WARN("missing new size for '%s' in MEMORY_DEVICE_SIZE_CHANGE event", name);
> +        return;
> +    }
> +
> +
> +    qemuMonitorEmitMemoryDeviceSizeChange(mon, name, size);
> +}
> +
> +
>   static void
>   qemuMonitorJSONHandleMemoryFailure(qemuMonitorPtr mon,
>                                      virJSONValuePtr data)
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index e68df5abe7..64576377b8 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -1932,6 +1932,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);
> +    virObjectUnlock(vm);
> +    return ret;
> +}
> +
> +
>   static qemuMonitorCallbacks monitorCallbacks = {
>       .eofNotify = qemuProcessHandleMonitorEOF,
>       .errorNotify = qemuProcessHandleMonitorError,
> @@ -1965,6 +2005,7 @@ static qemuMonitorCallbacks monitorCallbacks = {
>       .domainRdmaGidStatusChanged = qemuProcessHandleRdmaGidStatusChanged,
>       .domainGuestCrashloaded = qemuProcessHandleGuestCrashloaded,
>       .domainMemoryFailure = qemuProcessHandleMemoryFailure,
> +    .domainMemoryDeviceSizeChange = qemuProcessHandleMemoryDeviceSizeChange,
>   };
>   
>   static void