linux-kernel - Re: [PATCH v4 2/7] deactivate invalidated pages

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: <20101207151939.GF2356@cmpxchg.org>
Date:	Tue, 7 Dec 2010 16:19:39 +0100
From:	Johannes Weiner <hannes@...xchg.org>
To:	Minchan Kim <minchan.kim@...il.com>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	Rik van Riel <riel@...hat.com>,
	KOSAKI Motohiro <kosaki.motohiro@...fujitsu.com>,
	linux-mm <linux-mm@...ck.org>,
	LKML <linux-kernel@...r.kernel.org>,
	Peter Zijlstra <peterz@...radead.org>,
	Wu Fengguang <fengguang.wu@...el.com>,
	Nick Piggin <npiggin@...nel.dk>, Mel Gorman <mel@....ul.ie>
Subject: Re: [PATCH v4 2/7] deactivate invalidated pages

On Wed, Dec 08, 2010 at 12:07:10AM +0900, Minchan Kim wrote:
> On Tue, Dec 07, 2010 at 03:49:24PM +0100, Johannes Weiner wrote:
> > On Mon, Dec 06, 2010 at 02:29:10AM +0900, Minchan Kim wrote:
> > > Changelog since v3:
> > >  - Change function comments - suggested by Johannes
> > >  - Change function name - suggested by Johannes
> > >  - add only dirty/writeback pages to deactive pagevec
> > 
> > Why the extra check?
> > 
> > > @@ -359,8 +360,16 @@ unsigned long invalidate_mapping_pages(struct address_space *mapping,
> > >  			if (lock_failed)
> > >  				continue;
> > >  
> > > -			ret += invalidate_inode_page(page);
> > > -
> > > +			ret = invalidate_inode_page(page);
> > > +			/*
> > > +			 * If the page is dirty or under writeback, we can not
> > > +			 * invalidate it now.  But we assume that attempted
> > > +			 * invalidation is a hint that the page is no longer
> > > +			 * of interest and try to speed up its reclaim.
> > > +			 */
> > > +			if (!ret && (PageDirty(page) || PageWriteback(page)))
> > > +				deactivate_page(page);
> > 
> > The writeback completion handler does not take the page lock, so you
> > can still miss pages that finish writeback before this test, no?
> 
> Yes. but I think it's rare and even though it happens, it's not critical.
> > 
> > Can you explain why you felt the need to add these checks?
> 
> invalidate_inode_page can return 0 although the pages is !{dirty|writeback}.
> Look invalidate_complete_page. As easiest example, if the page has buffer and
> try_to_release_page can't release the buffer, it could return 0.

Ok, but somebody still tried to truncate the page, so why shouldn't we
try to reclaim it?  The reason for deactivating at this location is
that truncation is a strong hint for reclaim, not that it failed due
to dirty/writeback pages.

What's the problem with deactivating pages where try_to_release_page()
failed?

I don't think we should add more logic than necessary.  If there is a
good reason for it, it needs to get a code comment at least.
--
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/