From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752089AbbCWDBT (ORCPT ); Sun, 22 Mar 2015 23:01:19 -0400 Received: from mail-pa0-f46.google.com ([209.85.220.46]:35959 "EHLO mail-pa0-f46.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751997AbbCWDBQ (ORCPT ); Sun, 22 Mar 2015 23:01:16 -0400 Date: Sun, 22 Mar 2015 20:01:13 -0700 (PDT) From: Hugh Dickins X-X-Sender: hugh@eggly.anvils To: Mel Gorman cc: Hugh Dickins , "Kirill A. Shutemov" , Andrea Arcangeli , Ning Qu , Andrew Morton , linux-kernel@vger.kernel.org, linux-mm@kvack.org Subject: Re: [PATCH 03/24] mm: use __SetPageSwapBacked and don't ClearPageSwapBacked In-Reply-To: <20150225105315.GK23372@suse.de> Message-ID: References: <20150225105315.GK23372@suse.de> User-Agent: Alpine 2.11 (LSU 23 2013-08-11) MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, 25 Feb 2015, Mel Gorman wrote: > On Fri, Feb 20, 2015 at 07:56:15PM -0800, Hugh Dickins wrote: > > Commit 07a427884348 ("mm: shmem: avoid atomic operation during > > shmem_getpage_gfp") rightly replaced one instance of SetPageSwapBacked > > by __SetPageSwapBacked, pointing out that the newly allocated page is > > not yet visible to other users (except speculative get_page_unless_zero- > > ers, who may not update page flags before their further checks). > > > > That was part of a series in which Mel was focused on tmpfs profiles: > > but almost all SetPageSwapBacked uses can be so optimized, with the > > same justification. And remove the ClearPageSwapBacked from > > read_swap_cache_async()'s and zswap_get_swap_cache_page()'s error > > paths: it's not an error to free a page with PG_swapbacked set. > > > > (There's probably scope for further __SetPageFlags in other places, > > but SwapBacked is the one I'm interested in at the moment.) > > > > Signed-off-by: Hugh Dickins > > --- > > mm/migrate.c | 6 +++--- > > mm/rmap.c | 2 +- > > mm/shmem.c | 4 ++-- > > mm/swap_state.c | 3 +-- > > mm/zswap.c | 3 +-- > > 5 files changed, 8 insertions(+), 10 deletions(-) > > > > > > --- thpfs.orig/mm/shmem.c 2015-02-08 18:54:22.000000000 -0800 > > +++ thpfs/mm/shmem.c 2015-02-20 19:33:35.676074594 -0800 > > @@ -987,8 +987,8 @@ static int shmem_replace_page(struct pag > > flush_dcache_page(newpage); > > > > __set_page_locked(newpage); > > + __SetPageSwapBacked(newpage); > > SetPageUptodate(newpage); > > - SetPageSwapBacked(newpage); > > set_page_private(newpage, swap_index); > > SetPageSwapCache(newpage); > > > > It's clear why you did this but ... > > > @@ -1177,8 +1177,8 @@ repeat: > > goto decused; > > } > > > > - __SetPageSwapBacked(page); > > __set_page_locked(page); > > + __SetPageSwapBacked(page); > > if (sgp == SGP_WRITE) > > __SetPageReferenced(page); > > > > It's less clear why this was necessary. I don't think the reordering was necessary in either case (though perhaps the first hunk makes a subsequent patch smaller). I just get irritated by seeing the same lines of code permuted in different ways for no reason, and thought I'd tidy them up to establish one familiar sequence, that's all. > I don't think it causes any problems though so > > Reviewed-by: Mel Gorman Thanks! Hugh