Re: REGRESSION: Performance regressions from switching anon_vma->lock to mutex

[Linus Torvalds <torvalds@linux-foundation.org>]

[-- Attachment #1: Type: text/plain, Size: 645 bytes --]

On Fri, Jun 17, 2011 at 9:46 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> Oh, and can you do this with a commit log and sign-off, and I'll put
> it in my "anon_vma-locking" branch that I have. I'm not going to
> actually merge that branch into mainline until I've seen a few more
> acks or more testing by Tim.

Attached is the tentative commit I have, which is yours but with the
tests for anon_vma being NULL removed, and a made-up commit log. It
works for me, but needs more testing and eyeballs looking at it.

Tim? This is on top of my previous patch, replacing Peter's two patches.

                                 Linus

[-- Attachment #2: patch.diff --]
[-- Type: text/x-patch, Size: 2910 bytes --]

commit 33e4c75ce6c23e8a9fcb32216c4d843d5e9b49e2
Author: Peter Zijlstra <peterz@infradead.org>
Date:   Fri Jun 17 13:54:23 2011 +0200

    mm: avoid repeated anon_vma lock/unlock sequences in unlink_anon_vmas()
    
    This matches the anon_vma_clone() case, and uses the same lock helper
    functions.  Because of the need to potentially release the anon_vma's,
    it's a bit more complex, though.
    
    We traverse the 'vma->anon_vma_chain' in two phases: the first loop gets
    the anon_vma lock (with the helper function that only takes the lock
    once for the whole loop), and removes any entries that don't need any
    more processing.
    
    The second phase just traverses the remaining list entries (without
    holding the anon_vma lock), and does any actual freeing of the
    anon_vma's that is required.
    
    Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
---
 mm/rmap.c |   49 ++++++++++++++++++++++++++++---------------------
 1 files changed, 28 insertions(+), 21 deletions(-)

diff --git a/mm/rmap.c b/mm/rmap.c
index f286697c61dc..68756a77ef87 100644
--- a/mm/rmap.c
+++ b/mm/rmap.c
@@ -324,36 +324,43 @@ int anon_vma_fork(struct vm_area_struct *vma, struct vm_area_struct *pvma)
 	return -ENOMEM;
 }
 
-static void anon_vma_unlink(struct anon_vma_chain *anon_vma_chain)
-{
-	struct anon_vma *anon_vma = anon_vma_chain->anon_vma;
-	int empty;
-
-	/* If anon_vma_fork fails, we can get an empty anon_vma_chain. */
-	if (!anon_vma)
-		return;
-
-	anon_vma_lock(anon_vma);
-	list_del(&anon_vma_chain->same_anon_vma);
-
-	/* We must garbage collect the anon_vma if it's empty */
-	empty = list_empty(&anon_vma->head);
-	anon_vma_unlock(anon_vma);
-
-	if (empty)
-		put_anon_vma(anon_vma);
-}
-
 void unlink_anon_vmas(struct vm_area_struct *vma)
 {
 	struct anon_vma_chain *avc, *next;
+	struct anon_vma *root = NULL;
 
 	/*
 	 * Unlink each anon_vma chained to the VMA.  This list is ordered
 	 * from newest to oldest, ensuring the root anon_vma gets freed last.
 	 */
 	list_for_each_entry_safe(avc, next, &vma->anon_vma_chain, same_vma) {
-		anon_vma_unlink(avc);
+		struct anon_vma *anon_vma = avc->anon_vma;
+
+		root = lock_anon_vma_root(root, anon_vma);
+		list_del(&avc->same_anon_vma);
+
+		/*
+		 * Leave empty anon_vmas on the list - we'll need
+		 * to free them outside the lock.
+		 */
+		if (list_empty(&anon_vma->head))
+			continue;
+
+		list_del(&avc->same_vma);
+		anon_vma_chain_free(avc);
+	}
+	unlock_anon_vma_root(root);
+
+	/*
+	 * Iterate the list once more, it now only contains empty and unlinked
+	 * anon_vmas, destroy them. Could not do before due to __put_anon_vma()
+	 * needing to acquire the anon_vma->root->mutex.
+	 */
+	list_for_each_entry_safe(avc, next, &vma->anon_vma_chain, same_vma) {
+		struct anon_vma *anon_vma = avc->anon_vma;
+
+		put_anon_vma(anon_vma);
+
 		list_del(&avc->same_vma);
 		anon_vma_chain_free(avc);
 	}