From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754224Ab0LGPHZ (ORCPT ); Tue, 7 Dec 2010 10:07:25 -0500 Received: from mail-pw0-f46.google.com ([209.85.160.46]:64718 "EHLO mail-pw0-f46.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751630Ab0LGPHX (ORCPT ); Tue, 7 Dec 2010 10:07:23 -0500 DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=date:from:to:cc:subject:message-id:references:mime-version :content-type:content-disposition:in-reply-to:user-agent; b=DBaZhIEtTMNre6wA8OckSmUHJxI8EriHG5OQSBDNFHlF4bn1kmCwJ8HiWarjR5b/Pt 1RSUzJWs6vAWTYj/NxaJWokEwxz1GBAx36gOG6eHr4H6rbIsTR+gNlz5o/JiKOB1gmmy RNOnxCi/pyVnyp/eOuFuY6D4lXCYg+iBffhMc= Date: Wed, 8 Dec 2010 00:07:10 +0900 From: Minchan Kim To: Johannes Weiner Cc: Andrew Morton , Rik van Riel , KOSAKI Motohiro , linux-mm , LKML , Peter Zijlstra , Wu Fengguang , Nick Piggin , Mel Gorman Subject: Re: [PATCH v4 2/7] deactivate invalidated pages Message-ID: <20101207150710.GA26613@barrios-desktop> References: <20101207144923.GB2356@cmpxchg.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20101207144923.GB2356@cmpxchg.org> User-Agent: Mutt/1.5.20 (2009-06-14) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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. I want to check this. > > Thanks! > > Hannes -- Kind regards, Minchan Kim From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail143.messagelabs.com (mail143.messagelabs.com [216.82.254.35]) by kanga.kvack.org (Postfix) with SMTP id 4160F6B008C for ; Tue, 7 Dec 2010 10:07:28 -0500 (EST) Received: by pwi6 with SMTP id 6so19745pwi.14 for ; Tue, 07 Dec 2010 07:07:23 -0800 (PST) Date: Wed, 8 Dec 2010 00:07:10 +0900 From: Minchan Kim Subject: Re: [PATCH v4 2/7] deactivate invalidated pages Message-ID: <20101207150710.GA26613@barrios-desktop> References: <20101207144923.GB2356@cmpxchg.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20101207144923.GB2356@cmpxchg.org> Sender: owner-linux-mm@kvack.org To: Johannes Weiner Cc: Andrew Morton , Rik van Riel , KOSAKI Motohiro , linux-mm , LKML , Peter Zijlstra , Wu Fengguang , Nick Piggin , Mel Gorman List-ID: 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. I want to check this. > > Thanks! > > Hannes -- Kind regards, Minchan Kim -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/ Don't email: email@kvack.org