Re: [PATCH v5 24/24] xl: vNUMA support

[Dario Faggioli <dario.faggioli@citrix.com>]

[-- Attachment #1.1: Type: text/plain, Size: 2631 bytes --]

On Thu, 2015-02-12 at 19:44 +0000, Wei Liu wrote:
> This patch includes configuration options parser and documentation.
> 
> Please find the hunk to xl.cfg.pod.5 for more information.
> 
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
>
This all looks pretty good to me. I only have one comment and a
question.


> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index ec7fb2d..f52daf9 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c

[...]

> +                if (!strcmp("pnode", option)) {
> +                    val = strtoul(value, &endptr, 10);
> +                    ABORT_IF_FAILED(value);
> +                    if (val >= nr_nodes) {
> +                        fprintf(stderr,
> +                                "xl: invalid pnode number: %lu\n", val);
> +                        exit(1);
> +                    }
> +                    p->pnode = val;
>
This is, to all the effects, a form of placement so, if this part of
vNUMA specification is present, you should disable the automatic
placement happening in libxl.

This is all it takes to do so (look inside parse_vcpu_affinity() if you
need more insights):

libxl_defbool_set(&b_info->numa_placement, false);

> +                } else if (!strcmp("size", option)) {
> +                    val = strtoul(value, &endptr, 10);
> +                    ABORT_IF_FAILED(value);
> +                    p->memkb = val << 10;
> +                } else if (!strcmp("vcpus", option)) {
> +                    libxl_string_list cpu_spec_list;
> +                    int cpu;
> +                    unsigned long s, e;
> +
> +                    split_string_into_string_list(value, ",", &cpu_spec_list);
> +                    len = libxl_string_list_length(&cpu_spec_list);
> +
> +                    for (j = 0; j < len; j++) {
> +                        parse_range(cpu_spec_list[j], &s, &e);
> +                        for (cpu = s; cpu <=e; cpu++)
> +                            libxl_bitmap_set(&p->vcpus, cpu);
> +                    }
> +                    libxl_string_list_dispose(&cpu_spec_list);
>
I think that using vcpupin_parse() for "vcpus=" would allow for more
flexible syntax (i.e., things like "3-8,^5"), and save some code. The
only downside is that it also accepts things like "nodes:1", which we
clearly don't want in here... is that why you are not going for it?

If you decide to use it, BTW, you may want to change its name (again!)

Regards,
Dario

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel