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

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

[Dario Faggioli]

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) ****************************************

Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
Cc: Jan Beulich <JBeulich@suse.com>
Cc: Keir Fraser <keir.xen@gmail.com>
Cc: Juergen Gross <JGross@suse.com>
Cc: George Dunlap <george.dunlap@eu.citrix.com>
Cc: Justin Weaver <jtweaver@hawaii.edu>
---
 xen/common/cpupool.c |   13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/xen/common/cpupool.c b/xen/common/cpupool.c
index a758a8b..ad7bbb5 100644
--- a/xen/common/cpupool.c
+++ b/xen/common/cpupool.c
@@ -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();

Re: [PATCH] xen: postpone updating domains' node affinity when removing a CPU from a cpupool

[Dario Faggioli]

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

On Mon, 2015-03-09 at 17:49 +0100, Dario Faggioli wrote:
> 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) ****************************************
> 
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

[-- 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

Re: [PATCH] xen: postpone updating domains' node affinity when removing a CPU from a cpupool

[George Dunlap]

On 03/09/2015 04:49 PM, Dario Faggioli wrote:
> 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.

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

Re: [PATCH] xen: postpone updating domains' node affinity when removing a CPU from a cpupool

[Dario Faggioli]

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

On Wed, 2015-03-11 at 15:01 +0000, George Dunlap wrote:
> On 03/09/2015 04:49 PM, Dario Faggioli wrote:
> > 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.
> 
> 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?
> 
Mmm.. true, actually.

I'll send a patch to that effect.

Thanks and Regards,
Dario

[-- 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] xen: avoid updating node affinity twice when removing a CPU from a cpupool

[Dario Faggioli]

[-- Attachment #1.1.1: Type: text/plain, Size: 2231 bytes --]

On Wed, 2015-03-11 at 16:04 +0000, Dario Faggioli wrote:
> On Wed, 2015-03-11 at 15:01 +0000, George Dunlap wrote:

> > 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?
> > 
> Mmm.. true, actually.
> 
> I'll send a patch to that effect.
> 
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.

Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
Cc: Juergen Gross <JGross@suse.com>
Cc: George Dunlap <george.dunlap@eu.citrix.com>
Cc: Jan Beulich <JBeulich@suse.com>
Cc: Keir Fraser <keir.xen@gmail.com>

diff --git a/xen/common/cpupool.c b/xen/common/cpupool.c
index a758a8b..cd6aab9 100644
--- a/xen/common/cpupool.c
+++ b/xen/common/cpupool.c
@@ -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();

[-- Attachment #1.1.2: xen-fix-cpupool-unassign-cpu.patch --]
[-- Type: text/x-patch, Size: 1409 bytes --]

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.

Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
Cc: Juergen Gross <JGross@suse.com>
Cc: George Dunlap <george.dunlap@eu.citrix.com>
Cc: Jan Beulich <JBeulich@suse.com>
Cc: Keir Fraser <keir.xen@gmail.com>

diff --git a/xen/common/cpupool.c b/xen/common/cpupool.c
index a758a8b..cd6aab9 100644
--- a/xen/common/cpupool.c
+++ b/xen/common/cpupool.c
@@ -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();

[-- 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

Re: [PATCH] xen: avoid updating node affinity twice when removing a CPU from a cpupool

[Jan Beulich]

>>> On 12.03.15 at 14:45, <dario.faggioli@citrix.com> wrote:
> 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?

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

Re: [PATCH] xen: avoid updating node affinity twice when removing a CPU from a cpupool

[George Dunlap]

On 03/12/2015 01:45 PM, Dario Faggioli wrote:
> On Wed, 2015-03-11 at 16:04 +0000, Dario Faggioli wrote:
>> On Wed, 2015-03-11 at 15:01 +0000, George Dunlap wrote:
> 
>>> 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?
>>>
>> Mmm.. true, actually.
>>
>> I'll send a patch to that effect.
>>
> 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?

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

Re: [PATCH] xen: avoid updating node affinity twice when removing a CPU from a cpupool

[Dario Faggioli]

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

On Thu, 2015-03-12 at 14:51 +0000, Jan Beulich wrote:
> >>> On 12.03.15 at 14:45, <dario.faggioli@citrix.com> wrote:
> > 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?
> 
> 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: 
>
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 surely wasn't without
> reason, 
>
It was for a wrong 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.
> 
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

[-- 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

Re: [PATCH] xen: avoid updating node affinity twice when removing a CPU from a cpupool

[Dario Faggioli]

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

On Thu, 2015-03-12 at 14:52 +0000, George Dunlap wrote:
> On 03/12/2015 01:45 PM, Dario Faggioli wrote:

> > 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?
> 
> Funny you should mention that... 
>
Being the author of this mess... I'm more sorry than amused, but yes I
can imagine it's funny to see. :-)

> 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.  
>
That would be good, indeed.

> 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...
> 
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

[-- 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

Re: [PATCH] xen: avoid updating node affinity twice when removing a CPU from a cpupool

[George Dunlap]

On 03/12/2015 03:52 PM, Dario Faggioli wrote:
> On Thu, 2015-03-12 at 14:51 +0000, Jan Beulich wrote:
>>>>> On 12.03.15 at 14:45, <dario.faggioli@citrix.com> wrote:
>>> 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?
>>
>> 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: 
>>
> 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 surely wasn't without
>> reason, 
>>
> It was for a wrong 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.
>>
> I'm not sure I'm getting 100% of what you mean. Let me try:

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