[PATCH]: Fix deadlock in mm_pin

[PATCH]: Fix deadlock in mm_pin

[Chris Lalancette]

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

All,
    Attached is a patch to fix a deadlock that can occur in the Xen kernel.  The
patch comes from Oracle, originally reported against the RHEL-4 PV kernel, but
it applies to the 2.6.18 tree as well; the deadlock scenario is below.

"After running an arbitrary workload involving network traffic for some time
(1-2 days), a xen guest running the 2.6.9-67 x86_64 xenU kernel locks up with
both vcpu's spinning at 100%.

The problem is due to a race between the scheduler and network interrupts.  On
one vcpu, the scheduler takes the runqueue spinlock of the other vcpu to
schedule a process, and attempts to lock mm_unpinned_lock.  On the other vcpu,
another process is holding mm_unpinned_lock (because it is starting or
exiting), and is interrupted by a network interrupt.  The network interrupt
handler attempts to wake up the same process that the first vcpu is trying to
schedule, and will try to get the runqueue spinlock that the first vcpu is
already holding."

The fix is fairly simple; make sure to take mm_unpinned_lock with
spin_lock_irqsave() so that we can't be interrupted on this vcpu until after we
leave the critical section.

Signed-off-by: Herbert van den Bergh <herbert.van.den.bergh@oracle.com>
Signed-off-by: Chris Lalancette <clalance@redhat.com>

[-- Attachment #2: linux-2.6.18-xen-mm-rq-deadlock.patch --]
[-- Type: text/x-patch, Size: 2604 bytes --]

--- linux-2.6.18.noarch/arch/x86_64/kernel/ldt-xen.c.orig	2008-11-06 10:18:21.000000000 -0500
+++ linux-2.6.18.noarch/arch/x86_64/kernel/ldt-xen.c	2008-11-06 10:19:48.000000000 -0500
@@ -109,6 +109,8 @@ static inline int copy_ldt(mm_context_t 
  */
 int init_new_context(struct task_struct *tsk, struct mm_struct *mm)
 {
+	unsigned long flags;
+
 	struct mm_struct * old_mm;
 	int retval = 0;
 
@@ -121,9 +123,9 @@ int init_new_context(struct task_struct 
 		up(&old_mm->context.sem);
 	}
 	if (retval == 0) {
-		spin_lock(&mm_unpinned_lock);
+		spin_lock_irqsave(&mm_unpinned_lock, flags);
 		list_add(&mm->context.unpinned, &mm_unpinned);
-		spin_unlock(&mm_unpinned_lock);
+		spin_unlock_irqrestore(&mm_unpinned_lock, flags);
 	}
 	return retval;
 }
@@ -134,6 +136,8 @@ int init_new_context(struct task_struct 
  */
 void destroy_context(struct mm_struct *mm)
 {
+	unsigned long flags;
+
 	if (mm->context.size) {
 		if (mm == current->active_mm)
 			clear_LDT();
@@ -148,9 +152,9 @@ void destroy_context(struct mm_struct *m
 		mm->context.size = 0;
 	}
 	if (!mm->context.pinned) {
-		spin_lock(&mm_unpinned_lock);
+		spin_lock_irqsave(&mm_unpinned_lock, flags);
 		list_del(&mm->context.unpinned);
-		spin_unlock(&mm_unpinned_lock);
+		spin_unlock_irqrestore(&mm_unpinned_lock, flags);
 	}
 }
 
--- linux-2.6.18.noarch/arch/x86_64/mm/pageattr-xen.c.orig	2008-11-06 10:16:01.000000000 -0500
+++ linux-2.6.18.noarch/arch/x86_64/mm/pageattr-xen.c	2008-11-06 10:18:10.000000000 -0500
@@ -70,6 +70,8 @@ static void mm_walk(struct mm_struct *mm
 
 void mm_pin(struct mm_struct *mm)
 {
+	unsigned long flags;
+
 	if (xen_feature(XENFEAT_writable_page_tables))
 		return;
 
@@ -87,15 +89,17 @@ void mm_pin(struct mm_struct *mm)
 	xen_pgd_pin(__pa(mm->pgd)); /* kernel */
 	xen_pgd_pin(__pa(__user_pgd(mm->pgd))); /* user */
 	mm->context.pinned = 1;
-	spin_lock(&mm_unpinned_lock);
+	spin_lock_irqsave(&mm_unpinned_lock, flags);
 	list_del(&mm->context.unpinned);
-	spin_unlock(&mm_unpinned_lock);
+	spin_unlock_irqrestore(&mm_unpinned_lock, flags);
 
 	spin_unlock(&mm->page_table_lock);
 }
 
 void mm_unpin(struct mm_struct *mm)
 {
+	unsigned long flags;
+
 	if (xen_feature(XENFEAT_writable_page_tables))
 		return;
 
@@ -112,9 +116,9 @@ void mm_unpin(struct mm_struct *mm)
 	mm_walk(mm, PAGE_KERNEL);
 	xen_tlb_flush();
 	mm->context.pinned = 0;
-	spin_lock(&mm_unpinned_lock);
+	spin_lock_irqsave(&mm_unpinned_lock, flags);
 	list_add(&mm->context.unpinned, &mm_unpinned);
-	spin_unlock(&mm_unpinned_lock);
+	spin_unlock_irqrestore(&mm_unpinned_lock, flags);
 
 	spin_unlock(&mm->page_table_lock);
 }


[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [PATCH]: Fix deadlock in mm_pin

[Keir Fraser]

On 20/11/08 10:31, "Chris Lalancette" <clalance@redhat.com> wrote:

> it applies to the 2.6.18 tree as well; the deadlock scenario is below.
> 
> "After running an arbitrary workload involving network traffic for some time
> (1-2 days), a xen guest running the 2.6.9-67 x86_64 xenU kernel locks up with
> both vcpu's spinning at 100%.
> 
> The problem is due to a race between the scheduler and network interrupts.  On
> one vcpu, the scheduler takes the runqueue spinlock of the other vcpu to
> schedule a process, and attempts to lock mm_unpinned_lock.  On the other vcpu,
> another process is holding mm_unpinned_lock (because it is starting or
> exiting), and is interrupted by a network interrupt.  The network interrupt
> handler attempts to wake up the same process that the first vcpu is trying to
> schedule, and will try to get the runqueue spinlock that the first vcpu is
> already holding."

I don't believe that mm_unpinned_lock can ever be taken while a runqueue
lock is already held in 2.6.18. If you can provide a call chain then I'll
consider the patch -- but I think you'd still be screwed by the
mm->page_table_lock (also acquired in mm_pin() code, also not IRQ safe, but
less easy for you to go convert all the users of that lock).

You might have some backporting from 2.6.18 to do...

 -- Keir

Re: [PATCH]: Fix deadlock in mm_pin

[Chris Lalancette]

Keir Fraser wrote:
> On 20/11/08 10:31, "Chris Lalancette" <clalance@redhat.com> wrote:
> 
>> it applies to the 2.6.18 tree as well; the deadlock scenario is below.
>>
>> "After running an arbitrary workload involving network traffic for some time
>> (1-2 days), a xen guest running the 2.6.9-67 x86_64 xenU kernel locks up with
>> both vcpu's spinning at 100%.
>>
>> The problem is due to a race between the scheduler and network interrupts.  On
>> one vcpu, the scheduler takes the runqueue spinlock of the other vcpu to
>> schedule a process, and attempts to lock mm_unpinned_lock.  On the other vcpu,
>> another process is holding mm_unpinned_lock (because it is starting or
>> exiting), and is interrupted by a network interrupt.  The network interrupt
>> handler attempts to wake up the same process that the first vcpu is trying to
>> schedule, and will try to get the runqueue spinlock that the first vcpu is
>> already holding."
> 
> I don't believe that mm_unpinned_lock can ever be taken while a runqueue
> lock is already held in 2.6.18. If you can provide a call chain then I'll
> consider the patch -- but I think you'd still be screwed by the
> mm->page_table_lock (also acquired in mm_pin() code, also not IRQ safe, but
> less easy for you to go convert all the users of that lock).
> 
> You might have some backporting from 2.6.18 to do...

Arg.  I think I see what you mean.  In c/s 10343, mm_pin is moved from switch_mm
into activate_mm, which I *think* means that it is no longer called with the
runqueue lock held.  Indeed, the comment on that c/s says it removes a deadlock,
which may be the one the RHEL-4 kernel is running into.  OK, thanks for the
feedback, I'll look at backporting that code.

Chris Lalancette