* Re: Wrong cpupool handling
2014-11-11 12:18 Wrong cpupool handling Dietmar Hahn
@ 2014-11-11 14:17 ` Juergen Gross
2014-11-11 14:21 ` Juergen Gross
1 sibling, 0 replies; 6+ messages in thread
From: Juergen Gross @ 2014-11-11 14:17 UTC (permalink / raw)
To: Dietmar Hahn, xen-devel
[-- Attachment #1: Type: text/plain, Size: 884 bytes --]
Hi Dietmar,
On 11/11/2014 01:18 PM, Dietmar Hahn wrote:
> Hi list,
>
> When creating a cpupool, starting and destroying a guest within this pool,
> then removing this pool doesn't work because of EBUSY.
>
> It seems the cause of this behavior is the commit
> bac6334b51d9bcfe57ecf4a4cb5288348fcf044a.
>
> In domain_kill() the function sched_move_domain() gets called changing the
> d->cpupool pointer to the new cpupool without incrementing/decrementing the
> counters "n_dom" of the new/old cpupool.
>
> This leads to decrementing the wrong cpupool0->n_dom counter when
> cpupool_rm_domain() gets called at the end and my own cpupool can't be
> destroyed because n_dom = 1!
>
> I don't have a fast patch because I'am not enough familiar with the code
> this time but I think it should be fixed for 4.5.
Could you try the attached (untested) patch? Should apply to HEAD.
Juergen
[-- Attachment #2: 0001-Adjust-number-of-domains-in-cpupools-when-destroying.patch --]
[-- Type: text/x-patch, Size: 3929 bytes --]
>From f231f837b6dffd071c59517286562ccde7c5b5bc Mon Sep 17 00:00:00 2001
From: Juergen Gross <jgross@suse.com>
Date: Tue, 11 Nov 2014 15:03:33 +0100
Subject: [PATCH] Adjust number of domains in cpupools when destroying domain
Commit bac6334b51d9bcfe57ecf4a4cb5288348fcf044a (move domain to
cpupool0 before destroying it) introduced an error in the accounting
of cpupools regarding the number of domains. The number of domains
is nor adjusted when a domain is moved to cpupool0 in kill_domain().
Correct this by introducing a cpupool function doing the move
instead of open coding it by calling sched_move_domain().
Signed-off-by: Juergen Gross <jgross@suse.com>
---
xen/common/cpupool.c | 46 +++++++++++++++++++++++++++++++++-------------
xen/common/domain.c | 2 +-
xen/include/xen/sched.h | 1 +
3 files changed, 35 insertions(+), 14 deletions(-)
diff --git a/xen/common/cpupool.c b/xen/common/cpupool.c
index 73249d3..552d791 100644
--- a/xen/common/cpupool.c
+++ b/xen/common/cpupool.c
@@ -225,6 +225,35 @@ static int cpupool_destroy(struct cpupool *c)
}
/*
+ * Move domain to another cpupool
+ */
+static int cpupool_move_domain_unlocked(struct domain *d, struct cpupool *c)
+{
+ int ret;
+
+ d->cpupool->n_dom--;
+ ret = sched_move_domain(d, c);
+ if ( ret )
+ d->cpupool->n_dom++;
+ else
+ c->n_dom++;
+
+ return ret;
+}
+int cpupool_move_domain(struct domain *d, struct cpupool *c)
+{
+ int ret;
+
+ spin_lock(&cpupool_lock);
+
+ ret = cpupool_move_domain_unlocked(d, c);
+
+ spin_unlock(&cpupool_lock);
+
+ return ret;
+}
+
+/*
* assign a specific cpu to a cpupool
* cpupool_lock must be held
*/
@@ -338,13 +367,9 @@ static int cpupool_unassign_cpu(struct cpupool *c, unsigned int cpu)
ret = -EBUSY;
break;
}
- c->n_dom--;
- ret = sched_move_domain(d, cpupool0);
+ ret = cpupool_move_domain_unlocked(d, cpupool0);
if ( ret )
- {
- c->n_dom++;
break;
- }
cpupool0->n_dom++;
}
rcu_read_unlock(&domlist_read_lock);
@@ -613,16 +638,11 @@ int cpupool_do_sysctl(struct xen_sysctl_cpupool_op *op)
d->domain_id, op->cpupool_id);
ret = -ENOENT;
spin_lock(&cpupool_lock);
+
c = cpupool_find_by_id(op->cpupool_id);
if ( (c != NULL) && cpumask_weight(c->cpu_valid) )
- {
- d->cpupool->n_dom--;
- ret = sched_move_domain(d, c);
- if ( ret )
- d->cpupool->n_dom++;
- else
- c->n_dom++;
- }
+ ret = cpupool_move_domain_unlocked(d, c);
+
spin_unlock(&cpupool_lock);
cpupool_dprintk("cpupool move_domain(dom=%d)->pool=%d ret %d\n",
d->domain_id, op->cpupool_id, ret);
diff --git a/xen/common/domain.c b/xen/common/domain.c
index a3f51ec..4a62c1d 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -621,7 +621,7 @@ int domain_kill(struct domain *d)
rc = -EAGAIN;
break;
}
- if ( sched_move_domain(d, cpupool0) )
+ if ( cpupool_move_domain(d, cpupool0) )
return -EAGAIN;
for_each_vcpu ( d, v )
unmap_vcpu_info(v);
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index c5157e6..46fc6e3 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -871,6 +871,7 @@ struct cpupool *cpupool_get_by_id(int poolid);
void cpupool_put(struct cpupool *pool);
int cpupool_add_domain(struct domain *d, int poolid);
void cpupool_rm_domain(struct domain *d);
+int cpupool_move_domain(struct domain *d, struct cpupool *c);
int cpupool_do_sysctl(struct xen_sysctl_cpupool_op *op);
void schedule_dump(struct cpupool *c);
extern void dump_runq(unsigned char key);
--
2.1.2
[-- Attachment #3: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: Wrong cpupool handling
2014-11-11 12:18 Wrong cpupool handling Dietmar Hahn
2014-11-11 14:17 ` Juergen Gross
@ 2014-11-11 14:21 ` Juergen Gross
2014-11-12 9:53 ` Dietmar Hahn
1 sibling, 1 reply; 6+ messages in thread
From: Juergen Gross @ 2014-11-11 14:21 UTC (permalink / raw)
To: Dietmar Hahn, xen-devel
[-- Attachment #1: Type: text/plain, Size: 860 bytes --]
Hi again,
On 11/11/2014 01:18 PM, Dietmar Hahn wrote:
> Hi list,
>
> When creating a cpupool, starting and destroying a guest within this pool,
> then removing this pool doesn't work because of EBUSY.
>
> It seems the cause of this behavior is the commit
> bac6334b51d9bcfe57ecf4a4cb5288348fcf044a.
>
> In domain_kill() the function sched_move_domain() gets called changing the
> d->cpupool pointer to the new cpupool without incrementing/decrementing the
> counters "n_dom" of the new/old cpupool.
>
> This leads to decrementing the wrong cpupool0->n_dom counter when
> cpupool_rm_domain() gets called at the end and my own cpupool can't be
> destroyed because n_dom = 1!
>
> I don't have a fast patch because I'am not enough familiar with the code
> this time but I think it should be fixed for 4.5.
Please discard previous patch, try this one.
Juergen
[-- Attachment #2: 0001-Adjust-number-of-domains-in-cpupools-when-destroying.patch --]
[-- Type: text/x-patch, Size: 3950 bytes --]
>From 629a7fe8ed07a13304d9378d357420ec885f59e3 Mon Sep 17 00:00:00 2001
From: Juergen Gross <jgross@suse.com>
Date: Tue, 11 Nov 2014 15:03:33 +0100
Subject: [PATCH] Adjust number of domains in cpupools when destroying domain
Commit bac6334b51d9bcfe57ecf4a4cb5288348fcf044a (move domain to
cpupool0 before destroying it) introduced an error in the accounting
of cpupools regarding the number of domains. The number of domains
is nor adjusted when a domain is moved to cpupool0 in kill_domain().
Correct this by introducing a cpupool function doing the move
instead of open coding it by calling sched_move_domain().
Signed-off-by: Juergen Gross <jgross@suse.com>
---
xen/common/cpupool.c | 47 +++++++++++++++++++++++++++++++++--------------
xen/common/domain.c | 2 +-
xen/include/xen/sched.h | 1 +
3 files changed, 35 insertions(+), 15 deletions(-)
diff --git a/xen/common/cpupool.c b/xen/common/cpupool.c
index 73249d3..c6e3869 100644
--- a/xen/common/cpupool.c
+++ b/xen/common/cpupool.c
@@ -225,6 +225,35 @@ static int cpupool_destroy(struct cpupool *c)
}
/*
+ * Move domain to another cpupool
+ */
+static int cpupool_move_domain_unlocked(struct domain *d, struct cpupool *c)
+{
+ int ret;
+
+ d->cpupool->n_dom--;
+ ret = sched_move_domain(d, c);
+ if ( ret )
+ d->cpupool->n_dom++;
+ else
+ c->n_dom++;
+
+ return ret;
+}
+int cpupool_move_domain(struct domain *d, struct cpupool *c)
+{
+ int ret;
+
+ spin_lock(&cpupool_lock);
+
+ ret = cpupool_move_domain_unlocked(d, c);
+
+ spin_unlock(&cpupool_lock);
+
+ return ret;
+}
+
+/*
* assign a specific cpu to a cpupool
* cpupool_lock must be held
*/
@@ -338,14 +367,9 @@ static int cpupool_unassign_cpu(struct cpupool *c, unsigned int cpu)
ret = -EBUSY;
break;
}
- c->n_dom--;
- ret = sched_move_domain(d, cpupool0);
+ ret = cpupool_move_domain_unlocked(d, cpupool0);
if ( ret )
- {
- c->n_dom++;
break;
- }
- cpupool0->n_dom++;
}
rcu_read_unlock(&domlist_read_lock);
if ( ret )
@@ -613,16 +637,11 @@ int cpupool_do_sysctl(struct xen_sysctl_cpupool_op *op)
d->domain_id, op->cpupool_id);
ret = -ENOENT;
spin_lock(&cpupool_lock);
+
c = cpupool_find_by_id(op->cpupool_id);
if ( (c != NULL) && cpumask_weight(c->cpu_valid) )
- {
- d->cpupool->n_dom--;
- ret = sched_move_domain(d, c);
- if ( ret )
- d->cpupool->n_dom++;
- else
- c->n_dom++;
- }
+ ret = cpupool_move_domain_unlocked(d, c);
+
spin_unlock(&cpupool_lock);
cpupool_dprintk("cpupool move_domain(dom=%d)->pool=%d ret %d\n",
d->domain_id, op->cpupool_id, ret);
diff --git a/xen/common/domain.c b/xen/common/domain.c
index a3f51ec..4a62c1d 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -621,7 +621,7 @@ int domain_kill(struct domain *d)
rc = -EAGAIN;
break;
}
- if ( sched_move_domain(d, cpupool0) )
+ if ( cpupool_move_domain(d, cpupool0) )
return -EAGAIN;
for_each_vcpu ( d, v )
unmap_vcpu_info(v);
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index c5157e6..46fc6e3 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -871,6 +871,7 @@ struct cpupool *cpupool_get_by_id(int poolid);
void cpupool_put(struct cpupool *pool);
int cpupool_add_domain(struct domain *d, int poolid);
void cpupool_rm_domain(struct domain *d);
+int cpupool_move_domain(struct domain *d, struct cpupool *c);
int cpupool_do_sysctl(struct xen_sysctl_cpupool_op *op);
void schedule_dump(struct cpupool *c);
extern void dump_runq(unsigned char key);
--
2.1.2
[-- Attachment #3: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 6+ messages in thread