linux-kernel - Re: [PATCHv4 14/39] thp, mm: rewrite delete_from_page

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]

Message-ID: <519BD595.5040405@sr71.net>
Date:	Tue, 21 May 2013 13:14:13 -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 14/39] thp, mm: rewrite delete_from_page_cache() to
 support huge pages

On 05/11/2013 06:23 PM, Kirill A. Shutemov wrote:
> From: "Kirill A. Shutemov" <kirill.shutemov@...ux.intel.com>
> 
> As with add_to_page_cache_locked() we handle HPAGE_CACHE_NR pages a
> time.
> 
> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@...ux.intel.com>
> ---
>  mm/filemap.c |   31 +++++++++++++++++++++++++------
>  1 file changed, 25 insertions(+), 6 deletions(-)
> 
> diff --git a/mm/filemap.c b/mm/filemap.c
> index b0c7c8c..657ce82 100644
> --- a/mm/filemap.c
> +++ b/mm/filemap.c
> @@ -115,6 +115,9 @@
>  void __delete_from_page_cache(struct page *page)
>  {
>  	struct address_space *mapping = page->mapping;
> +	bool thp = PageTransHuge(page) &&
> +		IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE_PAGECACHE);
> +	int nr;

Is that check for the config option really necessary?  How would we get
a page with PageTransHuge() set without it being enabled?

>  	trace_mm_filemap_delete_from_page_cache(page);
>  	/*
> @@ -127,13 +130,29 @@ void __delete_from_page_cache(struct page *page)
>  	else
>  		cleancache_invalidate_page(mapping, page);
>  
> -	radix_tree_delete(&mapping->page_tree, page->index);
> +	if (thp) {
> +		int i;
> +
> +		nr = HPAGE_CACHE_NR;
> +		radix_tree_delete(&mapping->page_tree, page->index);
> +		for (i = 1; i < HPAGE_CACHE_NR; i++) {
> +			radix_tree_delete(&mapping->page_tree, page->index + i);
> +			page[i].mapping = NULL;
> +			page_cache_release(page + i);
> +		}
> +		__dec_zone_page_state(page, NR_FILE_TRANSPARENT_HUGEPAGES);
> +	} else {
> +		BUG_ON(PageTransHuge(page));
> +		nr = 1;
> +		radix_tree_delete(&mapping->page_tree, page->index);
> +	}
>  	page->mapping = NULL;

I like to rewrite your code. :)

	nr = hpage_nr_pages(page);
	for (i = 0; i < nr; i++) {
		page[i].mapping = NULL;
		radix_tree_delete(&mapping->page_tree, page->index + i);
		/* tail pages: */
		if (i)
			page_cache_release(page + i);
	}
	if (thp)
	     __dec_zone_page_state(page, NR_FILE_TRANSPARENT_HUGEPAGES);

I like this because it explicitly calls out the logic that tail pages
are different from head pages.  We handle their reference counts
differently.

Which reminds me...  Why do we handle their reference counts differently? :)

It seems like we could easily put a for loop in delete_from_page_cache()
that will release their reference counts along with the head page.
Wouldn't that make the code less special-cased for tail pages?

>  	/* Leave page->index set: truncation lookup relies upon it */
> -	mapping->nrpages--;
> -	__dec_zone_page_state(page, NR_FILE_PAGES);
> +	mapping->nrpages -= nr;
> +	__mod_zone_page_state(page_zone(page), NR_FILE_PAGES, -nr);
>  	if (PageSwapBacked(page))
> -		__dec_zone_page_state(page, NR_SHMEM);
> +		__mod_zone_page_state(page_zone(page), NR_SHMEM, -nr);
>  	BUG_ON(page_mapped(page));

Man, we suck:

	__dec_zone_page_state()
and
	__mod_zone_page_state()

take a differently-typed first argument.  <sigh>

Would there be any good to making __dec_zone_page_state() check to see
if the page we passed in _is_ a compound page, and adjusting its
behaviour accordingly?

>  	/*
> @@ -144,8 +163,8 @@ void __delete_from_page_cache(struct page *page)
>  	 * having removed the page entirely.
>  	 */
>  	if (PageDirty(page) && mapping_cap_account_dirty(mapping)) {
> -		dec_zone_page_state(page, NR_FILE_DIRTY);
> -		dec_bdi_stat(mapping->backing_dev_info, BDI_RECLAIMABLE);
> +		mod_zone_page_state(page_zone(page), NR_FILE_DIRTY, -nr);
> +		add_bdi_stat(mapping->backing_dev_info, BDI_RECLAIMABLE, -nr);
>  	}
>  }

Ahh, I see now why you didn't need a dec_bdi_stat().  Oh well...

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