RT trees fail ltp futex_wait04 - culprit=65d8fc777f6d (futex: Remove requirement for lock_page() in get_futex_key())

RT trees fail ltp futex_wait04 - culprit=65d8fc777f6d (futex: Remove requirement for lock_page() in get_futex_key())

[Mike Galbraith]

Hi Mel,

Doing some RT tree consolidation this weekend to see what I should try
to upstream, I found that 4.[67]-rt trees fail ltp futex_wait04, and
tracked it down to commit in $subject.

Looking at that commit, I recalled it being in SLE, and SLERT working
just fine, so I went to see what a futex.c diff looked like.  Taking
what was left after tossing THP bits (irrelevant to RT) and applying
that to my RT trees fixes the testcase failure.

Snarfed-and-munged-by: Mike Galbraith <umgwanakikbuti@gmail.com>
---
 kernel/futex.c |   39 ++++++++++++++++++++++-----------------
 1 file changed, 22 insertions(+), 17 deletions(-)

--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -469,7 +469,7 @@ get_futex_key(u32 __user *uaddr, int fsh
 {
 	unsigned long address = (unsigned long)uaddr;
 	struct mm_struct *mm = current->mm;
-	struct page *page;
+	struct page *page, *page_head;
 	struct address_space *mapping;
 	int err, ro = 0;
 
@@ -520,6 +520,12 @@ get_futex_key(u32 __user *uaddr, int fsh
 	else
 		err = 0;
 
+	page_head = compound_head(page);
+	if (page != page_head) {
+		get_page(page_head);
+		put_page(page);
+	}
+
 	/*
 	 * The treatment of mapping from this point on is critical. The page
 	 * lock protects many things but in this context the page lock
@@ -531,11 +537,10 @@ get_futex_key(u32 __user *uaddr, int fsh
 	 * From this point on, mapping will be re-verified if necessary and
 	 * page lock will be acquired only if it is unavoidable
 	 */
-	page = compound_head(page);
-	mapping = READ_ONCE(page->mapping);
+	mapping = READ_ONCE(page_head->mapping);
 
 	/*
-	 * If page->mapping is NULL, then it cannot be a PageAnon
+	 * If mapping is NULL, then it cannot be a PageAnon
 	 * page; but it might be the ZERO_PAGE or in the gate area or
 	 * in a special mapping (all cases which we are happy to fail);
 	 * or it may have been a good file page when get_user_pages_fast
@@ -547,7 +552,7 @@ get_futex_key(u32 __user *uaddr, int fsh
 	 *
 	 * The case we do have to guard against is when memory pressure made
 	 * shmem_writepage move it from filecache to swapcache beneath us:
-	 * an unlikely race, but we do need to retry for page->mapping.
+	 * an unlikely race, but we do need to retry for page_head->mapping.
 	 */
 	if (unlikely(!mapping)) {
 		int shmem_swizzled;
@@ -557,10 +562,10 @@ get_futex_key(u32 __user *uaddr, int fsh
 		 * applies. If this is really a shmem page then the page lock
 		 * will prevent unexpected transitions.
 		 */
-		lock_page(page);
-		shmem_swizzled = PageSwapCache(page) || page->mapping;
-		unlock_page(page);
-		put_page(page);
+		lock_page(page_head);
+		shmem_swizzled = PageSwapCache(page_head) || page_head->mapping;
+		unlock_page(page_head);
+		put_page(page_head);
 
 		if (shmem_swizzled)
 			goto again;
@@ -578,7 +583,7 @@ get_futex_key(u32 __user *uaddr, int fsh
 	 * it's a read-only handle, it's expected that futexes attach to
 	 * the object not the particular process.
 	 */
-	if (PageAnon(page)) {
+	if (PageAnon(page_head)) {
 		/*
 		 * A RO anonymous page will never change and thus doesn't make
 		 * sense for futex operations.
@@ -599,8 +604,8 @@ get_futex_key(u32 __user *uaddr, int fsh
 
 		/*
 		 * The associated futex object in this case is the inode and
-		 * the page->mapping must be traversed. Ordinarily this should
-		 * be stabilised under page lock but it's not strictly
+		 * the page_head->mapping must be traversed. Ordinarily this
+		 * should be stabilised under page lock but it's not strictly
 		 * necessary in this case as we just want to pin the inode, not
 		 * update the radix tree or anything like that.
 		 *
@@ -610,9 +615,9 @@ get_futex_key(u32 __user *uaddr, int fsh
 		 */
 		rcu_read_lock();
 
-		if (READ_ONCE(page->mapping) != mapping) {
+		if (READ_ONCE(page_head->mapping) != mapping) {
 			rcu_read_unlock();
-			put_page(page);
+			put_page(page_head);
 
 			goto again;
 		}
@@ -620,7 +625,7 @@ get_futex_key(u32 __user *uaddr, int fsh
 		inode = READ_ONCE(mapping->host);
 		if (!inode) {
 			rcu_read_unlock();
-			put_page(page);
+			put_page(page_head);
 
 			goto again;
 		}
@@ -638,7 +643,7 @@ get_futex_key(u32 __user *uaddr, int fsh
 		 */
 		if (WARN_ON_ONCE(!atomic_inc_not_zero(&inode->i_count))) {
 			rcu_read_unlock();
-			put_page(page);
+			put_page(page_head);
 
 			goto again;
 		}
@@ -659,7 +664,7 @@ get_futex_key(u32 __user *uaddr, int fsh
 	}
 
 out:
-	put_page(page);
+	put_page(page_head);
 	return err;
 }
 

Re: RT trees fail ltp futex_wait04 - culprit=65d8fc777f6d (futex: Remove requirement for lock_page() in get_futex_key())

[Mike Galbraith]

Oho, actually, it's not only RT kernels, master with an enterprise like
NOPREEMPT config fails as well, so it's a garden variety regression.

futex_wake04    1  TFAIL  :  futex_wake04.c:126: Bug: wait_thread2 did not wake after 30 secs.
futex_wait_bitset01    1  TPASS  :  futex_wait_bitset() waited 100068us, expected 100010us
futex_wait_bitset02    1  TPASS  :  futex_wait_bitset() waited 100068us, expected 100010us
INFO: ltp-pan reported some tests FAIL

	-Mike

Re: RT trees fail ltp futex_wait04 - culprit=65d8fc777f6d (futex: Remove requirement for lock_page() in get_futex_key())

[Mike Galbraith]

On Sat, 2016-06-04 at 18:07 +0200, Mike Galbraith wrote:
> Oho, actually, it's not only RT kernels, master with an enterprise like
> NOPREEMPT config fails as well, so it's a garden variety regression.
> 
> futex_wake04    1  TFAIL  :  futex_wake04.c:126: Bug: wait_thread2 did not wake after 30 secs.

And again, swiped bits fixed it.

Re: RT trees fail ltp futex_wait04 - culprit=65d8fc777f6d (futex: Remove requirement for lock_page() in get_futex_key())

[Sebastian Andrzej Siewior]

- Mel, tglx
* Mike Galbraith | 2016-06-04 18:16:59 [+0200]:

>On Sat, 2016-06-04 at 18:07 +0200, Mike Galbraith wrote:
>> Oho, actually, it's not only RT kernels, master with an enterprise like
>> NOPREEMPT config fails as well, so it's a garden variety regression.
>> 
>> futex_wake04    1  TFAIL  :  futex_wake04.c:126: Bug: wait_thread2 did not wake after 30 secs.
>
>And again, swiped bits fixed it.

for reference, this thread ends here:
  http://git.kernel.org/tip/077fa7aed17de5022e44bf07dbaf732078b7b5b2

Sebastian