Re: [PATCH v2 16/16] libxl: automatic NUMA placement affects soft affinity

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

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

On gio, 2013-11-14 at 15:17 +0000, Ian Jackson wrote:
> Dario Faggioli writes ("[PATCH v2 16/16] libxl: automatic NUMA placement affects soft affinity"):

> > diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
> > index a1c16b0..d241efc 100644
> > --- a/tools/libxl/libxl_dom.c
> > +++ b/tools/libxl/libxl_dom.c
> > @@ -222,21 +222,39 @@ int libxl__build_pre(libxl__gc *gc, uint32_t domid,
> >       * some weird error manifests) the subsequent call to
> >       * libxl_domain_set_nodeaffinity() will do the actual placement,
> >       * whatever that turns out to be.
> > +     *
> > +     * As far as scheduling is concerned, we achieve NUMA-aware scheduling
> > +     * by having the results of placement affect the soft affinity of all
> > +     * the vcpus of the domain. Of course, we want that iff placement is
> > +     * enabled and actually happens, so we only change info->cpumap_soft to
> > +     * reflect the placement result if that is the case
> >       */
> >      if (libxl_defbool_val(info->numa_placement)) {
> 
> But isn't the default for this true ?
> 
It is.

> > -        if (!libxl_bitmap_is_full(&info->cpumap)) {
> > +        /* We require both hard and soft affinity not to be set */
> > +        if (!libxl_bitmap_is_full(&info->cpumap) ||
> > +            !libxl_bitmap_is_full(&info->cpumap_soft)) {
> >              LOG(ERROR, "Can run NUMA placement only if no vcpu "
> > -                       "affinity is specified");
> > +                       "(hard or soft) affinity is specified");
> >              return ERROR_INVAL;
> 
> So the result will be that any attempt to set cpumaps in an
> otherwise-naive config file will cause errors, rather than just
> disabling the numa placement ?
> 
Nope, because, as it discussed already not more than a couple of days
back, what xl does when finding a "cpus=" option (and from this patch
on, a "cpus_soft=" option) is:
 1. set numa_placement to false
 2. set cpumap (or cpumap_soft) as requested.

:-)

So, as far as xl is concerned, this is fine. It is true that every other
consumer of libxl needs to do the same (i.e., setting numa_placement to
false), or it will hit the error, and that may or may not be obvious.
For sure, they'll figure out if the check out xl (which is meant to
serve as a 'reference implementation' too, right?), and, it is all
documented, at least in docs/misc (I can double check whether it is also
stressed enough in libxl.h somewhere, and put it there if not). For one,
I probably should improve the comment above the if(), to avoid this sort
of confusion to happen again.

Anyway, like it or not, this is the kind of interface we came up with
when designing, refining, and checking in that bit:

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

If we don't like it, I think there is some room for amending, since,
despite libxl API being stable, this looks enough as an implementation
detail to me.... It's just a matter of agreeing on what we actually
want. :-)

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