[PATCH V3] mm: convert rcu_read_lock() to srcu_read_lock(), thus allowing to sleep in callbacks

Subsystems: memory management, memory management - core, the rest

STALE5257d

3 messages, 2 authors, 2012-03-21 · open the first message on its own page

[PATCH V3] mm: convert rcu_read_lock() to srcu_read_lock(), thus allowing to sleep in callbacks

From: Sagi Grimberg <hidden>
Date: 2012-02-08 13:56:07

Now that anon_vma lock and i_mmap_mutex are both sleepable mutex, it is possible to schedule inside invalidation callbacks
(such as invalidate_page, invalidate_range_start/end and change_pte) .
This is essential for a scheduling HW sync in RDMA drivers which apply on demand paging methods.

Signed-off-by: Sagi Grimberg <redacted>
---
 changes from V2:
 - fixed error path srcu cleanup

 include/linux/mmu_notifier.h |    3 +++
 mm/mmu_notifier.c            |   37 ++++++++++++++++++++++++-------------
 2 files changed, 27 insertions(+), 13 deletions(-)
diff --git a/include/linux/mmu_notifier.h b/include/linux/mmu_notifier.h
index 1d1b1e1..f3d6f30 100644
--- a/include/linux/mmu_notifier.h
+++ b/include/linux/mmu_notifier.h
@@ -4,6 +4,7 @@
 #include <linux/list.h>
 #include <linux/spinlock.h>
 #include <linux/mm_types.h>
+#include <linux/srcu.h>
 
 struct mmu_notifier;
 struct mmu_notifier_ops;
@@ -21,6 +22,8 @@ struct mmu_notifier_mm {
 	struct hlist_head list;
 	/* to serialize the list modifications and hlist_unhashed */
 	spinlock_t lock;
+	/* to enable sleeping in callbacks */
+	struct srcu_struct srcu;
 };
 
 struct mmu_notifier_ops {
diff --git a/mm/mmu_notifier.c b/mm/mmu_notifier.c
index 9a611d3..42c7a6c 100644
--- a/mm/mmu_notifier.c
+++ b/mm/mmu_notifier.c
@@ -67,7 +67,7 @@ void __mmu_notifier_release(struct mm_struct *mm)
 	spin_unlock(&mm->mmu_notifier_mm->lock);
 
 	/*
-	 * synchronize_rcu here prevents mmu_notifier_release to
+	 * synchronize_srcu here prevents mmu_notifier_release to
 	 * return to exit_mmap (which would proceed freeing all pages
 	 * in the mm) until the ->release method returns, if it was
 	 * invoked by mmu_notifier_unregister.
@@ -75,7 +75,7 @@ void __mmu_notifier_release(struct mm_struct *mm)
 	 * The mmu_notifier_mm can't go away from under us because one
 	 * mm_count is hold by exit_mmap.
 	 */
-	synchronize_rcu();
+	synchronize_srcu(&mm->mmu_notifier_mm->srcu);
 }
 
 /*
@@ -123,10 +123,11 @@ int __mmu_notifier_test_young(struct mm_struct *mm,
 void __mmu_notifier_change_pte(struct mm_struct *mm, unsigned long address,
 			       pte_t pte)
 {
+	int idx;
 	struct mmu_notifier *mn;
 	struct hlist_node *n;
 
-	rcu_read_lock();
+	idx = srcu_read_lock(&mm->mmu_notifier_mm->srcu);
 	hlist_for_each_entry_rcu(mn, n, &mm->mmu_notifier_mm->list, hlist) {
 		if (mn->ops->change_pte)
 			mn->ops->change_pte(mn, mm, address, pte);
@@ -137,49 +138,52 @@ void __mmu_notifier_change_pte(struct mm_struct *mm, unsigned long address,
 		else if (mn->ops->invalidate_page)
 			mn->ops->invalidate_page(mn, mm, address);
 	}
-	rcu_read_unlock();
+	srcu_read_unlock(&mm->mmu_notifier_mm->srcu, idx);
 }
 
 void __mmu_notifier_invalidate_page(struct mm_struct *mm,
 					  unsigned long address)
 {
+	int idx;
 	struct mmu_notifier *mn;
 	struct hlist_node *n;
 
-	rcu_read_lock();
+	idx = srcu_read_lock(&mm->mmu_notifier_mm->srcu);
 	hlist_for_each_entry_rcu(mn, n, &mm->mmu_notifier_mm->list, hlist) {
 		if (mn->ops->invalidate_page)
 			mn->ops->invalidate_page(mn, mm, address);
 	}
-	rcu_read_unlock();
+	srcu_read_unlock(&mm->mmu_notifier_mm->srcu, idx);
 }
 
 void __mmu_notifier_invalidate_range_start(struct mm_struct *mm,
 				  unsigned long start, unsigned long end)
 {
+	int idx;
 	struct mmu_notifier *mn;
 	struct hlist_node *n;
 
-	rcu_read_lock();
+	idx = srcu_read_lock(&mm->mmu_notifier_mm->srcu);
 	hlist_for_each_entry_rcu(mn, n, &mm->mmu_notifier_mm->list, hlist) {
 		if (mn->ops->invalidate_range_start)
 			mn->ops->invalidate_range_start(mn, mm, start, end);
 	}
-	rcu_read_unlock();
+	srcu_read_unlock(&mm->mmu_notifier_mm->srcu, idx);
 }
 
 void __mmu_notifier_invalidate_range_end(struct mm_struct *mm,
 				  unsigned long start, unsigned long end)
 {
+	int idx;
 	struct mmu_notifier *mn;
 	struct hlist_node *n;
 
-	rcu_read_lock();
+	idx = srcu_read_lock(&mm->mmu_notifier_mm->srcu);
 	hlist_for_each_entry_rcu(mn, n, &mm->mmu_notifier_mm->list, hlist) {
 		if (mn->ops->invalidate_range_end)
 			mn->ops->invalidate_range_end(mn, mm, start, end);
 	}
-	rcu_read_unlock();
+	srcu_read_unlock(&mm->mmu_notifier_mm->srcu, idx);
 }
 
 static int do_mmu_notifier_register(struct mmu_notifier *mn,
@@ -196,6 +200,9 @@ static int do_mmu_notifier_register(struct mmu_notifier *mn,
 	if (unlikely(!mmu_notifier_mm))
 		goto out;
 
+	if (init_srcu_struct(&mmu_notifier_mm->srcu))
+		goto out_cleanup;
+
 	if (take_mmap_sem)
 		down_write(&mm->mmap_sem);
 	ret = mm_take_all_locks(mm);
@@ -226,8 +233,11 @@ static int do_mmu_notifier_register(struct mmu_notifier *mn,
 out_cleanup:
 	if (take_mmap_sem)
 		up_write(&mm->mmap_sem);
-	/* kfree() does nothing if mmu_notifier_mm is NULL */
-	kfree(mmu_notifier_mm);
+
+	if (mm->mmu_notifier_mm) {
+		cleanup_srcu_struct(&mmu_notifier_mm->srcu);
+		kfree(mmu_notifier_mm);
+	}
 out:
 	BUG_ON(atomic_read(&mm->mm_users) <= 0);
 	return ret;
@@ -266,6 +276,7 @@ EXPORT_SYMBOL_GPL(__mmu_notifier_register);
 void __mmu_notifier_mm_destroy(struct mm_struct *mm)
 {
 	BUG_ON(!hlist_empty(&mm->mmu_notifier_mm->list));
+	cleanup_srcu_struct(&mm->mmu_notifier_mm->srcu);
 	kfree(mm->mmu_notifier_mm);
 	mm->mmu_notifier_mm = LIST_POISON1; /* debug */
 }
@@ -309,7 +320,7 @@ void mmu_notifier_unregister(struct mmu_notifier *mn, struct mm_struct *mm)
 	 * Wait any running method to finish, of course including
 	 * ->release if it was run by mmu_notifier_relase instead of us.
 	 */
-	synchronize_rcu();
+	synchronize_srcu(&mm->mmu_notifier_mm->srcu);
 
 	BUG_ON(atomic_read(&mm->mm_count) <= 0);
 
-- 
1.7.8.2

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH V3] mm: convert rcu_read_lock() to srcu_read_lock(), thus allowing to sleep in callbacks

From: Andrea Arcangeli <hidden>
Date: 2012-03-07 15:08:46

Hello Sagi,

On Wed, Feb 08, 2012 at 03:55:43PM +0200, Sagi Grimberg wrote:
Now that anon_vma lock and i_mmap_mutex are both sleepable mutex, it is possible to schedule inside invalidation callbacks
(such as invalidate_page, invalidate_range_start/end and change_pte) .
This is essential for a scheduling HW sync in RDMA drivers which apply on demand paging methods.
Even after this, you still can't schedule in
invalidate_page/change_pte/clear_flush_young yet because the PT lock
is still hold there and that's a spinlock.

You can only schedule in invalidate_range_start/end. It's definitely a
change in the right direction but a few more patches are needed to
make all methods schedule capable.

Originally it started with srcu but then not even
invalidate_range_start/end could schedule because of the i_mmap_lock,
so then we changed to not sleepable rcu in the upstream final version
merged to keep it optimal.

Originally I developed a version of mmu notifier that was capable of
scheduling everywhere to proof the point it was possible, but it was
deferred because of the cost of using mutex instead of spinlocks for
anon-vma and i_mmap_lock (measured by Andi as a double digit percent
snowdown in some workloads in latest upstream, but hey it's upstream
already and unconditional change, so we should at least take advantage
of it in mmu notifier now! total agreement. At least we get the full
advantage of it in terms of easier coding of mmu notifier users).

I archived the missing patches that were deferred back then just
waiting for this moment, so now I'm attaching the ones that are still
missing and that are needed in addition to the srcu change you did to
get full scheduling capability from all mmu notifier methods. They're
unlikely to apply but you can revisit these now that only the PT lock
is a problem left and we should check the free_pgtable shenanigans
too.

The new test_young method used by khugepaged to know if it's worth
collapsing an hugepage (in case it was only accessed by the secondary
MMU), probably is ok to stay non-sleep capable as it's readonly thing
and doesn't need to invalidate but in that case proper docs should be
added on which methods are sleepable and which are not.

Thanks,
Andrea

Re: [PATCH V3] mm: convert rcu_read_lock() to srcu_read_lock(), thus allowing to sleep in callbacks

From: Andrea Arcangeli <hidden>
Date: 2012-03-21 18:02:30

Hi Sagi,

There are a couple of rcu_read_lock not converted at the top.

On Wed, Feb 08, 2012 at 03:55:43PM +0200, Sagi Grimberg wrote:
quoted hunk
@@ -196,6 +200,9 @@ static int do_mmu_notifier_register(struct mmu_notifier *mn,
 	if (unlikely(!mmu_notifier_mm))
 		goto out;
 
+	if (init_srcu_struct(&mmu_notifier_mm->srcu))
+		goto out_cleanup;
+
 	if (take_mmap_sem)
 		down_write(&mm->mmap_sem);
out_cleanup will up_write if take_mmap_sem is set, and at that point
the mmap_sem hasn't been taken yet.
quoted hunk
 	ret = mm_take_all_locks(mm);
@@ -226,8 +233,11 @@ static int do_mmu_notifier_register(struct mmu_notifier *mn,
 out_cleanup:
 	if (take_mmap_sem)
 		up_write(&mm->mmap_sem);
-	/* kfree() does nothing if mmu_notifier_mm is NULL */
-	kfree(mmu_notifier_mm);
+
+	if (mm->mmu_notifier_mm) {
I guess this should be "if (mmu_notifier_mm)";

I happened to notice my older patch still applies cleanly and it has
the above issues already correct, so I'm appending it after refreshing
it to upstream.

====
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help