linux-kernel - Re: [PATCHv5 14/28] futex, thp: remove special case for THP in get_futex

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]

Date:	Mon, 18 May 2015 13:49:39 +0200
From:	Vlastimil Babka <vbabka@...e.cz>
To:	"Kirill A. Shutemov" <kirill.shutemov@...ux.intel.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Andrea Arcangeli <aarcange@...hat.com>,
	Hugh Dickins <hughd@...gle.com>
CC:	Dave Hansen <dave.hansen@...el.com>, Mel Gorman <mgorman@...e.de>,
	Rik van Riel <riel@...hat.com>,
	Christoph Lameter <cl@...two.org>,
	Naoya Horiguchi <n-horiguchi@...jp.nec.com>,
	Steve Capper <steve.capper@...aro.org>,
	"Aneesh Kumar K.V" <aneesh.kumar@...ux.vnet.ibm.com>,
	Johannes Weiner <hannes@...xchg.org>,
	Michal Hocko <mhocko@...e.cz>,
	Jerome Marchand <jmarchan@...hat.com>,
	Sasha Levin <sasha.levin@...cle.com>,
	linux-kernel@...r.kernel.org, linux-mm@...ck.org
Subject: Re: [PATCHv5 14/28] futex, thp: remove special case for THP in get_futex_key

On 04/23/2015 11:03 PM, Kirill A. Shutemov wrote:
> With new THP refcounting, we don't need tricks to stabilize huge page.
> If we've got reference to tail page, it can't split under us.
>
> This patch effectively reverts a5b338f2b0b1.
>
> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@...ux.intel.com>
> Tested-by: Sasha Levin <sasha.levin@...cle.com>
> ---
>   kernel/futex.c | 61 ++++++++++++----------------------------------------------
>   1 file changed, 12 insertions(+), 49 deletions(-)
>
> diff --git a/kernel/futex.c b/kernel/futex.c
> index f4d8a85641ed..cf0192e60ef9 100644
> --- a/kernel/futex.c
> +++ b/kernel/futex.c
> @@ -399,7 +399,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, *page_head;
> +	struct page *page;
>   	int err, ro = 0;
>
>   	/*
> @@ -442,46 +442,9 @@ again:
>   	else
>   		err = 0;
>
> -#ifdef CONFIG_TRANSPARENT_HUGEPAGE
> -	page_head = page;
> -	if (unlikely(PageTail(page))) {
> -		put_page(page);
> -		/* serialize against __split_huge_page_splitting() */
> -		local_irq_disable();
> -		if (likely(__get_user_pages_fast(address, 1, !ro, &page) == 1)) {
> -			page_head = compound_head(page);
> -			/*
> -			 * page_head is valid pointer but we must pin
> -			 * it before taking the PG_lock and/or
> -			 * PG_compound_lock. The moment we re-enable
> -			 * irqs __split_huge_page_splitting() can
> -			 * return and the head page can be freed from
> -			 * under us. We can't take the PG_lock and/or
> -			 * PG_compound_lock on a page that could be
> -			 * freed from under us.
> -			 */
> -			if (page != page_head) {
> -				get_page(page_head);
> -				put_page(page);
> -			}
> -			local_irq_enable();
> -		} else {
> -			local_irq_enable();
> -			goto again;
> -		}
> -	}
> -#else
> -	page_head = compound_head(page);
> -	if (page != page_head) {
> -		get_page(page_head);
> -		put_page(page);
> -	}

Hmm, any idea why this was there? Without THP, it was already sure that 
get/put_page() on tail page operates on the head page's _count, no?

> -#endif
> -
> -	lock_page(page_head);
> -
> +	lock_page(page);
>   	/*
> -	 * If page_head->mapping is NULL, then it cannot be a PageAnon
> +	 * If page->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
> @@ -493,12 +456,12 @@ again:
>   	 *
>   	 * 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_head->mapping.
> +	 * an unlikely race, but we do need to retry for page->mapping.
>   	 */
> -	if (!page_head->mapping) {
> -		int shmem_swizzled = PageSwapCache(page_head);
> -		unlock_page(page_head);
> -		put_page(page_head);
> +	if (!page->mapping) {
> +		int shmem_swizzled = PageSwapCache(page);
> +		unlock_page(page);
> +		put_page(page);
>   		if (shmem_swizzled)
>   			goto again;
>   		return -EFAULT;
> @@ -511,7 +474,7 @@ again:
>   	 * it's a read-only handle, it's expected that futexes attach to
>   	 * the object not the particular process.
>   	 */
> -	if (PageAnon(page_head)) {
> +	if (PageAnon(page)) {
>   		/*
>   		 * A RO anonymous page will never change and thus doesn't make
>   		 * sense for futex operations.
> @@ -526,15 +489,15 @@ again:
>   		key->private.address = address;
>   	} else {
>   		key->both.offset |= FUT_OFF_INODE; /* inode-based key */
> -		key->shared.inode = page_head->mapping->host;
> +		key->shared.inode = page->mapping->host;
>   		key->shared.pgoff = basepage_index(page);
>   	}
>
>   	get_futex_key_refs(key); /* implies MB (B) */
>
>   out:
> -	unlock_page(page_head);
> -	put_page(page_head);
> +	unlock_page(page);
> +	put_page(page);
>   	return err;
>   }
>
>

--
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/