[PATCH] mm: Add more comments for MADV_FREE

[PATCH] mm: Add more comments for MADV_FREE

[Huang, Ying]

From: Huang Ying <ying.huang@intel.com>

To help people understand the MADV_FREE code, especially the page flag,
PG_swapbacked.

Signed-off-by: "Huang, Ying" <ying.huang@intel.com>
Suggested-by: David Hildenbrand <david@redhat.com>
Cc: David Rientjes <rientjes@google.com>
Cc: Michal Hocko <mhocko@kernel.org>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Mel Gorman <mgorman@suse.de>
Cc: Vlastimil Babka <vbabka@suse.cz>
Cc: Minchan Kim <minchan@kernel.org>
Cc: Johannes Weiner <hannes@cmpxchg.org>
Cc: Hugh Dickins <hughd@google.com>
Cc: Rik van Riel <riel@surriel.com>
---
 include/linux/mm_inline.h  | 9 +++++----
 include/linux/page-flags.h | 4 ++++
 mm/swap.c                  | 6 +++---
 3 files changed, 12 insertions(+), 7 deletions(-)

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.
  *
  * We would like to get this info without a page flag, but the state
  * needs to survive until the page is last deleted from the LRU, which
diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h
index 49c2697046b9..992b71c4fc85 100644
--- a/include/linux/page-flags.h
+++ b/include/linux/page-flags.h
@@ -63,6 +63,10 @@
  * page_waitqueue(page) is a wait queue of all tasks waiting for the page
  * to become unlocked.
  *
+ * PG_swapbacked is cleared if the page is page cache page backed by a regular
+ * file system or anonymous page lazily freed (e.g. via MADV_FREE).  It is set
+ * if the page is normal anonymous page, tmpfs or otherwise RAM or swap backed.
+ *
  * PG_uptodate tells whether the page's contents is valid.  When a read
  * completes, the page becomes uptodate, unless a disk I/O error happened.
  *
diff --git a/mm/swap.c b/mm/swap.c
index 6a8be910b14d..90bb14695809 100644
--- a/mm/swap.c
+++ b/mm/swap.c
@@ -573,9 +573,9 @@ static void lru_lazyfree_fn(struct page *page, struct lruvec *lruvec,
 		ClearPageActive(page);
 		ClearPageReferenced(page);
 		/*
-		 * lazyfree pages are clean anonymous pages. They have
-		 * SwapBacked flag cleared to distinguish normal anonymous
-		 * pages
+		 * Lazyfree pages are clean anonymous pages.  They
+		 * have PG_swapbacked flag cleared, to distinguish
+		 * them from normal anonymous pages
 		 */
 		ClearPageSwapBacked(page);
 		add_page_to_lru_list(page, lruvec, LRU_INACTIVE_FILE);
-- 
2.25.0

Re: [PATCH] mm: Add more comments for MADV_FREE

[David Rientjes]

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().

The rest looks fine.

Re: [PATCH] mm: Add more comments for MADV_FREE

[Huang, Ying]

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.

Best Regards,
Huang, Ying

Re: [PATCH] mm: Add more comments for MADV_FREE

[Johannes Weiner]

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.