Skip to contentSkip to conversation

powerpc: add rcu_read_lock() to gup_fast() implementation

PATCH In review [email protected] 3 participants 4 messages Apr 27, 2010 – Apr 28, 2010 PowerPC

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 powerpc: add rcu_read_lock() to gup_fast() implementation

From: Peter Zijlstra <[email protected]>

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.

---

 arch/powerpc/mm/gup.c |    3 +++
 1 file changed, 3 insertions(+)
arch/powerpc/mm/gup.c renamed from arch/powerpc/mm/gup.c~powerpc-add-rcu_read_lock-to-gup_fast-implementation +3 -0
diff -puN arch/powerpc/mm/gup.c~powerpc-add-rcu_read_lock-to-gup_fast-implementation 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);
 

_

#1 lore raw
Quoted 9 lines · Andrew Morton

From: Peter Zijlstra <[email protected]>

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.

Quoted 6 lines

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.

Quoted 33 lines

---

 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

arch/powerpc/mm/gup.c renamed from arch/powerpc/mm/gup.c~powerpc-add-rcu_read_lock-to-gup_fast-implementation +3 -0
@@ -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);
 

_

#2 lore raw
Quoted 3 lines · Nick Piggin

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.

#3 lore raw
In reply to [email protected]
Quoted 8 lines · On Tue, 2010-04-27 at 14:10 -0700, [email protected] wrote:

From: Peter Zijlstra <[email protected]>

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.

Quoted 39 lines

---

 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

arch/powerpc/mm/gup.c renamed from arch/powerpc/mm/gup.c~powerpc-add-rcu_read_lock-to-gup_fast-implementation +3 -0
@@ -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);
 

_