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

[Dario Faggioli <dario.faggioli@xxxxxxxxxx>]

On ven, 2013-07-12 at 17:51 +0100, Ian Jackson wrote:
> Dario Faggioli writes ("[PATCH] 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;
> >  * ...
> 
> I have some comments about this.
> 
Ok, thanks for reviewing. Also, sorry for being so late in replying, but
I was travelling and, after that, busy enough that I could not come back
to this before now. :-/

> I think the idea is fine, and the syntax is not bad.
> 
Ok, that's already something. :-)

> The documentation needs to explain the semantics more formally.
> 
I'll try to improve it. What was it that you think was not clear enough?

> The parsing code is starting to be rather impenetrable.  Have you
> considered parsing this with flex or maybe doing this with regexps or
> something ?
> 
HeHe, actually I haven't, for the very simple reason that I know nothing
of flex. :-) That means, for me, turning it to flex include the overhead
of learning what flex is and how to use it, which does not look
worthwhile for "just" this change.

Also, in this same patch I'm breaking down the parsing function,
improving its readability (or so I think), and I'd rather try to push a
bit more on this direction, rather than introducing flex.

Honestly, I also think that this isn't that bad, but of course my
opinion is not at all authoritative, since I really have not idea how it
would look like with flex.

So, I think I'm sending this back to you, as a maintainer. :-) My view
is that it is fine as it is, but I will conform with what _you_ think
it's best.

> If you do decide to keep it the way it is:
> 
> > +    ida = idb = strtoul(str, &endptr, 10);
> > +    if (endptr == str)
> > +        return EINVAL;
> 
> You need to check for overflow, too, everywhere you call strtoul.  It
> may be easiest to wrap strtoul.
> 
I see what you mean. This is pretty much code motion, from the point of
view of this patch, but that does not make it less worthwhile.

What I'm thinking about is that there are other uses of strtol(), most
of them assigning the returned result to an int, so I'm not sure how to
wrap it... What the wrapper should return?

> What if "long" is bigger than uint32_t ?  I think this can silently
> overflow.  (Although perhaps people who write crazy strings of hex
> asking for vcpu 2^40 or something deserve to silently lose.)
>
What about I change ida and idb to long? I checked and it looks like the
worst that can happen, if we really get some crazy big number, is that
libxl_bitmap_set() turns into a nop, and no range at all is set (which,
as you say, is probably what those crazy people deserves).

Thoughts?

> > -    if (!strcmp(cpu, "all")) {
> > +    if (!strcmp(cpu, "all") || !strcmp(cpu, "nodes:all")) {
> >          libxl_bitmap_set_any(cpumap);
> >          return 0;
> 
> With your new "not" syntax, this needs to become not a special case.
> Someone might write
>    all,^7
> and it should work.
> 
Oh, I like that. will do.

> I think your current code changes the meaning of
>    ^7
> from "not pcpu 7" to "empty set of pcpus", which is wrong.
> 
Mmm... you mean if one only specifies "^7"? That does not work even
right now (because it already translates to an "empty set of pcups", I
just tried). Or was it something else that you were saying?

If you're saying that specifying just "^7" should mean "all,^7", I think
I agree, and I can do that.

> > +        /* Are we dealing with cpus or nodes? */
> > +        if (!strncmp(ptr, "node:", 5) || !strncmp(ptr, "nodes:", 6)) {
> > +            isnode = true;
> > +            ptr += 5 + (ptr[4] == 's');
> 
> Cripes.  Please, no.  None of these magic 4s and 5s and 6s, and the
> ptr[4]=='s' thing is truly awful.
> 
> How about some macro STR_SKIP_PREFIX(), and two separate ifs,
> 
Another nice idea. I'll go for it.

> I think this is starting to be complex enough that it ought to have
> some in-tree test vectors.
> 
Oh, you mean like the one we have in check-xl-disk-parse, right? Ok, it
makes sense, I will look into this too.

Thanks again 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