Re: [PATCH] futex: Calculate the futex key based on a tail page for file-based futexes

[Peter Zijlstra <peterz@infradead.org>]

On Tue, Jun 07, 2016 at 01:30:17PM +0100, Mel Gorman wrote:
> Mike Galbraith reported that the LTP test case futex_wake04 was broken
> by commit 65d8fc777f6d ("futex: Remove requirement for lock_page()
> in get_futex_key()").
> 
> This test case uses futexes backed by hugetlbfs pages and so there is an
> associated inode with a futex stored on such pages. The problem is that
> the key is being calculated based on the head page index of the hugetlbfs
> page and not the tail page.
> 
> Prior to the optimisation, the page lock was used to stabilise mappings and
> pin the inode is file-backed which is overkill. If the page was a compound
> page, the head page was automatically looked up as part of the page lock
> operation but the tail page index was used to calculate the futex key.
> 
> After the optimisation, the compound head is looked up early and the page
> lock is only relied upon to identify truncated pages, special pages or a
> shmem page moving to swapcache. The head page is looked up because without
> the page lock, special care has to be taken to pin the inode correctly.
> However, the tail page is still required to calculate the futex key so this
> patch records the tail page.
> 
> On vanilla 4.6, the output of the test case is;
> 
> futex_wake04    0  TINFO  :  Hugepagesize 2097152
> futex_wake04    1  TFAIL  :  futex_wake04.c:126: Bug: wait_thread2 did not wake after 30 secs.
> 
> With the patch applied
> 
> futex_wake04    0  TINFO  :  Hugepagesize 2097152
> futex_wake04    1  TPASS  :  Hi hydra, thread2 awake!
> 
> Reported-by: Mike Galbraith <umgwanakikbuti@gmail.com>
> Signed-off-by: Mel Gorman <mgorman@techsingularity.net>

Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>

> ---
>  kernel/futex.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/futex.c b/kernel/futex.c
> index c20f06f38ef3..6555d5459e98 100644
> --- a/kernel/futex.c
> +++ b/kernel/futex.c
> @@ -469,7 +469,7 @@ get_futex_key(u32 __user *uaddr, int fshared, union futex_key *key, int rw)
>  {
>  	unsigned long address = (unsigned long)uaddr;
>  	struct mm_struct *mm = current->mm;
> -	struct page *page;
> +	struct page *page, *tail;
>  	struct address_space *mapping;
>  	int err, ro = 0;
>  
> @@ -530,7 +530,15 @@ get_futex_key(u32 __user *uaddr, int fshared, union futex_key *key, int rw)
>  	 * considered here and page lock forces unnecessarily serialization
>  	 * From this point on, mapping will be re-verified if necessary and
>  	 * page lock will be acquired only if it is unavoidable
> -	 */
> +	 *
> +	 * Mapping checks require the head page for any compound page so the
> +	 * head page and mapping is looked up now. For anonymous pages, it
> +	 * does not matter if the page splits in the future as the key is
> +	 * based on the address. For filesystem-backed pages, the tail is
> +	 * required as the index of the page determines the key. For
> +	 * base pages, there is no tail page and tail == page.
> +	 */
> +	tail = page;
>  	page = compound_head(page);
>  	mapping = READ_ONCE(page->mapping);
>  
> @@ -654,7 +662,7 @@ get_futex_key(u32 __user *uaddr, int fshared, union futex_key *key, int rw)
>  
>  		key->both.offset |= FUT_OFF_INODE; /* inode-based key */
>  		key->shared.inode = inode;
> -		key->shared.pgoff = basepage_index(page);
> +		key->shared.pgoff = basepage_index(tail);
>  		rcu_read_unlock();
>  	}
>