From mboxrd@z Thu Jan 1 00:00:00 1970 From: Wei Liu Subject: Re: [PATCH v5 24/24] xl: vNUMA support Date: Tue, 24 Feb 2015 16:31:19 +0000 Message-ID: <20150224163119.GK20083@zion.uk.xensource.com> References: <1423770294-9779-1-git-send-email-wei.liu2@citrix.com> <1423770294-9779-25-git-send-email-wei.liu2@citrix.com> <1424794739.4742.118.camel@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Content-Disposition: inline In-Reply-To: <1424794739.4742.118.camel@citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Dario Faggioli Cc: Wei Liu , "JBeulich@suse.com" , Andrew Cooper , "xen-devel@lists.xen.org" , "ufimtseva@gmail.com" , Ian Jackson , Ian Campbell List-Id: xen-devel@lists.xenproject.org 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 > > Cc: Ian Campbell > > Cc: Ian Jackson > > > 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