xen: postpone updating domains' node affinity when removing a CPU from a cpupool

93be8285 ("update domU's node-affinity on the cpupool_unassign_cpu() path") does the right thing, but does it too early. In fact, it is necessary to call domain_update_node_affinity() when a pCPU is removed from a cpupool, but that must happen after the pCPU is really gone from there.

More specifically, it is important that the update happens not before cpu_disable_scheduler() is called on that pCPU, to give that function the chance to fixup the vCPU affinity of the domains residing in the cpupool, if that is necessary.

Calling it earlier, as it happens before this change, causes (although only in rather awkward circumstances), the following:

(XEN) ****************************************
(XEN) Panic on CPU 0:
(XEN) Assertion '!cpumask_empty(dom_cpumask)' failed at domain.c:460 (XEN) ****************************************

---
 xen/common/cpupool.c |   13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

index a758a8b..ad7bbb5 100644
@@ -296,6 +296,8 @@ static int cpupool_assign_cpu_locked(struct cpupool *c, unsigned int cpu)
 static long cpupool_unassign_cpu_helper(void *info)
 {
     int cpu = cpupool_moving_cpu;
+    struct cpupool *c = info;
+    struct domain *d;
     long ret;
 
     cpupool_dprintk("cpupool_unassign_cpu(pool=%d,cpu=%d)\n",

@@ -318,6 +320,11 @@ static long cpupool_unassign_cpu_helper(void *info)
         cpupool_cpu_moving = NULL;
     }
 
+    rcu_read_lock(&domlist_read_lock);
+    for_each_domain_in_cpupool(d, c)
+        domain_update_node_affinity(d);
+    rcu_read_unlock(&domlist_read_lock);
+
 out:
     spin_unlock(&cpupool_lock);
     cpupool_dprintk("cpupool_unassign_cpu ret=%ld\n", ret);

@@ -379,12 +386,6 @@ static int cpupool_unassign_cpu(struct cpupool *c, unsigned int cpu)
     atomic_inc(&c->refcnt);
     cpupool_cpu_moving = c;
     cpumask_clear_cpu(cpu, c->cpu_valid);
-
-    rcu_read_lock(&domlist_read_lock);
-    for_each_domain_in_cpupool(d, c)
-        domain_update_node_affinity(d);
-    rcu_read_unlock(&domlist_read_lock);
-
     spin_unlock(&cpupool_lock);
 
     work_cpu = smp_processor_id();

Oh, and this affects 4.5, so, when we'll get there, a backport is in order, IMO (and I'll file the request as appropriate as soon as this will be in).

Regards,
Dario

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

It looks like domain_update_node_affinity() is already called for each domain in the cpupool in cpu_disable_scheduler(). It doesn't look like there should be a need to call it twice. Can we just remove the call to domain_update_node_affinity() in cpupool_unassign_cpu() and not add it back?

-George

Mmm.. true, actually.

I'll send a patch to that effect.

Thanks and Regards,
Dario

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

Patch below, and attached. However, I think the correct thing to do would be to just revert 93be8285 "update domU's node-affinity on the cpupool_unassign_cpu() path", wouldn't it?

Regards,
Dario

8-------------------------------------------------------------------
xen: avoid updating node affinity twice when removing a CPU from a cpupool

93be8285 ("update domU's node-affinity on the cpupool_unassign_cpu() path") introduced a call to domain_update_node_affinity() when a pCPU is removed from a cpupool, but that happens already, in cpu_disable_scheduler().

Furthermore, it causes (although only in rather awkward circumstances), the following ASSERT to trigger:

(XEN) ****************************************
(XEN) Panic on CPU 0:
(XEN) Assertion '!cpumask_empty(dom_cpumask)' failed at domain.c:460 (XEN) ****************************************

This change, therefore, undo that.

index a758a8b..cd6aab9 100644
@@ -379,12 +379,6 @@ static int cpupool_unassign_cpu(struct cpupool *c, unsigned int cpu)
     atomic_inc(&c->refcnt);
     cpupool_cpu_moving = c;
     cpumask_clear_cpu(cpu, c->cpu_valid);
-
-    rcu_read_lock(&domlist_read_lock);
-    for_each_domain_in_cpupool(d, c)
-        domain_update_node_affinity(d);
-    rcu_read_unlock(&domlist_read_lock);
-
     spin_unlock(&cpupool_lock);
 
     work_cpu = smp_processor_id();

xen: avoid updating node affinity twice when removing a CPU from a cpupool

93be8285 ("update domU's node-affinity on the cpupool_unassign_cpu() path") introduced a call to domain_update_node_affinity() when a pCPU is removed from a cpupool, but that happens already, in cpu_disable_scheduler().

Furthermore, it causes (although only in rather awkward circumstances), the following ASSERT to trigger:

(XEN) ****************************************
(XEN) Panic on CPU 0:
(XEN) Assertion '!cpumask_empty(dom_cpumask)' failed at domain.c:460 (XEN) ****************************************

This change, therefore, undo that.

index a758a8b..cd6aab9 100644
@@ -379,12 +379,6 @@ static int cpupool_unassign_cpu(struct cpupool *c, unsigned int cpu)
     atomic_inc(&c->refcnt);
     cpupool_cpu_moving = c;
     cpumask_clear_cpu(cpu, c->cpu_valid);
-
-    rcu_read_lock(&domlist_read_lock);
-    for_each_domain_in_cpupool(d, c)
-        domain_update_node_affinity(d);
-    rcu_read_unlock(&domlist_read_lock);
-
     spin_unlock(&cpupool_lock);
 
     work_cpu = smp_processor_id();

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

Indeed - if the presented patch is what we want, it should be carried out as a revert. But you'll then want to explain why you did what you did there in the first place: It surely wasn't without reason, and hence I'd be afraid the revert would re-introduce another problem. That explanation should then probably go in as description for the revert.

Jan

Because I thought it was necessary. ISTR I spotted the lack of symmetry that George is also mentioning, by looking at its _assign_ counterpart, and did not notice, at that time, that it was actually ok, as the update happens already, although in schedule.c...

It was for a wrong reason. :-)

I'm not sure I'm getting 100% of what you mean. Let me try:

<<Change 93be8285 ("update domU's node-affinity on the cpupool_unassign_cpu() path") is wrong. In fact, as a consequence of it, domains' node affinity is updated twice when a pCPU is removed from a cpupool. It seemed necessary to add such call in cpupool_unassign_cpu(), for symmetry with its _assign_ counterpart, but it went unnoticed that the update was already being performed, in cpu_disable_scheduler().

Besides being conceptually wrong, it causes issues, in the form of an ASSERT() that triggers, in debug builds.

For these reasons, 93be8285 really needs to be undone.>>

Was it something like this you were after? If not, please advise further... And sorry for the whole thing! :-/

Regards,
Dario

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

I'd say something like:
----
Revert c/s 93be8285

At the point this patch calls domain_update_node_affinity(), the vcpu hard affinities have not yet been updated; so calling it at this point can in some circumstances trigger an ASSERT().

domain_update_node_affinity() is already called in cpu_disable_scheduler(), so adding it to cpupool_unassign_cpu() is redundant. Simply reverting the patch is sufficient.
---

-George

Funny you should mention that... one of the things I was thinking of suggesting was trying to move the domain_update_node_affinity() out of cpupool.c and into schedule.c, somewhere on the path that brings up a cpu or assigns it to a pool, specifically so that the lack of symmetry didn't trip anybody up. But at a quick glance I couldn't find a likely candidate.

Now that I find out it already *has* tripped someone up, I think we had definitely better do something about it. :-)

Let me take a look and see what seems sensible...

-George

Being the author of this mess... I'm more sorry than amused, but yes I can imagine it's funny to see. :-)

That would be good, indeed.

:-)

Ok, let me know what you discover. If you're too busy and you want me to have a look, just say it (I'll do my best to get it right this time! ;-P)

Regards,
Dario

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel