Patchwork [1/1] powerpc: add rcu_read_lock() to gup_fast() implementation

login
register
mail settings
Submitter Andrew Morton
Date April 27, 2010, 9:10 p.m.
Message ID <201004272110.o3RLAl7P019943@imap1.linux-foundation.org>
Download mbox | patch
Permalink /patch/51102/
State Rejected
Headers show

Comments

Andrew Morton - April 27, 2010, 9:10 p.m.
From: Peter Zijlstra <a.p.zijlstra@chello.nl>

The powerpc page table freeing relies on the fact that IRQs hold off an
RCU grace period, this is currently true for all existing RCU
implementations but is not an assumption Paul wants to support.

Therefore, also take the RCU read lock along with disabling IRQs to ensure
the RCU grace period does at least cover these lookups.

Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
Requested-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Cc: Nick Piggin <npiggin@suse.de>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Reviewed-by: Rik van Riel <riel@redhat.com>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 arch/powerpc/mm/gup.c |    3 +++
 1 file changed, 3 insertions(+)
Nick Piggin - April 28, 2010, 3:29 a.m.
On Tue, Apr 27, 2010 at 02:10:47PM -0700, Andrew Morton wrote:
> From: Peter Zijlstra <a.p.zijlstra@chello.nl>
> 
> The powerpc page table freeing relies on the fact that IRQs hold off an
> RCU grace period, this is currently true for all existing RCU
> implementations but is not an assumption Paul wants to support.
> 
> Therefore, also take the RCU read lock along with disabling IRQs to ensure
> the RCU grace period does at least cover these lookups.
> 
> Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
> Requested-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> Cc: Nick Piggin <npiggin@suse.de>
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Reviewed-by: Rik van Riel <riel@redhat.com>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>

I think I nacked this because the rest of the powerpc code also
assumes irq disables provide an rcu critical section. The plan was
to convert powerpc pagetable code to use call_rcu_sched.

> ---
> 
>  arch/powerpc/mm/gup.c |    3 +++
>  1 file changed, 3 insertions(+)
> 
> diff -puN arch/powerpc/mm/gup.c~powerpc-add-rcu_read_lock-to-gup_fast-implementation arch/powerpc/mm/gup.c
> --- a/arch/powerpc/mm/gup.c~powerpc-add-rcu_read_lock-to-gup_fast-implementation
> +++ a/arch/powerpc/mm/gup.c
> @@ -142,6 +142,7 @@ int get_user_pages_fast(unsigned long st
>  	 * So long as we atomically load page table pointers versus teardown,
>  	 * we can follow the address down to the the page and take a ref on it.
>  	 */
> +	rcu_read_lock();
>  	local_irq_disable();
>  
>  	pgdp = pgd_offset(mm, addr);
> @@ -162,6 +163,7 @@ int get_user_pages_fast(unsigned long st
>  	} while (pgdp++, addr = next, addr != end);
>  
>  	local_irq_enable();
> +	rcu_read_unlock();
>  
>  	VM_BUG_ON(nr != (end - start) >> PAGE_SHIFT);
>  	return nr;
> @@ -171,6 +173,7 @@ int get_user_pages_fast(unsigned long st
>  
>  slow:
>  		local_irq_enable();
> +		rcu_read_unlock();
>  slow_irqon:
>  		pr_devel("  slow path ! nr = %d\n", nr);
>  
> _
Benjamin Herrenschmidt - April 28, 2010, 4:10 a.m.
On Wed, 2010-04-28 at 13:29 +1000, Nick Piggin wrote:
> I think I nacked this because the rest of the powerpc code also
> assumes irq disables provide an rcu critical section. The plan was
> to convert powerpc pagetable code to use call_rcu_sched.

Right, on my todo list.

Cheers,
Ben.
Benjamin Herrenschmidt - April 28, 2010, 4:26 a.m.
On Tue, 2010-04-27 at 14:10 -0700, akpm@linux-foundation.org wrote:
> From: Peter Zijlstra <a.p.zijlstra@chello.nl>
> 
> The powerpc page table freeing relies on the fact that IRQs hold off an
> RCU grace period, this is currently true for all existing RCU
> implementations but is not an assumption Paul wants to support.
> 
> Therefore, also take the RCU read lock along with disabling IRQs to ensure
> the RCU grace period does at least cover these lookups.

Nah, that's not right. The right fix is to use call_rcu_sched() from
the powerpc page table freeing code. Please drop.

Cheers,
Ben.

> Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
> Requested-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> Cc: Nick Piggin <npiggin@suse.de>
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Reviewed-by: Rik van Riel <riel@redhat.com>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> ---
> 
>  arch/powerpc/mm/gup.c |    3 +++
>  1 file changed, 3 insertions(+)
> 
> diff -puN arch/powerpc/mm/gup.c~powerpc-add-rcu_read_lock-to-gup_fast-implementation arch/powerpc/mm/gup.c
> --- a/arch/powerpc/mm/gup.c~powerpc-add-rcu_read_lock-to-gup_fast-implementation
> +++ a/arch/powerpc/mm/gup.c
> @@ -142,6 +142,7 @@ int get_user_pages_fast(unsigned long st
>  	 * So long as we atomically load page table pointers versus teardown,
>  	 * we can follow the address down to the the page and take a ref on it.
>  	 */
> +	rcu_read_lock();
>  	local_irq_disable();
>  
>  	pgdp = pgd_offset(mm, addr);
> @@ -162,6 +163,7 @@ int get_user_pages_fast(unsigned long st
>  	} while (pgdp++, addr = next, addr != end);
>  
>  	local_irq_enable();
> +	rcu_read_unlock();
>  
>  	VM_BUG_ON(nr != (end - start) >> PAGE_SHIFT);
>  	return nr;
> @@ -171,6 +173,7 @@ int get_user_pages_fast(unsigned long st
>  
>  slow:
>  		local_irq_enable();
> +		rcu_read_unlock();
>  slow_irqon:
>  		pr_devel("  slow path ! nr = %d\n", nr);
>  
> _

Patch

diff -puN arch/powerpc/mm/gup.c~powerpc-add-rcu_read_lock-to-gup_fast-implementation arch/powerpc/mm/gup.c
--- a/arch/powerpc/mm/gup.c~powerpc-add-rcu_read_lock-to-gup_fast-implementation
+++ a/arch/powerpc/mm/gup.c
@@ -142,6 +142,7 @@  int get_user_pages_fast(unsigned long st
 	 * So long as we atomically load page table pointers versus teardown,
 	 * we can follow the address down to the the page and take a ref on it.
 	 */
+	rcu_read_lock();
 	local_irq_disable();
 
 	pgdp = pgd_offset(mm, addr);
@@ -162,6 +163,7 @@  int get_user_pages_fast(unsigned long st
 	} while (pgdp++, addr = next, addr != end);
 
 	local_irq_enable();
+	rcu_read_unlock();
 
 	VM_BUG_ON(nr != (end - start) >> PAGE_SHIFT);
 	return nr;
@@ -171,6 +173,7 @@  int get_user_pages_fast(unsigned long st
 
 slow:
 		local_irq_enable();
+		rcu_read_unlock();
 slow_irqon:
 		pr_devel("  slow path ! nr = %d\n", nr);