[PATCH 1/3] frv: remove irqsave pgd_lock locking

[PATCH 1/3] frv: remove irqsave pgd_lock locking

[Michal Hocko]

a79e53d8: x86/mm: Fix pgd_lock deadlock dropped irqsave locking to fix a
deadlock. pgd_lock is not used from an irq context so we can drop
irqsave locking here as well.
The original patch was x86 only but the same applies here because both
pgd_ctor (aka mm_alloc_pgd) and pgd_dtor (aka mm_free_pgd) are not used
from an interrupt contexts.

Signed-off-by: Michal Hocko <mhocko@suse.cz>
Cc: David Howells <dhowells@redhat.com>
Cc: Andrea Arcangeli <aarcange@redhat.com>
---
 arch/frv/mm/pgalloc.c |   12 ++++--------
 1 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/arch/frv/mm/pgalloc.c b/arch/frv/mm/pgalloc.c
index 4fb63a3..9df65c6 100644
--- a/arch/frv/mm/pgalloc.c
+++ b/arch/frv/mm/pgalloc.c
@@ -104,10 +104,8 @@ static inline void pgd_list_del(pgd_t *pgd)
 
 void pgd_ctor(void *pgd)
 {
-	unsigned long flags;
-
 	if (PTRS_PER_PMD == 1)
-		spin_lock_irqsave(&pgd_lock, flags);
+		spin_lock(&pgd_lock);
 
 	memcpy((pgd_t *) pgd + USER_PGDS_IN_LAST_PML4,
 	       swapper_pg_dir + USER_PGDS_IN_LAST_PML4,
@@ -117,18 +115,16 @@ void pgd_ctor(void *pgd)
 		return;
 
 	pgd_list_add(pgd);
-	spin_unlock_irqrestore(&pgd_lock, flags);
+	spin_unlock(&pgd_lock);
 	memset(pgd, 0, USER_PGDS_IN_LAST_PML4 * sizeof(pgd_t));
 }
 
 /* never called when PTRS_PER_PMD > 1 */
 void pgd_dtor(void *pgd)
 {
-	unsigned long flags; /* can be called from interrupt context */
-
-	spin_lock_irqsave(&pgd_lock, flags);
+	spin_lock(&pgd_lock);
 	pgd_list_del(pgd);
-	spin_unlock_irqrestore(&pgd_lock, flags);
+	spin_unlock(&pgd_lock);
 }
 
 pgd_t *pgd_alloc(struct mm_struct *mm)
-- 
1.7.7.1

[PATCH 2/3] tile: remove irqsave pgd_lock locking

[Michal Hocko]

a79e53d8: x86/mm: Fix pgd_lock deadlock dropped irqsave locking to fix a
deadlock. pgd_lock is not used from an irq context so we can drop
irqsave locking here as well.
The original patch was x86 only but the same applies here
because pgd_ctor (aka mm_alloc_pgd), pgd_dtor (aka mm_free_pgd),
shatter_huge_page (not used anywhere) and vmalloc_sync_all are not used
from an interrupt contexts.

Signed-off-by: Michal Hocko <mhocko@suse.cz>
Cc: Chris Metcalf <cmetcalf@tilera.com>
Cc: Andrea Arcangeli <aarcange@redhat.com>
---
 arch/tile/mm/fault.c   |    5 ++---
 arch/tile/mm/pgtable.c |   19 +++++++------------
 2 files changed, 9 insertions(+), 15 deletions(-)

diff --git a/arch/tile/mm/fault.c b/arch/tile/mm/fault.c
index 25b7b90..8ea75a6 100644
--- a/arch/tile/mm/fault.c
+++ b/arch/tile/mm/fault.c
@@ -852,10 +852,9 @@ void vmalloc_sync_all(void)
 	BUILD_BUG_ON(PAGE_OFFSET & ~PGDIR_MASK);
 	for (address = start; address >= PAGE_OFFSET; address += PGDIR_SIZE) {
 		if (!test_bit(pgd_index(address), insync)) {
-			unsigned long flags;
 			struct list_head *pos;
 
-			spin_lock_irqsave(&pgd_lock, flags);
+			spin_lock(&pgd_lock);
 			list_for_each(pos, &pgd_list)
 				if (!vmalloc_sync_one(list_to_pgd(pos),
 								address)) {
@@ -863,7 +862,7 @@ void vmalloc_sync_all(void)
 					BUG_ON(pos != pgd_list.next);
 					break;
 				}
-			spin_unlock_irqrestore(&pgd_lock, flags);
+			spin_unlock(&pgd_lock);
 			if (pos != pgd_list.next)
 				set_bit(pgd_index(address), insync);
 		}
diff --git a/arch/tile/mm/pgtable.c b/arch/tile/mm/pgtable.c
index de7d8e2..445028c 100644
--- a/arch/tile/mm/pgtable.c
+++ b/arch/tile/mm/pgtable.c
@@ -161,7 +161,6 @@ void shatter_huge_page(unsigned long addr)
 	pgd_t *pgd;
 	pud_t *pud;
 	pmd_t *pmd;
-	unsigned long flags = 0;  /* happy compiler */
 #ifdef __PAGETABLE_PMD_FOLDED
 	struct list_head *pos;
 #endif
@@ -182,10 +181,10 @@ void shatter_huge_page(unsigned long addr)
 	 * Grab the pgd_lock, since we may need it to walk the pgd_list,
 	 * and since we need some kind of lock here to avoid races.
 	 */
-	spin_lock_irqsave(&pgd_lock, flags);
+	spin_lock(&pgd_lock);
 	if (!pmd_huge_page(*pmd)) {
 		/* Lost the race to convert the huge page. */
-		spin_unlock_irqrestore(&pgd_lock, flags);
+		spin_unlock(&pgd_lock);
 		return;
 	}
 
@@ -209,7 +208,7 @@ void shatter_huge_page(unsigned long addr)
 		     cpu_possible_mask, NULL, 0);
 
 	/* Hold the lock until the TLB flush is finished to avoid races. */
-	spin_unlock_irqrestore(&pgd_lock, flags);
+	spin_unlock(&pgd_lock);
 }
 
 /*
@@ -240,10 +239,8 @@ static inline void pgd_list_del(pgd_t *pgd)
 
 static void pgd_ctor(pgd_t *pgd)
 {
-	unsigned long flags;
-
 	memset(pgd, 0, KERNEL_PGD_INDEX_START*sizeof(pgd_t));
-	spin_lock_irqsave(&pgd_lock, flags);
+	spin_lock(&pgd_lock);
 
 #ifndef __tilegx__
 	/*
@@ -259,16 +256,14 @@ static void pgd_ctor(pgd_t *pgd)
 	       KERNEL_PGD_PTRS * sizeof(pgd_t));
 
 	pgd_list_add(pgd);
-	spin_unlock_irqrestore(&pgd_lock, flags);
+	spin_unlock(&pgd_lock);
 }
 
 static void pgd_dtor(pgd_t *pgd)
 {
-	unsigned long flags; /* can be called from interrupt context */
-
-	spin_lock_irqsave(&pgd_lock, flags);
+	spin_lock(&pgd_lock);
 	pgd_list_del(pgd);
-	spin_unlock_irqrestore(&pgd_lock, flags);
+	spin_unlock(&pgd_lock);
 }
 
 pgd_t *pgd_alloc(struct mm_struct *mm)
-- 
1.7.7.1

[PATCH 3/3] mn10300: remove irqsave pgd_lock locking

[Michal Hocko]

a79e53d8: x86/mm: Fix pgd_lock deadlock dropped irqsave locking to fix a
deadlock. pgd_lock is not used from an irq context so we can drop
irqsave locking here as well.
The original patch was x86 only but the same applies here because both
pgd_ctor (aka mm_alloc_pgd) and pgd_dtor (aka mm_free_pgd) are not used
from an interrupt contexts.

Signed-off-by: Michal Hocko <mhocko@suse.cz>
Cc: David Howells <dhowells@redhat.com>
Cc: Koichi Yasutake <yasutake.koichi@jp.panasonic.com>
Cc: linux-am33-list@redhat.com
Cc: Andrea Arcangeli <aarcange@redhat.com>
---
 arch/mn10300/mm/pgtable.c |   12 ++++--------
 1 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/arch/mn10300/mm/pgtable.c b/arch/mn10300/mm/pgtable.c
index 450f7ba..8e5e10d 100644
--- a/arch/mn10300/mm/pgtable.c
+++ b/arch/mn10300/mm/pgtable.c
@@ -123,10 +123,8 @@ static inline void pgd_list_del(pgd_t *pgd)
 
 void pgd_ctor(void *pgd)
 {
-	unsigned long flags;
-
 	if (PTRS_PER_PMD == 1)
-		spin_lock_irqsave(&pgd_lock, flags);
+		spin_lock(&pgd_lock);
 
 	memcpy((pgd_t *)pgd + USER_PTRS_PER_PGD,
 			swapper_pg_dir + USER_PTRS_PER_PGD,
@@ -136,18 +134,16 @@ void pgd_ctor(void *pgd)
 		return;
 
 	pgd_list_add(pgd);
-	spin_unlock_irqrestore(&pgd_lock, flags);
+	spin_unlock(&pgd_lock);
 	memset(pgd, 0, USER_PTRS_PER_PGD * sizeof(pgd_t));
 }
 
 /* never called when PTRS_PER_PMD > 1 */
 void pgd_dtor(void *pgd)
 {
-	unsigned long flags; /* can be called from interrupt context */
-
-	spin_lock_irqsave(&pgd_lock, flags);
+	spin_lock(&pgd_lock);
 	pgd_list_del(pgd);
-	spin_unlock_irqrestore(&pgd_lock, flags);
+	spin_unlock(&pgd_lock);
 }
 
 pgd_t *pgd_alloc(struct mm_struct *mm)
-- 
1.7.7.1

Re: [PATCH 2/3] tile: remove irqsave pgd_lock locking

[Chris Metcalf]

On 11/9/2011 10:47 AM, Michal Hocko wrote:
> a79e53d8: x86/mm: Fix pgd_lock deadlock dropped irqsave locking to fix a
> deadlock. pgd_lock is not used from an irq context so we can drop
> irqsave locking here as well.
> The original patch was x86 only but the same applies here
> because pgd_ctor (aka mm_alloc_pgd), pgd_dtor (aka mm_free_pgd),
> shatter_huge_page (not used anywhere) and vmalloc_sync_all are not used
> from an interrupt contexts.

In fact, shatter_huge_page() is used by some code in our internal version 
of arch/tile/mm/homecache.c that we haven't yet pushed back to the 
community, and that CAN be called from an interrupt context.  (For example, 
we may shatter a huge page to dynamically change the "home cache" of a 
small page when it's allocated from the page allocator, causing us to need 
to map all the memory under the old huge page with separate small pages 
that can now each carry separate "home cache" locations for those pages.)

We don't suffer from the x86 deadlock risk, since we handle TLB flushing 
using our hypervisor API, flush_remote(), which doesn't pay attention to 
the irq-disabled state on the remote core.

So this patch would not be safe for our architecture, but thanks for 
exploring the possibility.  I'll add some comments explaining this (at the 
definition of pgd_lock) to avoid confusion in the future.  In fact, 
reviewing the shatter_huge_page() code, I realize I should hold 
init_mm.page_table_lock as well as pgtable_lock to guard against another 
thread updating init_mm in parallel; I think this is only a theoretical 
issue but I'll add the locking to be consistent.

-- 
Chris Metcalf, Tilera Corp.
http://www.tilera.com

Re: [PATCH 2/3] tile: remove irqsave pgd_lock locking

[Michal Hocko]

On Thu 10-11-11 13:50:05, Chris Metcalf wrote:
> On 11/9/2011 10:47 AM, Michal Hocko wrote:
> >a79e53d8: x86/mm: Fix pgd_lock deadlock dropped irqsave locking to fix a
> >deadlock. pgd_lock is not used from an irq context so we can drop
> >irqsave locking here as well.
> >The original patch was x86 only but the same applies here
> >because pgd_ctor (aka mm_alloc_pgd), pgd_dtor (aka mm_free_pgd),
> >shatter_huge_page (not used anywhere) and vmalloc_sync_all are not used
> >from an interrupt contexts.
> 
> In fact, shatter_huge_page() is used by some code in our internal
> version of arch/tile/mm/homecache.c that we haven't yet pushed back
> to the community, and that CAN be called from an interrupt context.
> (For example, we may shatter a huge page to dynamically change the
> "home cache" of a small page when it's allocated from the page
> allocator, causing us to need to map all the memory under the old
> huge page with separate small pages that can now each carry separate
> "home cache" locations for those pages.)

OK

> 
> We don't suffer from the x86 deadlock risk, since we handle TLB
> flushing using our hypervisor API, flush_remote(), which doesn't pay
> attention to the irq-disabled state on the remote core.

this deserves a comment in the code

> 
> So this patch would not be safe for our architecture, but thanks for
> exploring the possibility.  I'll add some comments explaining this
> (at the definition of pgd_lock) to avoid confusion in the future.

great

> In fact, reviewing the shatter_huge_page() code, I realize I should
> hold init_mm.page_table_lock as well as pgtable_lock to guard
> against another thread updating init_mm in parallel; I think this is
> only a theoretical issue but I'll add the locking to be consistent.
> 
> -- 
> Chris Metcalf, Tilera Corp.
> http://www.tilera.com

Thanks
-- 
Michal Hocko
SUSE Labs
SUSE LINUX s.r.o.
Lihovarska 1060/12
190 00 Praha 9    
Czech Republic

Re: [PATCH 1/3] frv: remove irqsave pgd_lock locking

[Michal Hocko]

Ping

On Wed 09-11-11 16:47:17, Michal Hocko wrote:
> a79e53d8: x86/mm: Fix pgd_lock deadlock dropped irqsave locking to fix a
> deadlock. pgd_lock is not used from an irq context so we can drop
> irqsave locking here as well.
> The original patch was x86 only but the same applies here because both
> pgd_ctor (aka mm_alloc_pgd) and pgd_dtor (aka mm_free_pgd) are not used
> from an interrupt contexts.
> 
> Signed-off-by: Michal Hocko <mhocko@suse.cz>
> Cc: David Howells <dhowells@redhat.com>
> Cc: Andrea Arcangeli <aarcange@redhat.com>
> ---
>  arch/frv/mm/pgalloc.c |   12 ++++--------
>  1 files changed, 4 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/frv/mm/pgalloc.c b/arch/frv/mm/pgalloc.c
> index 4fb63a3..9df65c6 100644
> --- a/arch/frv/mm/pgalloc.c
> +++ b/arch/frv/mm/pgalloc.c
> @@ -104,10 +104,8 @@ static inline void pgd_list_del(pgd_t *pgd)
>  
>  void pgd_ctor(void *pgd)
>  {
> -	unsigned long flags;
> -
>  	if (PTRS_PER_PMD == 1)
> -		spin_lock_irqsave(&pgd_lock, flags);
> +		spin_lock(&pgd_lock);
>  
>  	memcpy((pgd_t *) pgd + USER_PGDS_IN_LAST_PML4,
>  	       swapper_pg_dir + USER_PGDS_IN_LAST_PML4,
> @@ -117,18 +115,16 @@ void pgd_ctor(void *pgd)
>  		return;
>  
>  	pgd_list_add(pgd);
> -	spin_unlock_irqrestore(&pgd_lock, flags);
> +	spin_unlock(&pgd_lock);
>  	memset(pgd, 0, USER_PGDS_IN_LAST_PML4 * sizeof(pgd_t));
>  }
>  
>  /* never called when PTRS_PER_PMD > 1 */
>  void pgd_dtor(void *pgd)
>  {
> -	unsigned long flags; /* can be called from interrupt context */
> -
> -	spin_lock_irqsave(&pgd_lock, flags);
> +	spin_lock(&pgd_lock);
>  	pgd_list_del(pgd);
> -	spin_unlock_irqrestore(&pgd_lock, flags);
> +	spin_unlock(&pgd_lock);
>  }
>  
>  pgd_t *pgd_alloc(struct mm_struct *mm)
> -- 
> 1.7.7.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

-- 
Michal Hocko
SUSE Labs
SUSE LINUX s.r.o.
Lihovarska 1060/12
190 00 Praha 9    
Czech Republic

Re: [PATCH 3/3] mn10300: remove irqsave pgd_lock locking

[Michal Hocko]

Ping

On Wed 09-11-11 16:47:19, Michal Hocko wrote:
> a79e53d8: x86/mm: Fix pgd_lock deadlock dropped irqsave locking to fix a
> deadlock. pgd_lock is not used from an irq context so we can drop
> irqsave locking here as well.
> The original patch was x86 only but the same applies here because both
> pgd_ctor (aka mm_alloc_pgd) and pgd_dtor (aka mm_free_pgd) are not used
> from an interrupt contexts.
> 
> Signed-off-by: Michal Hocko <mhocko@suse.cz>
> Cc: David Howells <dhowells@redhat.com>
> Cc: Koichi Yasutake <yasutake.koichi@jp.panasonic.com>
> Cc: linux-am33-list@redhat.com
> Cc: Andrea Arcangeli <aarcange@redhat.com>
> ---
>  arch/mn10300/mm/pgtable.c |   12 ++++--------
>  1 files changed, 4 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/mn10300/mm/pgtable.c b/arch/mn10300/mm/pgtable.c
> index 450f7ba..8e5e10d 100644
> --- a/arch/mn10300/mm/pgtable.c
> +++ b/arch/mn10300/mm/pgtable.c
> @@ -123,10 +123,8 @@ static inline void pgd_list_del(pgd_t *pgd)
>  
>  void pgd_ctor(void *pgd)
>  {
> -	unsigned long flags;
> -
>  	if (PTRS_PER_PMD == 1)
> -		spin_lock_irqsave(&pgd_lock, flags);
> +		spin_lock(&pgd_lock);
>  
>  	memcpy((pgd_t *)pgd + USER_PTRS_PER_PGD,
>  			swapper_pg_dir + USER_PTRS_PER_PGD,
> @@ -136,18 +134,16 @@ void pgd_ctor(void *pgd)
>  		return;
>  
>  	pgd_list_add(pgd);
> -	spin_unlock_irqrestore(&pgd_lock, flags);
> +	spin_unlock(&pgd_lock);
>  	memset(pgd, 0, USER_PTRS_PER_PGD * sizeof(pgd_t));
>  }
>  
>  /* never called when PTRS_PER_PMD > 1 */
>  void pgd_dtor(void *pgd)
>  {
> -	unsigned long flags; /* can be called from interrupt context */
> -
> -	spin_lock_irqsave(&pgd_lock, flags);
> +	spin_lock(&pgd_lock);
>  	pgd_list_del(pgd);
> -	spin_unlock_irqrestore(&pgd_lock, flags);
> +	spin_unlock(&pgd_lock);
>  }
>  
>  pgd_t *pgd_alloc(struct mm_struct *mm)
> -- 
> 1.7.7.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

-- 
Michal Hocko
SUSE Labs
SUSE LINUX s.r.o.
Lihovarska 1060/12
190 00 Praha 9    
Czech Republic