On Tue, Feb 24, 2015 at 04:19:02PM +0000, Dario Faggioli wrote:
> 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.l...@citrix.com>
> > Cc: Ian Campbell <ian.campb...@citrix.com>
> > Cc: Ian Jackson <ian.jack...@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);
>
Will fix this.
> > + } 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?
>
Yes. I don't want "nodes" so I didn't reuse that function, and at that
point I didn't think it's critical to support "^X".
If you think this "^X" syntax is important, I can check for "nodes"
before calling vcpupin_parse.
> If you decide to use it, BTW, you may want to change its name (again!)
>
vcpus_parse? It's not restricted to vcpu pinning in any way, I think.
Wei.
> Regards,
> Dario
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
