Re: [PATCH 05/10] qemu: domain: Add common function to perform memory hotplug checks

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 10/16/2015 08:11 AM, Peter Krempa wrote:
> Add a function that will aggregate various checks related to memory
> hotplug so that they aren't scattered accross various parts of the
> code.
> ---
>  src/qemu/qemu_command.c | 26 ++---------------
>  src/qemu/qemu_domain.c  | 77 +++++++++++++++++++++++++++++++++++++++++++++++++
>  src/qemu/qemu_domain.h  |  4 +++
>  src/qemu/qemu_hotplug.c |  9 ++----
>  4 files changed, 87 insertions(+), 29 deletions(-)
> 
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 43e81c7..4a709db 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -9414,26 +9414,12 @@ qemuBuildCommandLine(virConnectPtr conn,
>          qemuDomainAlignMemorySizes(def) < 0)
>          goto error;
> 
> +    if (qemuDomainDefValidateMemoryHotplug(def, qemuCaps, NULL) < 0)
> +        goto error;
> +
>      virCommandAddArg(cmd, "-m");
> 
>      if (virDomainDefHasMemoryHotplug(def)) {
> -        if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_PC_DIMM)) {
> -            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> -                           _("memory hotplug isn't supported by this QEMU binary"));
> -            goto error;
> -        }
> -
> -        /* due to guest support, qemu would silently enable NUMA with one node
> -         * once the memory hotplug backend is enabled. To avoid possible
> -         * confusion we will enforce user originated numa configuration along
> -         * with memory hotplug. */
> -        if (virDomainNumaGetNodeCount(def->numa) == 0) {
> -            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> -                           _("At least one numa node has to be configured when "
> -                             "enabling memory hotplug"));
> -            goto error;
> -        }
> -
>          /* Use the 'k' suffix to let qemu handle the units */
>          virCommandAddArgFormat(cmd, "size=%lluk,slots=%u,maxmem=%lluk",
>                                 virDomainDefGetMemoryInitial(def),
> @@ -9491,12 +9477,6 @@ qemuBuildCommandLine(virConnectPtr conn,
>      /* memory hotplug requires NUMA to be enabled - we already checked
>       * that memory devices are present only when NUMA is */
> 

Does the comment make sense any more?

> -    if (def->nmems > def->mem.memory_slots) {
> -        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> -                       _("memory device count '%zu' exceeds slots count '%u'"),
> -                       def->nmems, def->mem.memory_slots);
> -        goto error;
> -    }
> 
>      for (i = 0; i < def->nmems; i++) {
>          char *backStr;
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index bdc0e67..cb50754 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -3553,6 +3553,83 @@ qemuDomainMachineIsS390CCW(const virDomainDef *def)
> 
> 
>  /**
> + * qemuDomainDefValidateMemoryHotplug:

Not necessarily called only in Hotplug path, but I understand why it's
named this way since *HasMemoryHotplug uses similar naming and adding a
Capable on the end is an unnecessarily long name.

> + * @def: domain definition
> + * @qemuCaps: qemu capabilities object
> + * @mem: definition of memory device that is to be added to @def with hotplug

or NULL when ...

> + *
> + * Validates that the domain definition and memory modules have valid
> + * configuration and are possibly able to accept @mem via hotplug if it's
> + * passed.
> + *
> + * Returns 0 on success; -1 and a libvirt error on error.
> + */
> +int
> +qemuDomainDefValidateMemoryHotplug(const virDomainDef *def,
> +                                   virQEMUCapsPtr qemuCaps,
> +                                   const virDomainMemoryDef *mem)
> +{
> +    unsigned int nmems = def->nmems;
> +    unsigned long long hotplugSpace;
> +    unsigned long long hotplugMemory = 0;
> +    size_t i;
> +
> +    hotplugSpace = def->mem.max_memory - virDomainDefGetMemoryInitial(def);
> +
> +    if (mem) {
> +        nmems++;
> +        hotplugMemory = mem->size;

I think you'd have to move qemuDomainMemoryDeviceAlignSize to sooner in
the caller so that this calculation and future check is more correct.

> +    }
> +
> +    if (!virDomainDefHasMemoryHotplug(def)) {
> +        if (nmems) {
> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                           _("maxMemory has to be specified when using memory "
> +                             "devices "));

extraneous space                        ^

Although I have to say the check and error reads strange... The *Has
function returns true when memory_slots || max_memory > 0. So while it's
obvious knowing the code and XML that maxMemory needs to be defined in
order for hotplug to work, it just a strange message to see for someone
perhaps passing memory device XML.

Essentially there's a failure because someone was requesting to hotplug
a memory device, but the domain doesn't support it. Perhaps, "cannot
hotplug a memory device when domain 'maxMemory' is not defined" ?


> +            return -1;
> +        }
> +
> +        return 0;
> +    }
> +
> +    if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_PC_DIMM)) {
> +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                       _("memory hotplug isn't supported by this QEMU binary"));
> +        return -1;
> +    }
> +
> +    /* due to guest support, qemu would silently enable NUMA with one node
> +     * once the memory hotplug backend is enabled. To avoid possible
> +     * confusion we will enforce user originated numa configuration along
> +     * with memory hotplug. */
> +    if (virDomainNumaGetNodeCount(def->numa) == 0) {
> +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                       _("At least one numa node has to be configured when "
> +                         "enabling memory hotplug"));
> +        return -1;
> +    }
> +
> +    if (nmems > def->mem.memory_slots) {
> +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                       _("memory device count '%u' exceeds slots count '%u'"),
> +                       nmems, def->mem.memory_slots);

This is one of those cases where different error messages based on
operation may be useful.  In one case (hotplug) one cannot add a memory
device because there's no slots, while the other case (domain startup)
we cannot startup the domain because we have more 'nmems' provided than
'memory_slots' available.

> +        return -1;
> +    }
> +
> +    for (i = 0; i < def->nmems; i++)
> +        hotplugMemory += def->mems[i]->size;
> +
> +    if (hotplugMemory > hotplugSpace) {
> +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                       _("memory device total size exceeds hotplug space"));
> +        return -1;
> +    }
> +
> +    return 0;
> +}
> +
> +
> +/**
>   * qemuDomainUpdateCurrentMemorySize:
>   *
>   * Updates the current balloon size from the monitor if necessary. In case when
> diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
> index 64cd7e1..23f5b1f 100644
> --- a/src/qemu/qemu_domain.h
> +++ b/src/qemu/qemu_domain.h
> @@ -482,4 +482,8 @@ bool qemuDomainMachineIsS390CCW(const virDomainDef *def);
>  int qemuDomainUpdateCurrentMemorySize(virQEMUDriverPtr driver,
>                                        virDomainObjPtr vm);
> 
> +int qemuDomainDefValidateMemoryHotplug(const virDomainDef *def,
> +                                       virQEMUCapsPtr qemuCaps,
> +                                       const virDomainMemoryDef *mem);
> +
>  #endif /* __QEMU_DOMAIN_H__ */
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index afc5408..2c7f165 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -1784,18 +1784,15 @@ qemuDomainAttachMemory(virQEMUDriverPtr driver,
>      int id;
>      int ret = -1;
> 
> -    if (vm->def->nmems == vm->def->mem.memory_slots) {
> -        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> -                       _("no free memory device slot available"));
> -        goto cleanup;
> -    }
> -
>      if (virAsprintf(&mem->info.alias, "dimm%zu", vm->def->nmems) < 0)
>          goto cleanup;
> 
>      if (virAsprintf(&objalias, "mem%s", mem->info.alias) < 0)
>          goto cleanup;
> 
> +    if (qemuDomainDefValidateMemoryHotplug(vm->def, priv->qemuCaps, mem) < 0)
> +        goto cleanup;
> +

Why wait?  It's not like validate routine is checking the alias.

ACK with a couple of tweaks

John

>      if (vm->def->mem.cur_balloon == virDomainDefGetMemoryActual(vm->def))
>          fix_balloon = true;
> 

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]