Re: [Xen-devel] [PATCH RESEND 02/12] xl: allow for node-wise specification of vcpu pinning

[Dario Faggioli <dario.faggioli@xxxxxxxxxx>]

On mar, 2013-11-05 at 14:50 +0000, George Dunlap wrote:
> On 11/05/2013 02:34 PM, Dario Faggioli wrote:
> > Making it possible to use something like the following:
> >   * "nodes:0-3": all pCPUs of nodes 0,1,2,3;
> >   * "nodes:0-3,^node:2": all pCPUS of nodes 0,1,3;
> >   * "1,nodes:1-2,^6": pCPU 1 plus all pCPUs of nodes 1,2
> >     but not pCPU 6;
> >   * ...
> >
> > In both domain config file and `xl vcpu-pin'.
> >
> > Signed-off-by: Dario Faggioli <dario.faggioli@xxxxxxxxxx>
> 
> Overall looks like a pretty clean patch; just a few comments.
> 
Ok.

> > diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
> >
> > -To allow all the vcpus of the guest to run on cpus 0,2,3,5.
> > +To allow all the vcpus of the guest to run on cpus 0,2,3,5. Combining
> > +this with "all" is possible, meaning "all,^7" results in all the vcpus
> > +of the guest running on all the cpus on the host except cpu 7.
> > +
> > +=item "nodes:0-3,node:^2"
> 
> Here you use both "nodes" and "node", while the code seems to only check 
> for "nodes".  
>
Right... This was a leftover from a previous version where I was
actually checking for both, as well as trying to enforce singular/plural
consistency (which, I agree with you) is overkill.

> I was originally going to say we should just check one; 
> but on the other hand, it's just an extra string compare -- I feel like 
> we might as well accept either "node" or "nodes".  (No need to enforce 
> plurality: "nodes:2" and "node:1-3" should both be fine with me.)
> 
Right, I'll add the support for both then.

> > +static int update_cpumap_range(const char *str, libxl_bitmap *cpumap)
> >   {
> > -    libxl_bitmap exclude_cpumap;
> > -    uint32_t cpuida, cpuidb;
> > -    char *endptr, *toka, *tokb, *saveptr = NULL;
> > -    int i, rc = 0, rmcpu;
> > +    unsigned long ida, idb;
> > +    libxl_bitmap node_cpumap;
> > +    bool is_not = false, is_nodes = false;
> > +    int rc = 0;
> > +
> > +    libxl_bitmap_init(&node_cpumap);
> >
> > -    if (!strcmp(cpu, "all")) {
> > +    rc = libxl_node_bitmap_alloc(ctx, &node_cpumap, 0);
> > +    if (rc) {
> > +        fprintf(stderr, "libxl_node_bitmap_alloc failed.\n");
> > +        goto out;
> > +    }
> > +
> > +    /* Are we adding or removing cpus/nodes? */
> > +    if (STR_SKIP_PREFIX(str, "^")) {
> > +        is_not = true;
> > +    }
> > +
> > +    /* Are we dealing with cpus or full nodes? */
> > +    if (STR_SKIP_PREFIX(str, "nodes:")) {
> > +        is_nodes = true;
> > +    }
> > +
> > +    if (STR_HAS_PREFIX(str, "all")) {
> 
> Is there any reason not to keep this "strcmp"?  As it is, this will 
> accept any string that *starts* with "all", which isn't exactly what you 
> want, I don't think.
> 
Ah, true... I'll revert this to strcmp().

Thanks and Regards,
Dario

-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)

Attachment: signature.asc
Description: This is a digitally signed message part

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