Re: [PATCH -V3] mm: Add PageLayzyFree() helper functions for MADV_FREE

[Huang\, Ying]

David Hildenbrand <david@xxxxxxxxxx> writes:

> On 09.03.20 03:17, Huang, Ying wrote:
>> From: Huang Ying <ying.huang@xxxxxxxxx>
>> 
>> Now PageSwapBacked() is used as the helper function to check whether
>> pages have been freed lazily via MADV_FREE.  This isn't very obvious.
>> So Dave suggested to add PageLazyFree() family helper functions to
>> improve the code readability.
>> 
>> Signed-off-by: "Huang, Ying" <ying.huang@xxxxxxxxx>
>> Suggested-by: Dave Hansen <dave.hansen@xxxxxxxxxxxxxxx>
>> Cc: David Hildenbrand <david@xxxxxxxxxx>
>> Cc: Mel Gorman <mgorman@xxxxxxx>
>> Cc: Vlastimil Babka <vbabka@xxxxxxx>
>> Cc: Zi Yan <ziy@xxxxxxxxxx>
>> Cc: Michal Hocko <mhocko@xxxxxxxxxx>
>> Cc: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
>> Cc: Minchan Kim <minchan@xxxxxxxxxx>
>> Cc: Johannes Weiner <hannes@xxxxxxxxxxx>
>> Cc: Hugh Dickins <hughd@xxxxxxxxxx>
>> ---
>> Changelog:
>> 
>> v3:
>> 
>> - Improved comments, Thanks David Hildenbrand!
>> 
>> - Use the helper function in /proc/PID/smaps lazyfree reporting.
>> 
>> v2:
>> 
>> - Avoid code bloat via removing VM_BUG_ON_PAGE(), which doesn't exist
>>   in the original code.  Now there is no any text/data/bss size
>>   change.
>> 
>> - Fix one wrong replacement in try_to_unmap_one().
>> 
>> ---
>>  fs/proc/task_mmu.c         |  2 +-
>>  include/linux/page-flags.h | 25 +++++++++++++++++++++++++
>>  mm/rmap.c                  |  4 ++--
>>  mm/swap.c                  | 11 +++--------
>>  mm/vmscan.c                |  7 +++----
>>  5 files changed, 34 insertions(+), 15 deletions(-)
>> 
>> diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
>> index 3ba9ae83bff5..3458d5711e57 100644
>> --- a/fs/proc/task_mmu.c
>> +++ b/fs/proc/task_mmu.c
>> @@ -471,7 +471,7 @@ static void smaps_account(struct mem_size_stats *mss, struct page *page,
>>  	 */
>>  	if (PageAnon(page)) {
>>  		mss->anonymous += size;
>> -		if (!PageSwapBacked(page) && !dirty && !PageDirty(page))
>> +		if (__PageLazyFree(page) && !dirty && !PageDirty(page))
>>  			mss->lazyfree += size;
>>  	}
>>  
>> diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h
>> index 49c2697046b9..bb26f74cbe8e 100644
>> --- a/include/linux/page-flags.h
>> +++ b/include/linux/page-flags.h
>> @@ -498,6 +498,31 @@ static __always_inline int PageKsm(struct page *page)
>>  TESTPAGEFLAG_FALSE(Ksm)
>>  #endif
>>  
>> +/*
>> + * For pages freed lazily via MADV_FREE.  Lazyfree pages are clean
>> + * anonymous pages.  They don't have PG_swapbacked set, to distinguish
>> + * them from normal anonymous pages.
>> + */
>> +static __always_inline int __PageLazyFree(struct page *page)
>> +{
>> +	return !PageSwapBacked(page);
>> +}
>> +
>> +static __always_inline int PageLazyFree(struct page *page)
>> +{
>> +	return PageAnon(page) && __PageLazyFree(page);
>> +}
>> +
>> +static __always_inline void SetPageLazyFree(struct page *page)
>> +{
>> +	ClearPageSwapBacked(page);
>> +}
>> +
>> +static __always_inline void ClearPageLazyFree(struct page *page)
>> +{
>> +	SetPageSwapBacked(page);
>> +}
>> +
>>  u64 stable_page_flags(struct page *page);
>>  
>>  static inline int PageUptodate(struct page *page)
>> diff --git a/mm/rmap.c b/mm/rmap.c
>> index 1c02adaa233e..6ec96c8e7826 100644
>> --- a/mm/rmap.c
>> +++ b/mm/rmap.c
>> @@ -1609,7 +1609,7 @@ static bool try_to_unmap_one(struct page *page, struct vm_area_struct *vma,
>>  			}
>>  
>>  			/* MADV_FREE page check */
>> -			if (!PageSwapBacked(page)) {
>> +			if (__PageLazyFree(page)) {
>>  				if (!PageDirty(page)) {
>>  					/* Invalidate as we cleared the pte */
>>  					mmu_notifier_invalidate_range(mm,
>> @@ -1623,7 +1623,7 @@ static bool try_to_unmap_one(struct page *page, struct vm_area_struct *vma,
>>  				 * discarded. Remap the page to page table.
>>  				 */
>>  				set_pte_at(mm, address, pvmw.pte, pteval);
>> -				SetPageSwapBacked(page);
>> +				ClearPageLazyFree(page);
>>  				ret = false;
>>  				page_vma_mapped_walk_done(&pvmw);
>>  				break;
>> diff --git a/mm/swap.c b/mm/swap.c
>> index c1d3ca80ea10..d83f2cd4cdb8 100644
>> --- a/mm/swap.c
>> +++ b/mm/swap.c
>> @@ -563,7 +563,7 @@ static void lru_deactivate_fn(struct page *page, struct lruvec *lruvec,
>>  static void lru_lazyfree_fn(struct page *page, struct lruvec *lruvec,
>>  			    void *arg)
>>  {
>> -	if (PageLRU(page) && PageAnon(page) && PageSwapBacked(page) &&
>> +	if (PageLRU(page) && PageAnon(page) && !__PageLazyFree(page) &&
>>  	    !PageSwapCache(page) && !PageUnevictable(page)) {
>>  		bool active = PageActive(page);
>>  
>> @@ -571,12 +571,7 @@ static void lru_lazyfree_fn(struct page *page, struct lruvec *lruvec,
>>  				       LRU_INACTIVE_ANON + active);
>>  		ClearPageActive(page);
>>  		ClearPageReferenced(page);
>> -		/*
>> -		 * lazyfree pages are clean anonymous pages. They have
>> -		 * SwapBacked flag cleared to distinguish normal anonymous
>> -		 * pages
>> -		 */
>> -		ClearPageSwapBacked(page);
>> +		SetPageLazyFree(page);
>>  		add_page_to_lru_list(page, lruvec, LRU_INACTIVE_FILE);
>>  
>>  		__count_vm_events(PGLAZYFREE, hpage_nr_pages(page));
>> @@ -678,7 +673,7 @@ void deactivate_page(struct page *page)
>>   */
>>  void mark_page_lazyfree(struct page *page)
>>  {
>> -	if (PageLRU(page) && PageAnon(page) && PageSwapBacked(page) &&
>> +	if (PageLRU(page) && PageAnon(page) && !__PageLazyFree(page) &&
>>  	    !PageSwapCache(page) && !PageUnevictable(page)) {
>>  		struct pagevec *pvec = &get_cpu_var(lru_lazyfree_pvecs);
>>  
>> diff --git a/mm/vmscan.c b/mm/vmscan.c
>> index eca49a1c2f68..40bb41ada2d2 100644
>> --- a/mm/vmscan.c
>> +++ b/mm/vmscan.c
>> @@ -1043,8 +1043,7 @@ static void page_check_dirty_writeback(struct page *page,
>>  	 * Anonymous pages are not handled by flushers and must be written
>>  	 * from reclaim context. Do not stall reclaim based on them
>>  	 */
>> -	if (!page_is_file_cache(page) ||
>> -	    (PageAnon(page) && !PageSwapBacked(page))) {
>> +	if (!page_is_file_cache(page) || PageLazyFree(page)) {
>>  		*dirty = false;
>>  		*writeback = false;
>>  		return;
>> @@ -1235,7 +1234,7 @@ static unsigned long shrink_page_list(struct list_head *page_list,
>>  		 * Try to allocate it some swap space here.
>>  		 * Lazyfree page could be freed directly
>>  		 */
>> -		if (PageAnon(page) && PageSwapBacked(page)) {
>> +		if (PageAnon(page) && !__PageLazyFree(page)) {
>>  			if (!PageSwapCache(page)) {
>>  				if (!(sc->gfp_mask & __GFP_IO))
>>  					goto keep_locked;
>> @@ -1411,7 +1410,7 @@ static unsigned long shrink_page_list(struct list_head *page_list,
>>  			}
>>  		}
>>  
>> -		if (PageAnon(page) && !PageSwapBacked(page)) {
>> +		if (PageLazyFree(page)) {
>>  			/* follow __remove_mapping for reference */
>>  			if (!page_ref_freeze(page, 1))
>>  				goto keep_locked;
>> 
>
> I still prefer something like
>
> diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h
> index fd6d4670ccc3..7538501230bd 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 used with anonymous pages (PageAnon()) indicates that a
> + * page is backed by swap. Anonymous pages without PG_swapbacked are
> + * pages that can be lazily freed (e.g., MADV_FREE) on demand.
> + *
>   * 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.
>   *

Why not just send a formal patch?  So Andrew can just pick anything he
likes.  I am totally OK with that.

> and really don't like the use of !__PageLazyFree() instead of PageSwapBacked().

If adopted, !__PageLazyFree() should only be used in the context where
we really want to check whether pages are freed lazily.  Otherwise,
PageSwapBacked() should be used.

Best Regards,
Huang, Ying