Messages in this thread | | | Date | Tue, 7 Jun 2016 19:50:17 +0200 | From | Peter Zijlstra <> | Subject | Re: [PATCH] futex: Calculate the futex key based on a tail page for file-based futexes |
| |
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(); > } >
| |