: Fix deadlock in mm_pin

PATCH In review Chris Lalancette 2 participants 3 messages Nov 20, 2008

Share & tools

Chris Lalancette

author

2008-11-20 10:31 UTC #0 lore raw

PATCH 1/1 : Fix deadlock in mm_pin

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 <[email protected]>

Signed-off-by Chris Lalancette <[email protected]>

arch/x86_64/kernel/ldt-xen.c renamed from arch/x86_64/kernel/ldt-xen.c.orig +8 -4

@@ -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);
 	}
 }

arch/x86_64/mm/pageattr-xen.c renamed from arch/x86_64/mm/pageattr-xen.c.orig +8 -4

@@ -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);
 }

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

Keir Fraser

2008-11-20 14:46 UTC #1 lore raw

Quoted 6 lines · Chris Lalancette

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

Quoted 8 lines

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

Chris Lalancette

author

2008-11-20 18:37 UTC #2 lore raw

Quoted 17 lines · Keir Fraser

Quoted 6 lines · Chris Lalancette
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%.
Quoted 8 lines
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."

Quoted 7 lines

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