Re: [PATCH RESEND 12/12] xl: numa-sched: enable specifying node-affinity in VM config file

[Dario Faggioli <dario.faggioli@citrix.com>]

[-- Attachment #1.1: Type: text/plain, Size: 4170 bytes --]

On gio, 2013-11-07 at 18:35 +0000, Ian Jackson wrote:
> Dario Faggioli writes ("[PATCH RESEND 12/12] xl: numa-sched: enable specifying node-affinity in VM config file"):
> > in a similar way to how it is possible to specify vcpu-affinity.
> ...
> >      /*
> > -     * Check if the domain has any CPU affinity. If not, try to build
> > -     * up one. In case numa_place_domain() find at least a suitable
> > -     * candidate, it will affect info->nodemap accordingly; if it
> > -     * does not, it just leaves it as it is. This means (unless
> > -     * some weird error manifests) the subsequent call to
> > -     * libxl_domain_set_nodeaffinity() will do the actual placement,
> > +     * Check if the domain has any pinning or node-affinity and, if not, try
> > +     * to build up one.
> > +     *
> > +     * In case numa_place_domain() find at least a suitable candidate, it will
> > +     * affect info->nodemap accordingly; if it does not, it just leaves it as
> > +     * it is. This means (unless some weird error manifests) the subsequent
> > +     * call to libxl_domain_set_nodeaffinity() will do the actual placement,
> >       * whatever that turns out to be.
> >       */
> >      if (libxl_defbool_val(info->numa_placement)) {
> >  
> > -        if (!libxl_bitmap_is_full(&info->cpumap)) {
> > +        if (!libxl_bitmap_is_full(&info->cpumap) ||
> > +            !libxl_bitmap_is_full(&info->nodemap)) {
> >              LOG(ERROR, "Can run NUMA placement only if no vcpu "
> > -                       "affinity is specified");
> > +                       "pinning or node-affinity is specified");
> 
> I appreciate I may be a bit late with this complaint, but surely this
> approach (setting the cpumap and nodemap when they aren't
> all-bits-set) means that it's not possible to explicitly set "all
> cpus".
> 
The funny part is not that you're late, but to whom you're saying
it! :-D :-D

http://lists.xen.org/archives/html/xen-devel/2012-07/msg01077.html

From: Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>
Date: Mon, 23 Jul 2012 12:04:29 +0100
Dario Faggioli writes ("[PATCH 2 of 4 v6/leftover] xl: inform libxl if there was a cpumap in the config file"):
[...]
Shouldn't this be

  -    if (libxl_bitmap_is_full(&info->cpumap)) {
  +    if (libxl_defbool_val(info->numa_placement)) {
  +        if (!libxl_bitmap_is_full(&info->cpumap)) {
  +            rc = ERROR_INVAL;
  +            LOG blah blah invalid not supported
  +            goto out;
  +        }

The story behind all this is that, back at that time, someone (and I'm
sure it's you again, although I don't find the thread now) asked to
introduce info->numa_placement, to have a mean for: (1) disabling NUMA
placement completely; (2) help distinguishing the case where cpumap is
full because we want "cpus=all" from the one where it's full because we
did not touched it, since being full is the default value.

I remember finding it sound, and thus going for it. Perhaps we could
have done it differently, but given that info->cpumap is full by
default, I really don't see that many alternatives (apart from making
cpumap empty by default, which may look even weirder).

So, the idea here is:
 - if it's full, and placement is enabled, full means "do this for me",
   and that's what happens.
 - if placement is disabled, then nobody touches it, so if it was full
   it stay that way.

So, for saying "all", the procedure is as follows:

info->numa_placement = false;
fill_bitmap(info->cpumap);

> If I say in the config file "all" for cpus and "all" for nodes, this
> code will override that.  I don't think that can be right.
> 
And in fact, xl, when parsing the config file, turns numa_placement to
false when finding a "cpus=" item.

All the above applies to the new "nodes=" switch as well, of course.

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 #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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