[PATCH 0/2] libxl: fix NUMA placement preventing domain config to be re-used

[PATCH 0/2] libxl: fix NUMA placement preventing domain config to be re-used

[Dario Faggioli]

Hi,

This series fixes the issue reported here by Ian, about NUMA placement related
domain config fields preventing the config to be reused:
http://lists.xenproject.org/archives/html/xen-devel/2015-06/msg04454.html

The way it does it is by setting the b_info->numa_placement config switch to
false, after NUMA placement has actually happened. This way, the next time we
re-use the config, we won't run the placement again, we just will use the
result obtained from the original execution of it. This is, both IMO and for
Ian Jackson (as per an IRC conversation we had yesterday), the best behaviour,
as it yields consistent results across config re-uses.

BTW, with patch 2 applied, I've been able to run the test in here, and it
worked for me:
http://xenbits.xen.org/gitweb/?p=people/iwj/ring3-xl-test.git;a=shortlog;h=refs/heads/t.numa-bug

While I was there, though, I took the chance to put patch 1 together as well.
What it does it turning a couple of error conditions (still related to the NUMA
placement domain config switch) into warnings, as error was too much. This has
been discussed a while back, and I distinctly remember it mentioned in a
conversation on xen-devel, but I can't find it now. In any case, I think it's
the right thing to do.

Regards,
Dario
---
Dario Faggioli (2):
      libxl: turn NUMA placement misconfigs into warnings
      libxl: unset info->numa_placement upon successful placement


 tools/libxl/libxl_dom.c |   85 +++++++++++++++++++++++++----------------------
 1 file changed, 46 insertions(+), 39 deletions(-)
--
<<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)

[PATCH 1/2] libxl: turn NUMA placement misconfigs into warnings

[Dario Faggioli]

instead than errors. More specifically, in libxl,
b_info->numa_autoplacement is meant as a way to
disable automatic NUMA placement, if one does not
want it to happen. It is, however, useful for
consistency checking as well, i.e., to ensure that
the user provided configuration (such as, for instance,
vcpu hard or soft affinity) and NUMA placement itself
will not clash.

However, right now, if such a clash happens we abort
domain creation and error out, which is too much! It
is, in fact, enough to infom the user/caller that NUMA
placement won't be performed, with a WARN, and that's
what this commit does.

Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
---
 tools/libxl/libxl_dom.c |   76 +++++++++++++++++++++++------------------------
 1 file changed, 37 insertions(+), 39 deletions(-)

diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index 1c9418a..b9a77a5 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -330,50 +330,48 @@ int libxl__build_pre(libxl__gc *gc, uint32_t domid,
      * reflect the placement result if that is the case
      */
     if (libxl_defbool_val(info->numa_placement)) {
-        libxl_bitmap cpumap_soft;
-
-        if (info->cpumap.size ||
-            info->num_vcpu_hard_affinity || info->num_vcpu_soft_affinity) {
-            LOG(ERROR, "Can run NUMA placement only if no vcpu "
-                       "(hard or soft) affinity is specified explicitly");
-            return ERROR_INVAL;
-        }
-        if (info->nodemap.size) {
-            LOG(ERROR, "Can run NUMA placement only if the domain does not "
-                       "have any NUMA node affinity set already");
-            return ERROR_INVAL;
-        }
-
-        rc = libxl_node_bitmap_alloc(ctx, &info->nodemap, 0);
-        if (rc)
-            return rc;
-        libxl_bitmap_set_any(&info->nodemap);
+        if (info->cpumap.size || info->num_vcpu_hard_affinity ||
+            info->num_vcpu_soft_affinity)
+            LOG(WARN, "Can't run NUMA placement, as an (hard or soft) "
+                      "affinity has been specified explicitly");
+        else if (info->nodemap.size)
+            LOG(WARN, "Can't run NUMA placement, as the domain has "
+                      "NUMA node affinity set already");
+        else {
+            libxl_bitmap cpumap_soft;
+
+            rc = libxl_node_bitmap_alloc(ctx, &info->nodemap, 0);
+            if (rc)
+                return rc;
+            libxl_bitmap_set_any(&info->nodemap);
+
+            rc = libxl_cpu_bitmap_alloc(ctx, &cpumap_soft, 0);
+            if (rc)
+                return rc;
+
+            rc = numa_place_domain(gc, domid, info);
+            if (rc) {
+                libxl_bitmap_dispose(&cpumap_soft);
+                return rc;
+            }
 
-        rc = libxl_cpu_bitmap_alloc(ctx, &cpumap_soft, 0);
-        if (rc)
-            return rc;
+            /*
+             * All we need to do now is converting the result of automatic
+             * placement from nodemap to cpumap, and then use such cpumap
+             * as the soft affinity for all the vcpus of the domain.
+             *
+             * When calling libxl_set_vcpuaffinity_all(), it is ok to use
+             * NULL as hard affinity, as we know we don't have one, or we
+             * won't be here.
+             */
+            libxl_nodemap_to_cpumap(ctx, &info->nodemap, &cpumap_soft);
+            libxl_set_vcpuaffinity_all(ctx, domid, info->max_vcpus,
+                                       NULL, &cpumap_soft);
 
-        rc = numa_place_domain(gc, domid, info);
-        if (rc) {
             libxl_bitmap_dispose(&cpumap_soft);
-            return rc;
         }
-
-        /*
-         * All we need to do now is converting the result of automatic
-         * placement from nodemap to cpumap, and then use such cpumap as
-         * the soft affinity for all the vcpus of the domain.
-         *
-         * When calling libxl_set_vcpuaffinity_all(), it is ok to use NULL
-         * as hard affinity, as we know we don't have one, or we won't be
-         * here.
-         */
-        libxl_nodemap_to_cpumap(ctx, &info->nodemap, &cpumap_soft);
-        libxl_set_vcpuaffinity_all(ctx, domid, info->max_vcpus,
-                                   NULL, &cpumap_soft);
-
-        libxl_bitmap_dispose(&cpumap_soft);
     }
+
     if (info->nodemap.size)
         libxl_domain_set_nodeaffinity(ctx, domid, &info->nodemap);

Re: [PATCH 1/2] libxl: turn NUMA placement misconfigs into warnings

[Ian Jackson]

Dario Faggioli writes ("[PATCH 1/2] libxl: turn NUMA placement misconfigs into warnings"):
> instead than errors. More specifically, in libxl,
> b_info->numa_autoplacement is meant as a way to
> disable automatic NUMA placement, if one does not
> want it to happen. It is, however, useful for
> consistency checking as well, i.e., to ensure that
> the user provided configuration (such as, for instance,
> vcpu hard or soft affinity) and NUMA placement itself
> will not clash.

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

But, be aware that you are making a semantic change to the API which
cannot be reverted for backwards-compatibility reasons.

Ian.

Re: [PATCH 1/2] libxl: turn NUMA placement misconfigs into warnings

[Ian Campbell]

On Wed, 2015-07-01 at 15:38 +0100, Ian Jackson wrote:
> Dario Faggioli writes ("[PATCH 1/2] libxl: turn NUMA placement misconfigs into warnings"):
> > instead than errors. More specifically, in libxl,
> > b_info->numa_autoplacement is meant as a way to
> > disable automatic NUMA placement, if one does not
> > want it to happen. It is, however, useful for
> > consistency checking as well, i.e., to ensure that
> > the user provided configuration (such as, for instance,
> > vcpu hard or soft affinity) and NUMA placement itself
> > will not clash.
> 
> Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

Applied both patches with your ack.

> But, be aware that you are making a semantic change to the API which
> cannot be reverted for backwards-compatibility reasons.

Is any further action required due to this?

Re: [PATCH 1/2] libxl: turn NUMA placement misconfigs into warnings

[Ian Jackson]

Ian Campbell writes ("Re: [PATCH 1/2] libxl: turn NUMA placement misconfigs into warnings"):
> On Wed, 2015-07-01 at 15:38 +0100, Ian Jackson wrote:
> > Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
> 
> Applied both patches with your ack.

Right.

> > But, be aware that you are making a semantic change to the API which
> > cannot be reverted for backwards-compatibility reasons.
> 
> Is any further action required due to this?

Not if it doesn't cause you or Dario to change your mind about the
patch :-).

Thanks,
Ian.

Re: [PATCH 1/2] libxl: turn NUMA placement misconfigs into warnings

[Dario Faggioli]

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

On Fri, 2015-07-03 at 12:09 +0100, Ian Jackson wrote:
> Ian Campbell writes ("Re: [PATCH 1/2] libxl: turn NUMA placement misconfigs into warnings"):
> > On Wed, 2015-07-01 at 15:38 +0100, Ian Jackson wrote:

> > > But, be aware that you are making a semantic change to the API which
> > > cannot be reverted for backwards-compatibility reasons.
> > 
> > Is any further action required due to this?
> 
> Not if it doesn't cause you or Dario to change your mind about the
> patch :-).
> 
It doesn't. This is the right thing to do, and should have been like
this from the beginning!

Just FTR, the reason why we were using the numa_autoplacement for sanity
checking was that, a while back, we were always passing a (potentially)
valid cpumap (for vcpu affinity) as part of the domain config. Hence we
needed something to distinguish the case were such a cpumap was full (or
empty, I can't recall) because of an explicit user choice, or as a mean
to ask libxl to perform the automatic placement.

I can't remember either why we did not do it, in the first place, the
way it is now (i.e., cpumaps *not allocated* --> do the placement), but
I never liked the fact that, if one[*] sets the affinity explicitly, but
forget to set numa_autoplacement to false, the domain does not build.

So, in summary, no, I don't think I will easily change my mind about
this needing to be a warning, rather than a fatal error.

Thanks and Regards,
Dario

[*] of course I'm talking about downstream toolstacks, like xl or
libvirt, not actual users, as in xl we do this consistently!
-- 
<<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: 181 bytes --]

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

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

[PATCH 2/2] libxl: unset info->numa_placement upon successful placement

[Dario Faggioli]

so that, if the same config is reused later, the following
two (good) things happen:
 - we do not trip over warnings because node and/or vcpu
   soft affinity now exist (as a consequence of the
   successful placement), but numa_placement is still
   true;
 - we end up always using the results of the original
   execution of the placement algorithm, rather than
   re-running it at each re-use of the same config,
   which is what most users expects and wants.

This fixes the bug reported here:
http://lists.xenproject.org/archives/html/xen-devel/2015-06/msg04454.html

Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
Cc: Euan Harris <euan.harris@citrix.com>
---
 tools/libxl/libxl_dom.c |    9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index b9a77a5..8642192 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -369,6 +369,15 @@ int libxl__build_pre(libxl__gc *gc, uint32_t domid,
                                        NULL, &cpumap_soft);
 
             libxl_bitmap_dispose(&cpumap_soft);
+
+            /*
+             * Placement has run, so avoid for it to be re-run, if this
+             * same config we are using and building here is ever re-used.
+             * This means that people re-using configs will get the same
+             * results, consistently, across every re-use, which is what
+             * we expect most people to want.
+             */
+            libxl_defbool_set(&info->numa_placement, false);
         }
     }

Re: [PATCH 2/2] libxl: unset info->numa_placement upon successful placement

[Ian Jackson]

Dario Faggioli writes ("[PATCH 2/2] libxl: unset info->numa_placement upon successful placement"):
> so that, if the same config is reused later, the following
> two (good) things happen:
>  - we do not trip over warnings because node and/or vcpu
>    soft affinity now exist (as a consequence of the
>    successful placement), but numa_placement is still
>    true;
>  - we end up always using the results of the original
>    execution of the placement algorithm, rather than
>    re-running it at each re-use of the same config,
>    which is what most users expects and wants.

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

Ian.