Re: [Xen-devel] [PATCH v2 3/5] xl: allow for node-wise specification of vcpu pinning

[Dario Faggioli <dario.faggioli@xxxxxxxxxx>]

On ven, 2013-09-06 at 17:37 +0100, Ian Jackson wrote:
> Dario Faggioli writes ("[PATCH v2 3/5] xl: allow for node-wise specification 
> of vcpu pinning"):
> > 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;
> >  * ...
> ...
> >  * code rearranged in order to look more simple to follow
> >    and understand, as requested during review;
> 
> This is much better now.  Thank you!
> 
Glad to hear that. :-)

> > +static int parse_range(const char *str, unsigned long *a, unsigned long *b)
> >  {
> ...
> > +    if (endptr == str)
> > +        return EINVAL;
> > +    if (*a == ULONG_MAX)
> > +        return ERANGE;
> 
> So parse_range returns errno value or 0.  This isn't mentioned
> anywhere and is a bit unusual.
> 
> > +static int update_cpumap_range(const char *str, libxl_bitmap *cpumap)
> > +{
> ...
> > +    rc = libxl_node_bitmap_alloc(ctx, &node_cpumap, 0);
> > +    if (rc) {
> > +        fprintf(stderr, "libxl_node_bitmap_alloc failed.\n");
> > +        return rc;
> 
> So update_cpumap_range returns a libxl error code.
> 
> > +    rc = parse_range(str, &ida, &idb);
> > +    if (rc) {
> 
> But here you assign the errno value to an `rc' variable which holds a
> libxl error code.  I think it would be better to make parse_range
> return a libxl rc value.
> 
You're right, I cleaned up the code, but messed up quite a bit with the
error codes!

Thanks for pointing this out, I will put thing in a more consistent
state.

> I think also that parse_range doesn't notice if the specified range
> contains junk between the second number and the comma ?
> 
Mmm... I will double check, although I'm not sure I'm getting you 100%,
since parse_range does not deal with comas... That are handled by
"strtok_r(cpu, ",", &saveptr)", which now is in vcpupin_parse().

> > +static int vcpupin_parse(char *cpu, libxl_bitmap *cpumap)
> > +{
> ...
> > +        if (STR_HAS_PREFIX(ptr, "all") ||
> > +            STR_HAS_PREFIX(ptr, "nodes:all")) {
> > +            libxl_bitmap_set_any(cpumap);
> 
> Why not deal with all in parse_range ?  You'd avoid the second
> special-case of "nodes:", and constructions like
>    all,^3
> would work.
> 
Having it here already makes that ("all,^3") work, but it is true that
it'd simplify the if, and that it belongs more within parse_range() than
here. I'll move it.

> > -vcpp_out:
> > -    libxl_bitmap_dispose(&exclude_cpumap);
> > +        rc = update_cpumap_range(ptr, cpumap);
> > +        if (rc) {
> > +            /* If failing, reset the cpumap and exit */
> > +            libxl_bitmap_set_none(cpumap);
> 
> Surely the caller who gets a error should expect the cpumap to contain
> arbitrary contents ?
> 
Fair enough.

BTW, any chance you could have a look at 4/5 and 5/5 as well, so that I
can repost?

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