linux-kernel - Re: [PATCHv4 24/39] thp, mm: truncate support for transparent huge page cache

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:	Tue, 21 May 2013 15:39:18 -0700
From:	Dave Hansen <dave@...1.net>
To:	"Kirill A. Shutemov" <kirill.shutemov@...ux.intel.com>
CC:	Andrea Arcangeli <aarcange@...hat.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Al Viro <viro@...iv.linux.org.uk>,
	Hugh Dickins <hughd@...gle.com>,
	Wu Fengguang <fengguang.wu@...el.com>, Jan Kara <jack@...e.cz>,
	Mel Gorman <mgorman@...e.de>, linux-mm@...ck.org,
	Andi Kleen <ak@...ux.intel.com>,
	Matthew Wilcox <matthew.r.wilcox@...el.com>,
	"Kirill A. Shutemov" <kirill@...temov.name>,
	Hillf Danton <dhillf@...il.com>, linux-fsdevel@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCHv4 24/39] thp, mm: truncate support for transparent huge
 page cache

On 05/11/2013 06:23 PM, Kirill A. Shutemov wrote:
> If we starting position of truncation is in tail page we have to spilit
> the huge page page first.

That's a very interesting sentence sentence. :)

> We also have to split if end is within the huge page. Otherwise we can
> truncate whole huge page at once.

How about something more like this as a description?

Splitting a huge page is relatively expensive.  If at all possible, we
would like to do truncation without first splitting a page.  However, if
the truncation request starts or ends in the middle of a huge page, we
have no choice and must split it.

> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@...ux.intel.com>
> ---
>  mm/truncate.c |   13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/mm/truncate.c b/mm/truncate.c
> index c75b736..0152feb 100644
> --- a/mm/truncate.c
> +++ b/mm/truncate.c
> @@ -231,6 +231,17 @@ void truncate_inode_pages_range(struct address_space *mapping,
>  			if (index > end)
>  				break;
>  
> +			/* split page if we start from tail page */
> +			if (PageTransTail(page))
> +				split_huge_page(compound_trans_head(page));

I know it makes no logical difference, but should this be an "else if"?
 It would make it more clear to me that PageTransTail() and
PageTransHead() are mutually exclusive.

> +			if (PageTransHuge(page)) {
> +				/* split if end is within huge page */
> +				if (index == (end & ~HPAGE_CACHE_INDEX_MASK))

How about:

	if ((end - index) > HPAGE_CACHE_NR)

That seems a bit more straightforward, to me at least.

> +					split_huge_page(page);
> +				else
> +					/* skip tail pages */
> +					i += HPAGE_CACHE_NR - 1;
> +			}

Hmm..  This is all inside a loop, right?

                for (i = 0; i < pagevec_count(&pvec); i++) {
                        struct page *page = pvec.pages[i];

PAGEVEC_SIZE is only 14 here, so it seems a bit odd to be incrementing i
by 512-1.  We'll break out of the pagevec loop, but won't 'index' be set
to the wrong thing on the next iteration of the loop?  Did you want to
be incrementing 'index' instead of 'i'?

This is also another case where I wonder about racing split_huge_page()
operations.

>  			if (!trylock_page(page))
>  				continue;
>  			WARN_ON(page->index != index);
> @@ -280,6 +291,8 @@ void truncate_inode_pages_range(struct address_space *mapping,
>  			if (index > end)
>  				break;
>  
> +			if (PageTransHuge(page))
> +				split_huge_page(page);
>  			lock_page(page);
>  			WARN_ON(page->index != index);
>  			wait_on_page_writeback(page);

This seems to imply that we would have taken care of the case where we
encountered a tail page in the first pass.  Should we put a comment in
to explain that assumption?

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