linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] futex: Calculate the futex key based on a tail page for file-based futexes
@ 2016-06-07 12:30 Mel Gorman
  2016-06-07 17:50 ` Peter Zijlstra
  2016-06-08 13:05 ` Davidlohr Bueso
  0 siblings, 2 replies; 6+ messages in thread
From: Mel Gorman @ 2016-06-07 12:30 UTC (permalink / raw)
  To: Thomas Gleixner, Sebastian Andrzej Siewior
  Cc: Mike Galbraith, Davidlohr Bueso, Peter Zijlstra, lkml

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>
---
 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();
 	}
 

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] futex: Calculate the futex key based on a tail page for file-based futexes
  2016-06-07 12:30 [PATCH] futex: Calculate the futex key based on a tail page for file-based futexes Mel Gorman
@ 2016-06-07 17:50 ` Peter Zijlstra
  2016-06-07 17:52   ` Mike Galbraith
  2016-06-08 13:05 ` Davidlohr Bueso
  1 sibling, 1 reply; 6+ messages in thread
From: Peter Zijlstra @ 2016-06-07 17:50 UTC (permalink / raw)
  To: Mel Gorman
  Cc: Thomas Gleixner, Sebastian Andrzej Siewior, Mike Galbraith,
	Davidlohr Bueso, lkml

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();
>  	}
>  

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] futex: Calculate the futex key based on a tail page for file-based futexes
  2016-06-07 17:50 ` Peter Zijlstra
@ 2016-06-07 17:52   ` Mike Galbraith
  2016-06-08  8:04     ` Mel Gorman
  0 siblings, 1 reply; 6+ messages in thread
From: Mike Galbraith @ 2016-06-07 17:52 UTC (permalink / raw)
  To: Peter Zijlstra, Mel Gorman
  Cc: Thomas Gleixner, Sebastian Andrzej Siewior, Davidlohr Bueso, lkml

On Tue, 2016-06-07 at 19:50 +0200, Peter Zijlstra wrote:
> On Tue, Jun 07, 2016 at 01:30:17PM +0100, Mel Gorman wrote:
> > Signed-off-by: Mel Gorman <mgorman@techsingularity.net>
> 
> Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>

Wants a stable tag so it gets auto-snarfed by Greg?

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] futex: Calculate the futex key based on a tail page for file-based futexes
  2016-06-07 17:52   ` Mike Galbraith
@ 2016-06-08  8:04     ` Mel Gorman
  2016-06-08  8:42       ` Mike Galbraith
  0 siblings, 1 reply; 6+ messages in thread
From: Mel Gorman @ 2016-06-08  8:04 UTC (permalink / raw)
  To: Mike Galbraith
  Cc: Peter Zijlstra, Thomas Gleixner, Sebastian Andrzej Siewior,
	Davidlohr Bueso, lkml

On Tue, Jun 07, 2016 at 07:52:59PM +0200, Mike Galbraith wrote:
> On Tue, 2016-06-07 at 19:50 +0200, Peter Zijlstra wrote:
> > On Tue, Jun 07, 2016 at 01:30:17PM +0100, Mel Gorman wrote:
> > > Signed-off-by: Mel Gorman <mgorman@techsingularity.net>
> > 
> > Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> 
> Wants a stable tag so it gets auto-snarfed by Greg?

Can I assume it passed tests for you?

If so, I'll resend with any acks and a "# 4.6+" stable tag to Thomas
assuming he does not object to the patch in the meantime.

-- 
Mel Gorman
SUSE Labs

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] futex: Calculate the futex key based on a tail page for file-based futexes
  2016-06-08  8:04     ` Mel Gorman
@ 2016-06-08  8:42       ` Mike Galbraith
  0 siblings, 0 replies; 6+ messages in thread
From: Mike Galbraith @ 2016-06-08  8:42 UTC (permalink / raw)
  To: Mel Gorman
  Cc: Peter Zijlstra, Thomas Gleixner, Sebastian Andrzej Siewior,
	Davidlohr Bueso, lkml

On Wed, 2016-06-08 at 09:04 +0100, Mel Gorman wrote:
> On Tue, Jun 07, 2016 at 07:52:59PM +0200, Mike Galbraith wrote:
> > On Tue, 2016-06-07 at 19:50 +0200, Peter Zijlstra wrote:
> > > On Tue, Jun 07, 2016 at 01:30:17PM +0100, Mel Gorman wrote:
> > > > Signed-off-by: Mel Gorman <mgorman@techsingularity.net>
> > > 
> > > Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> > 
> > Wants a stable tag so it gets auto-snarfed by Greg?
> 
> Can I assume it passed tests for you?

While I didn't test this specific patch, modulo 'tail' vs 'pinned',
it's identical to the three liner I tested to confirm that the
get/put_page() bits I added were in indeed superfluous.. so yeah, it
definitely squashes buglet.

	-Mike

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] futex: Calculate the futex key based on a tail page for file-based futexes
  2016-06-07 12:30 [PATCH] futex: Calculate the futex key based on a tail page for file-based futexes Mel Gorman
  2016-06-07 17:50 ` Peter Zijlstra
@ 2016-06-08 13:05 ` Davidlohr Bueso
  1 sibling, 0 replies; 6+ messages in thread
From: Davidlohr Bueso @ 2016-06-08 13:05 UTC (permalink / raw)
  To: Mel Gorman
  Cc: Thomas Gleixner, Sebastian Andrzej Siewior, Mike Galbraith,
	Davidlohr Bueso, Peter Zijlstra, lkml

On Tue, 07 Jun 2016, 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>

Reviewed-by: Davidlohr Bueso <dave@stgolabs.net>

(and yay for maintaining performance)

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2016-06-08 13:05 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-06-07 12:30 [PATCH] futex: Calculate the futex key based on a tail page for file-based futexes Mel Gorman
2016-06-07 17:50 ` Peter Zijlstra
2016-06-07 17:52   ` Mike Galbraith
2016-06-08  8:04     ` Mel Gorman
2016-06-08  8:42       ` Mike Galbraith
2016-06-08 13:05 ` Davidlohr Bueso

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).