LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] mm: Add more comments for MADV_FREE
@ 2020-03-11  1:11 Huang, Ying
  2020-03-11  5:08 ` David Rientjes
  0 siblings, 1 reply; 4+ messages in thread
From: Huang, Ying @ 2020-03-11  1:11 UTC (permalink / raw)
  To: Andrew Morton
  Cc: linux-mm, linux-kernel, Huang Ying, David Hildenbrand,
	David Rientjes, Michal Hocko, Dave Hansen, Mel Gorman,
	Vlastimil Babka, Minchan Kim, Johannes Weiner, Hugh Dickins,
	Rik van Riel

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


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] mm: Add more comments for MADV_FREE
  2020-03-11  1:11 [PATCH] mm: Add more comments for MADV_FREE Huang, Ying
@ 2020-03-11  5:08 ` David Rientjes
  2020-03-11  5:22   ` Huang, Ying
  0 siblings, 1 reply; 4+ messages in thread
From: David Rientjes @ 2020-03-11  5:08 UTC (permalink / raw)
  To: Huang, Ying
  Cc: Andrew Morton, linux-mm, linux-kernel, David Hildenbrand,
	Michal Hocko, Dave Hansen, Mel Gorman, Vlastimil Babka,
	Minchan Kim, Johannes Weiner, Hugh Dickins, Rik van Riel

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.

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] mm: Add more comments for MADV_FREE
  2020-03-11  5:08 ` David Rientjes
@ 2020-03-11  5:22   ` Huang, Ying
  2020-03-11 14:47     ` Johannes Weiner
  0 siblings, 1 reply; 4+ messages in thread
From: Huang, Ying @ 2020-03-11  5:22 UTC (permalink / raw)
  To: David Rientjes
  Cc: Andrew Morton, linux-mm, linux-kernel, David Hildenbrand,
	Michal Hocko, Dave Hansen, Mel Gorman, Vlastimil Babka,
	Minchan Kim, Johannes Weiner, Hugh Dickins, Rik van Riel,
	Matthew Wilcox

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] mm: Add more comments for MADV_FREE
  2020-03-11  5:22   ` Huang, Ying
@ 2020-03-11 14:47     ` Johannes Weiner
  0 siblings, 0 replies; 4+ messages in thread
From: Johannes Weiner @ 2020-03-11 14:47 UTC (permalink / raw)
  To: Huang, Ying
  Cc: David Rientjes, Andrew Morton, linux-mm, linux-kernel,
	David Hildenbrand, Michal Hocko, Dave Hansen, Mel Gorman,
	Vlastimil Babka, Minchan Kim, Hugh Dickins, Rik van Riel,
	Matthew Wilcox

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.

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2020-03-11 14:47 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-11  1:11 [PATCH] mm: Add more comments for MADV_FREE Huang, Ying
2020-03-11  5:08 ` David Rientjes
2020-03-11  5:22   ` Huang, Ying
2020-03-11 14:47     ` Johannes Weiner

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).