[libvirt] [PATCH 03/10] qemu: command: Make qemuBuildMemoryBackendStr usable without NUMA

[John Ferlan]

On 10/16/2015 08:11 AM, Peter Krempa wrote:
> Make the function usable so that -1 can be passed to it as cell ID so
> that we can later enable memory hotplug on non-NUMA guests for certain
> architectures.
> 
> I've inspected all functions that take guestNode as an argument to
> verify that they are eiter safe to be called or are not called at all.

either

Although it seems that comment is left for under the --- ...

> ---
>  src/qemu/qemu_command.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index f727d0b..a37a4fa 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -5011,7 +5011,8 @@ qemuBuildControllerDevStr(virDomainDefPtr domainDef,
>   * qemuBuildMemoryBackendStr:
>   * @size: size of the memory device in kibibytes
>   * @pagesize: size of the requested memory page in KiB, 0 for default
> - * @guestNode: NUMA node in the guest that the memory object will be attached to
> + * @guestNode: NUMA node in the guest that the memory object will be attached
> + *             to, or -1 if NUMA is not used in the guest
>   * @hostNodes: map of host nodes to alloc the memory in, NULL for default
>   * @autoNodeset: fallback nodeset in case of automatic numa placement
>   * @def: domain definition object
> @@ -5058,7 +5059,8 @@ qemuBuildMemoryBackendStr(unsigned long long size,
>      *backendType = NULL;
> 
>      /* memory devices could provide a invalid guest node */
> -    if (guestNode >= virDomainNumaGetNodeCount(def->numa)) {
> +    if (guestNode >= 0 &&
> +        guestNode >= virDomainNumaGetNodeCount(def->numa)) {
>          virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>                         _("can't add memory backend for guest node '%d' as "
>                           "the guest has only '%zu' NUMA nodes configured"),
> @@ -5069,7 +5071,9 @@ qemuBuildMemoryBackendStr(unsigned long long size,
>      if (!(props = virJSONValueNewObject()))
>          return -1;

^^ this could move
> 
> -    memAccess = virDomainNumaGetNodeMemoryAccessMode(def->numa, guestNode);
> +    if (guestNode >= 0)
> +        memAccess = virDomainNumaGetNodeMemoryAccessMode(def->numa, guestNode);
> +
>      if (virDomainNumatuneGetMode(def->numa, guestNode, &mode) < 0 &&
>          virDomainNumatuneGetMode(def->numa, -1, &mode) < 0)
>          mode = VIR_DOMAIN_NUMATUNE_MEM_STRICT;

^^
Seems to me the code could be adjusted a bit to have:

    if (guestNode >= 0) {
        if (guestNode >= virDomainNumaGetNodeCount(def->numa))
        ...
        memAccess = virDomainNumaGetNodeMemoryAccessMode(def->numa,
guestNode);
        ...
        if (virDomainNumatuneGetMode(def->numa, guestNode, &mode) < 0 &&
        ...
    }

where 'mode' is initialized to VIR_DOMAIN_NUMATUNE_MEM_STRICT;

and the props call moves either before or after the blob.


> @@ -5086,6 +5090,10 @@ qemuBuildMemoryBackendStr(unsigned long long size,
>                  continue;
>              }
> 
> +            /* just find the master hugepage in case we don't use NUMA */
> +            if (guestNode < 0)
> +                continue;
> +

So what you're say is if master_hugepage is set/found, then we don't
need to find anything else? Or can there be more than one
master_hugepage and it is desired to find the "last"?

Is there perhaps a cause/reason to break the loop rather than continue?
 IOW: Move the check inside the previous if and use it as a cause to
break the loop.

I think this is ACK-able with a couple of adjustments, but just wanted
to check what was more reasonable first - especially with this < 0 change.

John

>              if (virBitmapGetBit(hugepage->nodemask, guestNode,
>                                  &thisHugepage) < 0) {
>                  /* Ignore this error. It's not an error after all. Well,
>