xen-devel.lists.xenproject.org archive mirror
 help / color / mirror / Atom feed
From: David Vrabel <david.vrabel@citrix.com>
To: xen-devel@lists.xen.org
Cc: George Dunlap <george.dunlap@eu.citrix.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	Juergen Gross <juergen.gross@ts.fujitsu.com>,
	David Vrabel <david.vrabel@citrix.com>
Subject: [PATCH] sched: fix race between sched_move_domain() and vcpu_wake()
Date: Thu, 10 Oct 2013 18:29:56 +0100	[thread overview]
Message-ID: <1381426196-11392-1-git-send-email-david.vrabel@citrix.com> (raw)

From: David Vrabel <david.vrabel@citrix.com>

sched_move_domain() changes v->processor for all the domain's VCPUs.
If another domain, softirq etc. triggers a simultaneous call to
vcpu_wake() (e.g., by setting an event channel as pending), then
vcpu_wake() may lock one schedule lock and try to unlock another.

vcpu_schedule_lock() attempts to handle this but only does so for the
window between reading the schedule_lock from the per-CPU data and the
spin_lock() call.  This does not help with sched_move_domain()
changing v->processor between the calls to vcpu_schedule_lock() and
vcpu_schedule_unlock().

Fix the race by taking the schedule_lock for v->processor in
sched_move_domain().

Signed-off-by: David Vrabel <david.vrabel@citrix.com>
Cc: George Dunlap <george.dunlap@eu.citrix.com>
Cc: Juergen Gross <juergen.gross@ts.fujitsu.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---

Just taking the lock for the old processor seemed sufficient to me as
anything seeing the new value would lock and unlock using the same new
value.  But do we need to take the schedule_lock for the new processor
as well (in the right order of course)?

This is reproducable by constantly migrating a domain between two CPU
pools.
8<------------
while true; do
    xl cpupool-migrate $1 Pool-1
    xl cpupool-migrate $1 Pool-0
done
---
 xen/common/schedule.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/xen/common/schedule.c b/xen/common/schedule.c
index 1ddfb22..28e063e 100644
--- a/xen/common/schedule.c
+++ b/xen/common/schedule.c
@@ -278,6 +278,9 @@ int sched_move_domain(struct domain *d, struct cpupool *c)
     new_p = cpumask_first(c->cpu_valid);
     for_each_vcpu ( d, v )
     {
+        spinlock_t *schedule_lock = per_cpu(schedule_data,
+                                            v->processor).schedule_lock;
+
         vcpudata = v->sched_priv;
 
         migrate_timer(&v->periodic_timer, new_p);
@@ -285,7 +288,11 @@ int sched_move_domain(struct domain *d, struct cpupool *c)
         migrate_timer(&v->poll_timer, new_p);
 
         cpumask_setall(v->cpu_affinity);
+
+        spin_lock_irq(schedule_lock);
         v->processor = new_p;
+        spin_unlock_irq(schedule_lock);
+
         v->sched_priv = vcpu_priv[v->vcpu_id];
         evtchn_move_pirqs(v);
 
-- 
1.7.2.5

             reply	other threads:[~2013-10-10 17:29 UTC|newest]

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2013-10-10 17:29 David Vrabel [this message]
2013-10-10 18:01 ` [PATCH] sched: fix race between sched_move_domain() and vcpu_wake() Andrew Cooper
2013-10-10 18:27   ` Keir Fraser
2013-10-11  7:12     ` Jan Beulich
2013-10-11  8:07       ` Keir Fraser
2013-10-11  9:02         ` Andrew Cooper
2013-10-11  9:32           ` Jan Beulich
2013-10-11  9:36             ` David Vrabel
2013-10-11  9:37               ` Jan Beulich
2013-10-11 12:20             ` Jan Beulich
2013-10-11 14:39               ` George Dunlap
2013-10-11 14:45               ` George Dunlap
2013-10-11 15:00                 ` Processed: " xen
2013-10-11 10:36       ` George Dunlap
2013-10-11  6:37 ` Juergen Gross
2013-10-11 10:32 ` George Dunlap
2013-10-11 11:15   ` Dario Faggioli
2013-10-11 11:32     ` George Dunlap
2013-10-11 11:49       ` Dario Faggioli
2013-10-11 12:03         ` Jan Beulich
2013-10-11 11:47 ` Keir Fraser

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1381426196-11392-1-git-send-email-david.vrabel@citrix.com \
    --to=david.vrabel@citrix.com \
    --cc=andrew.cooper3@citrix.com \
    --cc=george.dunlap@eu.citrix.com \
    --cc=juergen.gross@ts.fujitsu.com \
    --cc=xen-devel@lists.xen.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).