| Message ID | 20200311011117.1656744-1-ying.huang@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mm: Add more comments for MADV_FREE | expand |
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.
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
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.
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);