Re: [PATCH 20/20] mm: Optimize page_lock_anon_vma() fast-path

[Andrew Morton]

On Fri, 01 Apr 2011 14:13:18 +0200
Peter Zijlstra <a.p.zijlstra@xxxxxxxxx> wrote:

> Optimize the page_lock_anon_vma() fast path to be one atomic op,
> instead of two.
> 
> Reviewed-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@xxxxxxxxxxxxxx>
> Signed-off-by: Peter Zijlstra <a.p.zijlstra@xxxxxxxxx>
> LKML-Reference: <new-submission>
> ---
>  mm/rmap.c |   86 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 82 insertions(+), 4 deletions(-)
> 
> Index: linux-2.6/mm/rmap.c
> ===================================================================
> --- linux-2.6.orig/mm/rmap.c
> +++ linux-2.6/mm/rmap.c
> @@ -85,6 +85,29 @@ static inline struct anon_vma *anon_vma_
>  static inline void anon_vma_free(struct anon_vma *anon_vma)
>  {
>  	VM_BUG_ON(atomic_read(&anon_vma->refcount));
> +
> +	/*
> +	 * Synchronize against page_lock_anon_vma() such that
> +	 * we can safely hold the lock without the anon_vma getting
> +	 * freed.
> +	 *
> +	 * Relies on the full mb implied by the atomic_dec_and_test() from
> +	 * put_anon_vma() against the acquire barrier implied by
> +	 * mutex_trylock() from page_lock_anon_vma(). This orders:
> +	 *
> +	 * page_lock_anon_vma()		VS	put_anon_vma()
> +	 *   mutex_trylock()			  atomic_dec_and_test()
> +	 *   LOCK				  MB
> +	 *   atomic_read()			  mutex_is_locked()
> +	 *
> +	 * LOCK should suffice since the actual taking of the lock must
> +	 * happen _before_ what follows.
> +	 */
> +	if (mutex_is_locked(&anon_vma->root->mutex)) {
> +		anon_vma_lock(anon_vma);
> +		anon_vma_unlock(anon_vma);
> +	}
> +
>  	kmem_cache_free(anon_vma_cachep, anon_vma);
>  }

Did we need to include all this stuff in uniprocessor builds?

It would be neater to add a new anon_vma_is_locked().

This code is too tricksy to deserve life :(

> @@ -371,20 +394,75 @@ struct anon_vma *page_get_anon_vma(struc
>  	return anon_vma;
>  }
>  
> +/*
> + * Similar to page_get_anon_vma() except it locks the anon_vma.
> + *
> + * Its a little more complex as it tries to keep the fast path to a single
> + * atomic op -- the trylock. If we fail the trylock, we fall back to getting a
> + * reference like with page_get_anon_vma() and then block on the mutex.
> + */
>  struct anon_vma *page_lock_anon_vma(struct page *page)
>  {
> -	struct anon_vma *anon_vma = page_get_anon_vma(page);
> +	struct anon_vma *anon_vma = NULL;
> +	unsigned long anon_mapping;
>  
> -	if (anon_vma)
> -		anon_vma_lock(anon_vma);
> +	rcu_read_lock();
> +	anon_mapping = (unsigned long) ACCESS_ONCE(page->mapping);
> +	if ((anon_mapping & PAGE_MAPPING_FLAGS) != PAGE_MAPPING_ANON)
> +		goto out;

Why?  Needs a comment.

> +	if (!page_mapped(page))
> +		goto out;

Why?  How can this come about? Needs a comment.

> +
> +	anon_vma = (struct anon_vma *) (anon_mapping - PAGE_MAPPING_ANON);
> +	if (mutex_trylock(&anon_vma->root->mutex)) {

anon_vma_trylock()?

Or just remove all the wrapper functions and open-code all the locking.
These tricks all seem pretty tied-up with the mutex implementation
anyway.

> +		/*
> +		 * If we observe a !0 refcount, then holding the lock ensures
> +		 * the anon_vma will not go away, see __put_anon_vma().
> +		 */
> +		if (!atomic_read(&anon_vma->refcount)) {
> +			anon_vma_unlock(anon_vma);
> +			anon_vma = NULL;
> +		}
> +		goto out;
> +	}
> +
> +	/* trylock failed, we got to sleep */
> +	if (!atomic_inc_not_zero(&anon_vma->refcount)) {
> +		anon_vma = NULL;
> +		goto out;
> +	}
>  
> +	if (!page_mapped(page)) {
> +		put_anon_vma(anon_vma);
> +		anon_vma = NULL;
> +		goto out;
> +	}

Also quite opaque, needs decent commentary.

I'd have expected this test to occur after the lock was acquired.

> +	/* we pinned the anon_vma, its safe to sleep */
> +	rcu_read_unlock();
> +	anon_vma_lock(anon_vma);
> +
> +	if (atomic_dec_and_test(&anon_vma->refcount)) {
> +		/*
> +		 * Oops, we held the last refcount, release the lock
> +		 * and bail -- can't simply use put_anon_vma() because
> +		 * we'll deadlock on the anon_vma_lock() recursion.
> +		 */
> +		anon_vma_unlock(anon_vma);
> +		__put_anon_vma(anon_vma);
> +		anon_vma = NULL;
> +	}
> +
> +	return anon_vma;
> +
> +out:
> +	rcu_read_unlock();
>  	return anon_vma;
>  }
>  
>  void page_unlock_anon_vma(struct anon_vma *anon_vma)
>  {
>  	anon_vma_unlock(anon_vma);
> -	put_anon_vma(anon_vma);
>  }

Geeze, I hope this patch is worth it :( :(
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/