Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Wed, 11 Mar 2020 10:47:51 -0400
From:   Johannes Weiner <hannes@cmpxchg.org>
To:     "Huang, Ying" <ying.huang@intel.com>
Cc:     David Rientjes <rientjes@google.com>,
        Andrew Morton <akpm@linux-foundation.org>, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org, David Hildenbrand <david@redhat.com>,
        Michal Hocko <mhocko@kernel.org>,
        Dave Hansen <dave.hansen@linux.intel.com>,
        Mel Gorman <mgorman@suse.de>, Vlastimil Babka <vbabka@suse.cz>,
        Minchan Kim <minchan@kernel.org>,
        Hugh Dickins <hughd@google.com>,
        Rik van Riel <riel@surriel.com>,
        Matthew Wilcox <willy@infradead.org>
Subject: Re: [PATCH] mm: Add more comments for MADV_FREE
Message-ID: <20200311144751.GA29835@cmpxchg.org>
References: <20200311011117.1656744-1-ying.huang@intel.com>
 <alpine.DEB.2.21.2003102204450.255869@chino.kir.corp.google.com>
 <87imjbv51t.fsf@yhuang-dev.intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <87imjbv51t.fsf@yhuang-dev.intel.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Wed, Mar 11, 2020 at 01:22:54PM +0800, Huang, Ying wrote:
> David Rientjes <rientjes@google.com> writes:
> 
> > On Wed, 11 Mar 2020, Huang, Ying wrote:
> >
> >> diff --git a/include/linux/mm_inline.h b/include/linux/mm_inline.h
> >> index 6f2fef7b0784..01144dd02a5f 100644
> >> --- a/include/linux/mm_inline.h
> >> +++ b/include/linux/mm_inline.h
> >> @@ -9,10 +9,11 @@
> >>   * page_is_file_cache - should the page be on a file LRU or anon LRU?
> >>   * @page: the page to test
> >>   *
> >> - * Returns 1 if @page is page cache page backed by a regular filesystem,
> >> - * or 0 if @page is anonymous, tmpfs or otherwise ram or swap backed.
> >> - * Used by functions that manipulate the LRU lists, to sort a page
> >> - * onto the right LRU list.
> >> + * Returns 1 if @page is page cache page backed by a regular filesystem or
> >> + * anonymous page lazily freed (e.g. via MADV_FREE).  Returns 0 if @page is
> >> + * normal anonymous page, tmpfs or otherwise ram or swap backed.  Used by
> >> + * functions that manipulate the LRU lists, to sort a page onto the right LRU
> >> + * list.
> >
> > The function name is misleading: anonymous pages that can be lazily freed 
> > are not file cache.  This returns 1 because of the question it is asking: 
> > anonymous lazily freeable pages should be on the file lru, not the anon 
> > lru.  So before adjusting the comment I'd suggest renaming the function to 
> > something like page_is_file_lru().
> 
> Yes.  I think page_is_file_lru() is a better name too.  And whether
> tmpfs pages are file cache pages is confusing too.  But I think we can
> do that after this patch if others think this is a good idea too.

I also think the rename is a great idea.

Personally, I'd prefer it in the same patch. Right now the name and
the documentation are out of date, but at least they're consistent in
their view of the world. Fixing this interface - name and
documentation - to reflect the existence of MADV_FREE anon pages is
one logical change, not two.