linux-kernel - Re: [PATCH 20/20] mm: Optimize page_lock_anon

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-Id: <20110419130800.7148a602.akpm@linux-foundation.org>
Date:	Tue, 19 Apr 2011 13:08:00 -0700
From:	Andrew Morton <akpm@...ux-foundation.org>
To:	Peter Zijlstra <a.p.zijlstra@...llo.nl>
Cc:	Andrea Arcangeli <aarcange@...hat.com>,
	Avi Kivity <avi@...hat.com>,
	Thomas Gleixner <tglx@...utronix.de>,
	Rik van Riel <riel@...hat.com>, Ingo Molnar <mingo@...e.hu>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	linux-kernel@...r.kernel.org, linux-arch@...r.kernel.org,
	linux-mm@...ck.org,
	Benjamin Herrenschmidt <benh@...nel.crashing.org>,
	David Miller <davem@...emloft.net>,
	Hugh Dickins <hugh.dickins@...cali.co.uk>,
	Mel Gorman <mel@....ul.ie>, Nick Piggin <npiggin@...nel.dk>,
	Paul McKenney <paulmck@...ux.vnet.ibm.com>,
	Yanmin Zhang <yanmin_zhang@...ux.intel.com>
Subject: Re: [PATCH 20/20] mm: Optimize page_lock_anon_vma() fast-path

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

> Optimize the page_lock_anon_vma() fast path to be one atomic op,
> instead of two.
> 
> Reviewed-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@...fujitsu.com>
> Signed-off-by: Peter Zijlstra <a.p.zijlstra@...llo.nl>
> 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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/