Re: [Xen-devel] [PATCH] tools: libxl/xl: run NUMA placement even when an hard-affinity is set
Dario Faggioli writes ("Re: [Xen-devel] [PATCH] tools: libxl/xl: run NUMA
placement even when an hard-affinity is set"):
> Do I leave it under this special "manually enable for development and
> testing" kind of thing?
Yes, I think #ifdef DEBUG or whatever is fine. libxl_event.c has that
style already.
> Do I make it always on?
Not if you think it's too slow.
Ian.
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] tools: libxl/xl: run NUMA placement even when an hard-affinity is set
On Tue, 2018-08-21 at 11:25 +0100, Ian Jackson wrote:
> Dario Faggioli writes ("Re: [Xen-devel] [PATCH] tools: libxl/xl: run
> NUMA placement even when an hard-affinity is set"):
> > On Mon, 2018-08-20 at 11:14 +0100, Wei Liu wrote:
> > > You can use NDEBUG instead.
> >
> > But I don't have a too strong opinion; I'll change it to us NDEBUG
> > if
> > that's what you prefer.
>
> Please do not use NDEBUG. libxl is not designed to be compiled with
> NDEBUG.
>
Err.. Ok. So, what do I do?
Do I leave it under this special "manually enable for development and
testing" kind of thing?
Do I make it always on?
Do I get rid of it?
:-)
Dario
--
<> (Raistlin Majere)
-
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Software Engineer @ SUSE https://www.suse.com/
signature.asc
Description: This is a digitally signed message part
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] tools: libxl/xl: run NUMA placement even when an hard-affinity is set
Dario Faggioli writes ("Re: [Xen-devel] [PATCH] tools: libxl/xl: run NUMA
placement even when an hard-affinity is set"):
> On Mon, 2018-08-20 at 11:14 +0100, Wei Liu wrote:
> > You can use NDEBUG instead.
>
> I found this in one other place in libxl (libxl_event.c), and figured I
> could use it here too.
>
> Reason is that this guards a (potentially) rather expensive check,
> which I personally consider too much, even for debug build. At the same
> time, it could be handy for us developers to have it there, and be able
> to enable it when making changes to affinity and/or NUMA placement
> code.
>
> But I don't have a too strong opinion; I'll change it to us NDEBUG if
> that's what you prefer.
Please do not use NDEBUG. libxl is not designed to be compiled with
NDEBUG.
Ian.
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] tools: libxl/xl: run NUMA placement even when an hard-affinity is set
On Mon, 2018-08-20 at 11:14 +0100, Wei Liu wrote:
> On Fri, Aug 17, 2018 at 07:03:03PM +0200, Dario Faggioli wrote:
> > @@ -27,6 +27,8 @@
> >
> > #include "_paths.h"
> >
> > +//#define DEBUG 1
> > +
>
> Stray changes here?
>
> You can use NDEBUG instead.
>
I found this in one other place in libxl (libxl_event.c), and figured I
could use it here too.
Reason is that this guards a (potentially) rather expensive check,
which I personally consider too much, even for debug build. At the same
time, it could be handy for us developers to have it there, and be able
to enable it when making changes to affinity and/or NUMA placement
code.
But I don't have a too strong opinion; I'll change it to us NDEBUG if
that's what you prefer.
> > @@ -162,6 +165,38 @@ static int numa_place_domain(libxl__gc *gc,
> > uint32_t domid,
> > rc = libxl_cpupool_info(CTX, &cpupool_info, cpupool);
> > if (rc)
> > goto out;
> > +map = &cpupool_info.cpumap;
> > +
> > +/*
> > + * If there's a well defined hard affinity mask (i.e., the
> > same one for all
> > + * the vcpus), we can try to run the placement considering
> > only the pcpus
> > + * within such mask.
> > + */
> > +if (info->num_vcpu_hard_affinity)
> > +{
>
> Placement of "{" is wrong.
>
Yep, sorry.
> > +int j;
> > +
> > +for (j = 0; j < info->num_vcpu_hard_affinity; j++)
> > +assert(libxl_bitmap_equal(&info-
> > >vcpu_hard_affinity[0],
> > + &info-
> > >vcpu_hard_affinity[j], 0));
> > +#endif /* DEBUG */
>
> But why should the above be debug only? The assumption doesn't seem
> to
> always hold.
>
I'm not sure I'm getting your point. If we're here, we do indeed want
to be sure that we don't have a different hard-affinity mask for the
various vcpus... In fact, if we do, which one should we use for
placement?
So, yes, if we are here, I think the assert() should be true. I just
considered this check only useful to developers, and too expensive even
for debug builds. But as said above, I can gate this with NDEBUG if you
prefer so.
> > +/*
> > + * Hard affinity should _really_ contain cpus that are
> > inside our
> > + * cpupool. Anyway, if it does not, log a warning and only
> > use the
> > + * cpupool's cpus for placement.
> > + */
> > +if (!libxl_bitmap_is_empty(&cpumap))
> > +map = &cpumap;
> > +else
> > +LOG(WARN, "Hard affinity completely outside of
> > domain's cpupool?");
>
> Should this be an error?
>
> What is the expected interaction for hard affinity and cpupool?
>
cpupools rulez. :-D
Basically, a domain can't possibly run on any cpu which is outside of
the pool where the domain itself lives, no matter what the affinity is.
BTW, I tested this better, and I believe there is no need for adding
this WARN. In fact, if the domain is being created inside a pool,
setting its hard affinity to a mask which is completely disjoint from
the cpupool's one fails _before_ getting here.
Regards,
Dario
--
<> (Raistlin Majere)
-
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Software Engineer @ SUSE https://www.suse.com/
signature.asc
Description: This is a digitally signed message part
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] tools: libxl/xl: run NUMA placement even when an hard-affinity is set
On Fri, Aug 17, 2018 at 07:03:03PM +0200, Dario Faggioli wrote:
> Right now, if either an hard or soft-affinity are explicitly specified
> in a domain's config file, automatic NUMA placement is skipped. However,
> automatic NUMA placement affects only the soft-affinity of the domain
> which is being created.
>
> Therefore, it is ok to let it run if an hard-affinity is specified. The
> semantics will be that the best placement candidate would be found,
> respecting the specified hard-affinity, i.e., using only the nodes that
> contain the pcpus in the hard-affinity mask.
The reasoning sound plausible. I have some questions below.
>
> This is particularly helpful if global xl pinning masks are defined, as
> made possible by commit aa67b97ed34279c43 ("xl.conf: Add global affinity
> masks"). In fact, without this commit, defining a global affinity mask
> would also mean disabling automatic placement, but that does not
> necessarily have to be the case (especially in large systems).
>
> Signed-off-by: Dario Faggioli
> ---
> Cc: Ian Jackson
> Cc: Wei Liu
> Cc: George Dunlap
> ---
> tools/libxl/libxl_dom.c | 46 --
> tools/xl/xl_parse.c |6 --
> 2 files changed, 44 insertions(+), 8 deletions(-)
>
> diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
> index eb401cf1d6..e30e2dca9a 100644
> --- a/tools/libxl/libxl_dom.c
> +++ b/tools/libxl/libxl_dom.c
> @@ -27,6 +27,8 @@
>
> #include "_paths.h"
>
> +//#define DEBUG 1
> +
Stray changes here?
You can use NDEBUG instead.
> libxl_domain_type libxl__domain_type(libxl__gc *gc, uint32_t domid)
> {
> libxl_ctx *ctx = libxl__gc_owner(gc);
> @@ -142,12 +144,13 @@ static int numa_place_domain(libxl__gc *gc, uint32_t
> domid,
> {
> int found;
> libxl__numa_candidate candidate;
> -libxl_bitmap cpupool_nodemap;
> +libxl_bitmap cpumap, cpupool_nodemap, *map;
> libxl_cpupoolinfo cpupool_info;
> int i, cpupool, rc = 0;
> uint64_t memkb;
>
> libxl__numa_candidate_init(&candidate);
> +libxl_bitmap_init(&cpumap);
> libxl_bitmap_init(&cpupool_nodemap);
> libxl_cpupoolinfo_init(&cpupool_info);
>
> @@ -162,6 +165,38 @@ static int numa_place_domain(libxl__gc *gc, uint32_t
> domid,
> rc = libxl_cpupool_info(CTX, &cpupool_info, cpupool);
> if (rc)
> goto out;
> +map = &cpupool_info.cpumap;
> +
> +/*
> + * If there's a well defined hard affinity mask (i.e., the same one for
> all
> + * the vcpus), we can try to run the placement considering only the pcpus
> + * within such mask.
> + */
> +if (info->num_vcpu_hard_affinity)
> +{
Placement of "{" is wrong.
> +#ifdef DEBUG
#ifndef NDEBUG ?
> +int j;
> +
> +for (j = 0; j < info->num_vcpu_hard_affinity; j++)
> +assert(libxl_bitmap_equal(&info->vcpu_hard_affinity[0],
> + &info->vcpu_hard_affinity[j], 0));
> +#endif /* DEBUG */
But why should the above be debug only? The assumption doesn't seem to
always hold.
> +
> +rc = libxl_bitmap_and(CTX, &cpumap, &info->vcpu_hard_affinity[0],
> + &cpupool_info.cpumap);
> +if (rc)
> +goto out;
> +
> +/*
> + * Hard affinity should _really_ contain cpus that are inside our
> + * cpupool. Anyway, if it does not, log a warning and only use the
> + * cpupool's cpus for placement.
> + */
> +if (!libxl_bitmap_is_empty(&cpumap))
> +map = &cpumap;
> +else
> +LOG(WARN, "Hard affinity completely outside of domain's
> cpupool?");
Should this be an error?
What is the expected interaction for hard affinity and cpupool?
Wei.
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH] tools: libxl/xl: run NUMA placement even when an hard-affinity is set
Right now, if either an hard or soft-affinity are explicitly specified
in a domain's config file, automatic NUMA placement is skipped. However,
automatic NUMA placement affects only the soft-affinity of the domain
which is being created.
Therefore, it is ok to let it run if an hard-affinity is specified. The
semantics will be that the best placement candidate would be found,
respecting the specified hard-affinity, i.e., using only the nodes that
contain the pcpus in the hard-affinity mask.
This is particularly helpful if global xl pinning masks are defined, as
made possible by commit aa67b97ed34279c43 ("xl.conf: Add global affinity
masks"). In fact, without this commit, defining a global affinity mask
would also mean disabling automatic placement, but that does not
necessarily have to be the case (especially in large systems).
Signed-off-by: Dario Faggioli
---
Cc: Ian Jackson
Cc: Wei Liu
Cc: George Dunlap
---
tools/libxl/libxl_dom.c | 46 --
tools/xl/xl_parse.c |6 --
2 files changed, 44 insertions(+), 8 deletions(-)
diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index eb401cf1d6..e30e2dca9a 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -27,6 +27,8 @@
#include "_paths.h"
+//#define DEBUG 1
+
libxl_domain_type libxl__domain_type(libxl__gc *gc, uint32_t domid)
{
libxl_ctx *ctx = libxl__gc_owner(gc);
@@ -142,12 +144,13 @@ static int numa_place_domain(libxl__gc *gc, uint32_t
domid,
{
int found;
libxl__numa_candidate candidate;
-libxl_bitmap cpupool_nodemap;
+libxl_bitmap cpumap, cpupool_nodemap, *map;
libxl_cpupoolinfo cpupool_info;
int i, cpupool, rc = 0;
uint64_t memkb;
libxl__numa_candidate_init(&candidate);
+libxl_bitmap_init(&cpumap);
libxl_bitmap_init(&cpupool_nodemap);
libxl_cpupoolinfo_init(&cpupool_info);
@@ -162,6 +165,38 @@ static int numa_place_domain(libxl__gc *gc, uint32_t domid,
rc = libxl_cpupool_info(CTX, &cpupool_info, cpupool);
if (rc)
goto out;
+map = &cpupool_info.cpumap;
+
+/*
+ * If there's a well defined hard affinity mask (i.e., the same one for all
+ * the vcpus), we can try to run the placement considering only the pcpus
+ * within such mask.
+ */
+if (info->num_vcpu_hard_affinity)
+{
+#ifdef DEBUG
+int j;
+
+for (j = 0; j < info->num_vcpu_hard_affinity; j++)
+assert(libxl_bitmap_equal(&info->vcpu_hard_affinity[0],
+ &info->vcpu_hard_affinity[j], 0));
+#endif /* DEBUG */
+
+rc = libxl_bitmap_and(CTX, &cpumap, &info->vcpu_hard_affinity[0],
+ &cpupool_info.cpumap);
+if (rc)
+goto out;
+
+/*
+ * Hard affinity should _really_ contain cpus that are inside our
+ * cpupool. Anyway, if it does not, log a warning and only use the
+ * cpupool's cpus for placement.
+ */
+if (!libxl_bitmap_is_empty(&cpumap))
+map = &cpumap;
+else
+LOG(WARN, "Hard affinity completely outside of domain's cpupool?");
+}
rc = libxl_domain_need_memory(CTX, info, &memkb);
if (rc)
@@ -174,8 +209,7 @@ static int numa_place_domain(libxl__gc *gc, uint32_t domid,
/* Find the best candidate with enough free memory and at least
* as much pcpus as the domain has vcpus. */
rc = libxl__get_numa_candidate(gc, memkb, info->max_vcpus,
- 0, 0, &cpupool_info.cpumap,
- numa_cmpf, &candidate, &found);
+ 0, 0, map, numa_cmpf, &candidate, &found);
if (rc)
goto out;
@@ -206,6 +240,7 @@ static int numa_place_domain(libxl__gc *gc, uint32_t domid,
out:
libxl__numa_candidate_dispose(&candidate);
libxl_bitmap_dispose(&cpupool_nodemap);
+libxl_bitmap_dispose(&cpumap);
libxl_cpupoolinfo_dispose(&cpupool_info);
return rc;
}
@@ -379,9 +414,8 @@ 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)) {
-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) "
+if (info->cpumap.size || info->num_vcpu_soft_affinity)
+LOG(WARN, "Can't run NUMA placement, as a soft "
"affinity has been specified explicitly");
else if (info->nodemap.size)
LOG(WARN, "Can't run NUMA placement, as the domain has "
diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
index 971ec1bc56..ad6774a7f7 100644
--- a/tools/xl/xl_parse.c
+++ b/tools/xl/xl_parse.c
@@ -356,7 +356,7 @@ static void parse_vcpu_affinity(libxl_domain_build_info
*b_info,
j++;
}
-/*
