Skip to contentSkip to conversation

powerpc: Fix deadlock in icswx code

PATCH In review Anton Blanchard 2 participants 3 messages Sep 14, 2011 – Sep 27, 2011 1 message not in the archive PowerPC

lore
Share & tools
lore

A message that is not in the archive (sent off-list or not yet received): 20110915052313.7127520d@kryten

Anton Blanchard

author
#1 lore raw

PATCH 1/1 powerpc: Fix deadlock in icswx code

The icswx code introduced an A-B B-A deadlock:

     CPU0                    CPU1
     ----                    ----
lock(&anon_vma->mutex);
                             lock(&mm->mmap_sem);
                             lock(&anon_vma->mutex);

lock(&mm->mmap_sem);

Instead of using the mmap_sem to keep mm_users constant, take the page table spinlock.

Signed-off-by Anton Blanchard <[email protected]>
Cc <[email protected]>

---

arch/powerpc/mm/mmu_context_hash64.c +6 -6
index 3bafc3d..4ff587e 100644
@@ -136,8 +136,8 @@ int use_cop(unsigned long acop, struct mm_struct *mm)
 	if (!mm || !acop)
 		return -EINVAL;
 
-	/* We need to make sure mm_users doesn't change */
-	down_read(&mm->mmap_sem);
+	/* The page_table_lock ensures mm_users won't change under us */
+	spin_lock(&mm->page_table_lock);
 	spin_lock(mm->context.cop_lockp);
 
 	if (mm->context.cop_pid == COP_PID_NONE) {

@@ -164,7 +164,7 @@ int use_cop(unsigned long acop, struct mm_struct *mm)
 
 out:
 	spin_unlock(mm->context.cop_lockp);
-	up_read(&mm->mmap_sem);
+	spin_unlock(&mm->page_table_lock);
 
 	return ret;
 }

@@ -185,8 +185,8 @@ void drop_cop(unsigned long acop, struct mm_struct *mm)
 	if (WARN_ON_ONCE(!mm))
 		return;
 
-	/* We need to make sure mm_users doesn't change */
-	down_read(&mm->mmap_sem);
+	/* The page_table_lock ensures mm_users won't change under us */
+	spin_lock(&mm->page_table_lock);
 	spin_lock(mm->context.cop_lockp);
 
 	mm->context.acop &= ~acop;

@@ -213,7 +213,7 @@ void drop_cop(unsigned long acop, struct mm_struct *mm)
 	}
 
 	spin_unlock(mm->context.cop_lockp);
-	up_read(&mm->mmap_sem);
+	spin_unlock(&mm->page_table_lock);
 }
 EXPORT_SYMBOL_GPL(drop_cop);
#2 lore raw

On Sep 14, 2011, at 2:43 PM, Anton Blanchard wrote:

Hi Anton,
It would really help me a lot if you could review and maybe merge this = with my earlier patch that splits this file.
<http://patchwork.ozlabs.org/patch/109103/>
All it does is split.. I promise.
You don't have to take the other stuff.. yet :)

-JX

=

Quoted 18 lines

=20
The icswx code introduced an A-B B-A deadlock:
=20

    CPU0                    CPU1
    ----                    ----
lock(&anon_vma->mutex);
                            lock(&mm->mmap_sem);
                            lock(&anon_vma->mutex);

lock(&mm->mmap_sem);
=20
Instead of using the mmap_sem to keep mm_users constant, take the page table spinlock.
=20

Signed-off-by Anton Blanchard <[email protected]>
Cc <[email protected]>

---
=20

= renamed from arch/powerpc/mm/mmu_context_hash64.c +0 -0

b/arch/powerpc/mm/mmu_context_hash64.c

arch/powerpc/mm/mmu_context_hash64.c int use_cop(unsigned long acop, struct mm_struct =

Quoted 4 lines

index 3bafc3d..4ff587e 100644

arch/powerpc/mm/mmu_context_hash64.c +0 -0
@@ -136,8 +136,8 @@ int use_cop(unsigned long acop, struct mm_struct =

*mm)

arch/powerpc/mm/mmu_context_hash64.c int use_cop(unsigned long acop, struct mm_struct =

Quoted 11 lines
arch/powerpc/mm/mmu_context_hash64.c +0 -0
 	if (!mm || !acop)
 		return -EINVAL;

=20

arch/powerpc/mm/mmu_context_hash64.c +2 -2
-	/* We need to make sure mm_users doesn't change */
-	down_read(&mm->mmap_sem);
+	/* The page_table_lock ensures mm_users won't change under us */
+	spin_lock(&mm->page_table_lock);

	spin_lock(mm->context.cop_lockp);
=20
	if (mm->context.cop_pid =3D=3D COP_PID_NONE) {
arch/powerpc/mm/mmu_context_hash64.c +0 -0
@@ -164,7 +164,7 @@ int use_cop(unsigned long acop, struct mm_struct =

*mm)

arch/powerpc/mm/mmu_context_hash64.c void drop_cop(unsigned long acop, struct mm_struct =

Quoted 9 lines

=20
out:

	spin_unlock(mm->context.cop_lockp);
arch/powerpc/mm/mmu_context_hash64.c +1 -1
-	up_read(&mm->mmap_sem);
+	spin_unlock(&mm->page_table_lock);

=20

	return ret;
}
arch/powerpc/mm/mmu_context_hash64.c +0 -0
@@ -185,8 +185,8 @@ void drop_cop(unsigned long acop, struct mm_struct =

*mm)

arch/powerpc/mm/mmu_context_hash64.c void drop_cop(unsigned long acop, struct mm_struct =

Quoted 11 lines
arch/powerpc/mm/mmu_context_hash64.c +0 -0
 	if (WARN_ON_ONCE(!mm))
 		return;

=20

arch/powerpc/mm/mmu_context_hash64.c +2 -2
-	/* We need to make sure mm_users doesn't change */
-	down_read(&mm->mmap_sem);
+	/* The page_table_lock ensures mm_users won't change under us */
+	spin_lock(&mm->page_table_lock);

	spin_lock(mm->context.cop_lockp);
=20
	mm->context.acop &=3D ~acop;
arch/powerpc/mm/mmu_context_hash64.c +0 -0
@@ -213,7 +213,7 @@ void drop_cop(unsigned long acop, struct mm_struct =

*mm)

Quoted 12 lines
arch/powerpc/mm/mmu_context_hash64.c +0 -0
 	}

=20

	spin_unlock(mm->context.cop_lockp);
arch/powerpc/mm/mmu_context_hash64.c +1 -1
-	up_read(&mm->mmap_sem);
+	spin_unlock(&mm->page_table_lock);

}
EXPORT_SYMBOL_GPL(drop_cop);
=20
_______________________________________________
Linuxppc-dev mailing list
[email protected]
https://lists.ozlabs.org/listinfo/linuxppc-dev

author
#3 lore raw

Hi Jimi,

Quoted 4 lines

It would really help me a lot if you could review and maybe merge this with my earlier patch that splits this file.
<http://patchwork.ozlabs.org/patch/109103/> All it does is split.. I promise. You don't have to take the other stuff.. yet :)

Sorry it took so long to get to this. Looking at it now.

Anton