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

Re: [libvirt] [PATCH 1/5] numad: Set memory policy according to the advisory nodeset from numad



On 05/08/2012 10:04 AM, Osier Yang wrote:
> Though numad will manage the memory allocation of task dynamically,
> but it wants management application (libvirt) to pre-set the memory
> policy according to the advisory nodeset returned from querying numad,
> (just like pre-bind CPU nodeset for domain process), and thus the
> performance could benifit much more from it.

s/benifit/benefit/

> 
> This patch introduces new XML tag 'placement', value 'auto' indicates
> whether to set the memory policy with the advisory nodeset from numad,
> and its value defaults to the value of <vcpu> placement, or 'static'
> if 'nodeset' is specified. Example of the new XML tag's usage:
> 
>   <numatune>
>     <memory placement='auto' mode='interleave'/>
>   </numatune>
> 
> Just like what current "numatune" does, the 'auto' numa memory policy
> setting uses libnuma's API too.
> 
> If <vcpu> "placement" is "auto", and <numatune> is not specified
> explicitly, a default <numatume> will be added with "placement"
> set as "auto", and "mode" set as "strict".
> 
> The following XML can now fully drive numad:
> 
> 1) <vcpu> placement is 'auto', no <numatune> is specified.
> 
>    <vcpu placement='auto'>10</vcpu>
> 
> 2) <vcpu> placement is 'auto', no 'placement' is specified for
>    <numatune>.
> 
>    <vcpu placement='auto'>10</vcpu>
>    <numatune>
>      <memory mode='interleave'/>
>    </numatune>
> 
> And it's aslo able to control the CPU placement and memory policy

s/aslo/also/

> independantly. e.g.

s/independantly/independently/

> 
> 1) <vcpu> placement is 'auto', and <numatune> placement is 'static'
> 
>    <vcpu placement='auto'>10</vcpu>
>    <numatune>
>      <memory mode='strict' nodeset='0-10,^7'/>
>    </numatune>
> 
> 2) <vcpu> placement is 'static', and <numatune> placement is 'auto'
> 
>    <vcpu placement='static' cpuset='0-24,^12'>10</vcpu>
>    <numatune>
>      <memory mode='interleave' placement='auto'/>
>    </numatume>
> 
> A follow up patch will change the XML formating codes to always output

s/formating/formatting/

> 'placement' for <vcpu>, even it's 'static'.
> 
> v1 ~ v2:
>   * Changes on <numatune> parsing so that the <numatune> "placement"
>     could default to <vcpu> "placement".
>   * Add member 'default' for enum virDomainNumatuneMemPlacementMode.
>   * Output "placement" for <numatune> even it's static.
>   * Update docs/formatdomain.html.in
>   * Correct the changes on docs/schemas/domaincommon.rng
>   * New tests for the combinations of <vcpu> and <numatune>.
>   * Change on spec file is splitted off.

Patch versioning details can go after the ---, so that it isn't recorded
into the actual git commit log (it is useful for review, but not worth
storing permanently).

> ---
>  docs/formatdomain.html.in                          |   23 +++-
>  docs/schemas/domaincommon.rng                      |   36 ++++--
>  src/conf/domain_conf.c                             |  128 +++++++++++++++-----
>  src/conf/domain_conf.h                             |   10 ++
>  src/libvirt_private.syms                           |    2 +
>  src/qemu/qemu_process.c                            |   85 ++++++++------
>  .../qemuxml2argv-numad-auto-vcpu-no-numatune.xml   |   29 +++++
>  ...muxml2argv-numad-auto-vcpu-static-numatune.args |    4 +
>  ...emuxml2argv-numad-auto-vcpu-static-numatune.xml |   31 +++++
>  .../qemuxml2argv-numad-static-vcpu-no-numatune.xml |   29 +++++
>  tests/qemuxml2argvdata/qemuxml2argv-numad.args     |    4 +
>  tests/qemuxml2argvdata/qemuxml2argv-numad.xml      |   31 +++++
>  tests/qemuxml2argvtest.c                           |    2 +
>  .../qemuxml2xmlout-numad-auto-vcpu-no-numatune.xml |   32 +++++
>  ...emuxml2xmlout-numad-static-vcpu-no-numatune.xml |   29 +++++
>  tests/qemuxml2xmltest.c                            |    2 +
>  16 files changed, 394 insertions(+), 83 deletions(-)
>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-numad-auto-vcpu-no-numatune.xml
>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-numad-auto-vcpu-static-numatune.args
>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-numad-auto-vcpu-static-numatune.xml
>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-numad-static-vcpu-no-numatune.xml
>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-numad.args
>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-numad.xml
>  create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-numad-auto-vcpu-no-numatune.xml
>  create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-numad-static-vcpu-no-numatune.xml

Git (and for that matter, patch) did not like your patch; you had a hunk
that was misnumbered [*].  I had a bear of a time figuring out how to
get this to apply.

> +++ b/src/conf/domain_conf.c
...

> @@ -12491,25 +12545,33 @@ virDomainDefFormatInternal(virDomainDefPtr def,

[*] here's where the patch was corrupt - it claims 33 lines post-patch,
but you only provide 32 lines...

>          def->cputune.period || def->cputune.quota)
>          virBufferAddLit(buf, "  </cputune>\n");
>  
> -    if (def->numatune.memory.nodemask) {
> +    if (def->numatune.memory.nodemask ||
> +        def->numatune.memory.placement_mode) {
>          virBufferAddLit(buf, "  <numatune>\n");
>          const char *mode;
>          char *nodemask = NULL;
> -
> -        nodemask = virDomainCpuSetFormat(def->numatune.memory.nodemask,
> -                                         VIR_DOMAIN_CPUMASK_LEN);
> -        if (nodemask == NULL) {
> -            virDomainReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> -                                 _("failed to format nodeset for "
> -                                   "NUMA memory tuning"));
> -            goto cleanup;
> -        }
> +        const char *placement;
>  
>          mode = virDomainNumatuneMemModeTypeToString(def->numatune.memory.mode);
> -        virBufferAsprintf(buf, "    <memory mode='%s' nodeset='%s'/>\n",
> -                              mode, nodemask);
> -        VIR_FREE(nodemask);
> +        virBufferAsprintf(buf, "    <memory mode='%s' ", mode);
>  
> +        if (def->numatune.memory.placement_mode ==
> +            VIR_DOMAIN_NUMATUNE_MEM_PLACEMENT_MODE_STATIC) {
> +            nodemask = virDomainCpuSetFormat(def->numatune.memory.nodemask,
> +                                         VIR_DOMAIN_CPUMASK_LEN);
> +            if (nodemask == NULL) {
> +                virDomainReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                                     _("failed to format nodeset for "
> +                                       "NUMA memory tuning"));
> +                goto cleanup;
> +            }
> +            virBufferAsprintf(buf, "nodeset='%s'/>\n", nodemask);
> +            VIR_FREE(nodemask);
> +        } else if (def->numatune.memory.placement_mode) {
> +            placement = virDomainNumatuneMemPlacementModeTypeToString(def->numatune.memory.placement_mode);
> +            virBufferAsprintf(buf, "placement='%s'/>\n", placement);
> +        }
>          virBufferAddLit(buf, "  </numatune>\n");
>      }
>  
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h

which mean that the patch tools tried to treat this line as part of the
previous hunk instead of the start of a new file to patch.

But everything else looked okay.  ACK; I'll finish reviewing the rest of
the series, and then probably push.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature


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