Re: [Xen-devel] [PATCH v5 24/24] xl: vNUMA support

[Wei Liu <wei.liu2@xxxxxxxxxx>]

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.liu2@xxxxxxxxxx>
> > Cc: Ian Campbell <ian.campbell@xxxxxxxxxx>
> > Cc: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
> >
> 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@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel