All of lore.kernel.org
 help / color / mirror / Atom feed
From: Zachary Amsden <zach@vmware.com>
To: Linus Torvalds <torvalds@osdl.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	Andrew Morton <akpm@osdl.org>,
	Jeremy Fitzhardinge <jeremy@goop.org>,
	Chris Wright <chrisw@sous-sol.org>,
	stable@kernel.org, Rusty Russell <rusty@rustcorp.com.au>,
	Virtualization Mailing List <virtualization@lists.osdl.org>,
	Andi Kleen <ak@suse.de>
Subject: [PATCH] Fix preemptible lazy mode bug
Date: Thu, 23 Aug 2007 22:46:48 -0700	[thread overview]
Message-ID: <46CE70C8.2030005@vmware.com> (raw)

[-- Attachment #1: Type: text/plain, Size: 993 bytes --]

I recently sent off a fix for lazy vmalloc faults which can happen under 
paravirt when lazy mode is enabled.  Unfortunately, I jumped the gun a 
bit on fixing this.  I neglected to notice that since the new call to 
flush the MMU update queue is called from the page fault handler, it can 
be pre-empted.  Both VMI and Xen use per-cpu variables to track lazy 
mode state, as all previous calls to set, disable, or flush lazy mode 
happened from a non-preemptable state.

I have no idea how to convincingly produce the problem, as generating a 
kernel pre-emption at the required point is, um, difficult, but it is 
most certainly a real possibility, and potentially more likely than the 
bug I fixed originally.

Rusty, you may have to modify lguest code if you use lazy mode and rely 
on per-cpu variables during the callout for paravirt_ops.set_lazy_mode.

I have tested as best as I can, and am trying to write a suite destined 
for LTP which will help catch and debug these issues.

Zach

[-- Attachment #2: i386-paravirt-preempt-fix.patch --]
[-- Type: text/x-patch, Size: 2256 bytes --]

Since set_lazy_mode(LAZY_MODE_FLUSH) is now called from the page fault
handler, it can potentially happen in a preemptible state.  We therefore
must make all lazy mode paravirt-ops handlers non-preemptible.

Signed-off-by: Zachary Amsden <zamsden@mysore.(none)>
---
 arch/i386/kernel/vmi.c    |   14 ++++++++++----
 arch/i386/xen/enlighten.c |    4 +++-
 2 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/arch/i386/kernel/vmi.c b/arch/i386/kernel/vmi.c
index 18673e0..9e669cb 100644
--- a/arch/i386/kernel/vmi.c
+++ b/arch/i386/kernel/vmi.c
@@ -555,21 +555,27 @@ vmi_startup_ipi_hook(int phys_apicid, unsigned long start_eip,
 static void vmi_set_lazy_mode(enum paravirt_lazy_mode mode)
 {
 	static DEFINE_PER_CPU(enum paravirt_lazy_mode, lazy_mode);
+	int cpu;
+	enum paravirt_lazy_mode cur_mode;
 
 	if (!vmi_ops.set_lazy_mode)
 		return;
 
+	cpu = get_cpu();
+	cur_mode = per_cpu(lazy_mode, cpu);
+
 	/* Modes should never nest or overlap */
-	BUG_ON(__get_cpu_var(lazy_mode) && !(mode == PARAVIRT_LAZY_NONE ||
-					     mode == PARAVIRT_LAZY_FLUSH));
+	BUG_ON(cur_mode && !(mode == PARAVIRT_LAZY_NONE ||
+			     mode == PARAVIRT_LAZY_FLUSH));
 
 	if (mode == PARAVIRT_LAZY_FLUSH) {
 		vmi_ops.set_lazy_mode(0);
-		vmi_ops.set_lazy_mode(__get_cpu_var(lazy_mode));
+		vmi_ops.set_lazy_mode(cur_mode);
 	} else {
 		vmi_ops.set_lazy_mode(mode);
-		__get_cpu_var(lazy_mode) = mode;
+		per_cpu(lazy_mode, cpu) = mode;
 	}
+	put_cpu();
 }
 
 static inline int __init check_vmi_rom(struct vrom_header *rom)
diff --git a/arch/i386/xen/enlighten.c b/arch/i386/xen/enlighten.c
index f0c3751..2dafb8a 100644
--- a/arch/i386/xen/enlighten.c
+++ b/arch/i386/xen/enlighten.c
@@ -251,7 +251,7 @@ static void xen_halt(void)
 
 static void xen_set_lazy_mode(enum paravirt_lazy_mode mode)
 {
-	BUG_ON(preemptible());
+	get_cpu();
 
 	switch (mode) {
 	case PARAVIRT_LAZY_NONE:
@@ -267,11 +267,13 @@ static void xen_set_lazy_mode(enum paravirt_lazy_mode mode)
 		/* flush if necessary, but don't change state */
 		if (x86_read_percpu(xen_lazy_mode) != PARAVIRT_LAZY_NONE)
 			xen_mc_flush();
+		put_cpu();
 		return;
 	}
 
 	xen_mc_flush();
 	x86_write_percpu(xen_lazy_mode, mode);
+	put_cpu();
 }
 
 static unsigned long xen_store_tr(void)
-- 
1.4.4.4


             reply	other threads:[~2007-08-24  5:52 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2007-08-24  5:46 Zachary Amsden [this message]
2007-08-24  6:53 ` [PATCH] Fix preemptible lazy mode bug Jeremy Fitzhardinge
2007-08-24  6:59   ` Zachary Amsden
2007-08-25 11:57     ` Rusty Russell
2007-09-01 21:09     ` Jeremy Fitzhardinge
2007-09-01 21:09       ` Jeremy Fitzhardinge
2007-09-03 20:14       ` Rusty Russell
2007-09-04 13:42         ` Jeremy Fitzhardinge
2007-09-05 16:33           ` Rusty Russell
2007-09-05 17:05             ` Zachary Amsden
2007-09-05 17:48               ` Rusty Russell
2007-09-05 20:10             ` Jeremy Fitzhardinge
2007-09-05 20:37 ` Rusty Russell
2007-09-05 23:49   ` Zachary Amsden
2007-09-06  5:41     ` Andi Kleen
2007-09-06  9:56       ` Jeremy Fitzhardinge
2007-09-06  9:57       ` Jeremy Fitzhardinge
  -- strict thread matches above, loose matches on Subject: below --
2007-08-24  5:46 Zachary Amsden

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=46CE70C8.2030005@vmware.com \
    --to=zach@vmware.com \
    --cc=ak@suse.de \
    --cc=akpm@osdl.org \
    --cc=chrisw@sous-sol.org \
    --cc=jeremy@goop.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=rusty@rustcorp.com.au \
    --cc=stable@kernel.org \
    --cc=torvalds@osdl.org \
    --cc=virtualization@lists.osdl.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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.