Re: [PATCH v4 4/5] mm/vmscan: extract folio unmap logic into folio_try_unmap()

[David Hildenbrand (Arm)]

On 5/25/26 16:57, Zhang Peng wrote:
> shrink_folio_list() contains a self-contained block that sets up
> TTU flags and calls try_to_unmap(), accounting for failures via
> reclaim_stat. Extract it into folio_try_unmap() to reduce the size
> of shrink_folio_list() and make the unmap step independently readable.
> 
> folio_try_unmap() is only called when the folio is actually mapped;
> the !folio_mapped() check stays in the caller, keeping the function's
> semantics clear: it tries to unmap a mapped folio and returns whether
> the unmap succeeded.
> 
> No functional change.
> 
> Signed-off-by: Zhang Peng <bruzzhang@xxxxxxxxxxx>
> ---
>  mm/vmscan.c | 68 ++++++++++++++++++++++++++++++++++---------------------------
>  1 file changed, 38 insertions(+), 30 deletions(-)
> 
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index 456d38eb172c..abf3a2878456 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -1221,6 +1221,41 @@ static bool pageout_one(struct folio *folio,
>  	return false;
>  }
>  
> +static bool folio_try_unmap(struct folio *folio, struct reclaim_stat *stat,
> +			    unsigned int nr_pages)

Two tabs.

folio_try_unmap() vs. try_to_unmap() Hm.

Again, maybe we should throw in a "for_reclaim" ?

folio_try_unmap_for_reclaim() ?

Not sure.

> +{
> +	enum ttu_flags flags = TTU_BATCH_FLUSH;
> +	bool was_swapbacked;
> +
> +	was_swapbacked = folio_test_swapbacked(folio);


const bool was_swapbacked = folio_test_swapbacked(folio);

> +	if (folio_test_pmd_mappable(folio))
> +		flags |= TTU_SPLIT_HUGE_PMD;
> +	/*
> +	 * Without TTU_SYNC, try_to_unmap will only begin to
> +	 * hold PTL from the first present PTE within a large
> +	 * folio. Some initial PTEs might be skipped due to
> +	 * races with parallel PTE writes in which PTEs can be
> +	 * cleared temporarily before being written new present
> +	 * values. This will lead to a large folio is still
> +	 * mapped while some subpages have been partially
> +	 * unmapped after try_to_unmap; TTU_SYNC helps
> +	 * try_to_unmap acquire PTL from the first PTE,
> +	 * eliminating the influence of temporary PTE values.
> +	 */

Comment can now use less LOC.

> +	if (folio_test_large(folio))
> +		flags |= TTU_SYNC;
> +
> +	try_to_unmap(folio, flags);
> +	if (folio_mapped(folio)) {
> +		stat->nr_unmap_fail += nr_pages;
> +		if (!was_swapbacked &&
> +		    folio_test_swapbacked(folio))

Probably best in a single line.

> +			stat->nr_lazyfree_fail += nr_pages;
> +		return false;
> +	}
> +	return true;
> +}
> +
>  /*
>   * Reclaimed folios are counted in the return value.
>   */
> @@ -1495,36 +1530,9 @@ static unsigned int shrink_folio_list(struct list_head *folio_list,
>  		 * The folio is mapped into the page tables of one or more
>  		 * processes. Try to unmap it here.
>  		 */
> -		if (folio_mapped(folio)) {
> -			enum ttu_flags flags = TTU_BATCH_FLUSH;
> -			bool was_swapbacked = folio_test_swapbacked(folio);
> -
> -			if (folio_test_pmd_mappable(folio))
> -				flags |= TTU_SPLIT_HUGE_PMD;
> -			/*
> -			 * Without TTU_SYNC, try_to_unmap will only begin to
> -			 * hold PTL from the first present PTE within a large
> -			 * folio. Some initial PTEs might be skipped due to
> -			 * races with parallel PTE writes in which PTEs can be
> -			 * cleared temporarily before being written new present
> -			 * values. This will lead to a large folio is still
> -			 * mapped while some subpages have been partially
> -			 * unmapped after try_to_unmap; TTU_SYNC helps
> -			 * try_to_unmap acquire PTL from the first PTE,
> -			 * eliminating the influence of temporary PTE values.
> -			 */
> -			if (folio_test_large(folio))
> -				flags |= TTU_SYNC;
> -
> -			try_to_unmap(folio, flags);
> -			if (folio_mapped(folio)) {
> -				stat->nr_unmap_fail += nr_pages;
> -				if (!was_swapbacked &&
> -				    folio_test_swapbacked(folio))
> -					stat->nr_lazyfree_fail += nr_pages;
> -				goto activate_locked;
> -			}
> -		}
> +		if (folio_mapped(folio) &&
> +		    !folio_try_unmap(folio, stat, nr_pages))

Probably best in a single line.

> +			goto activate_locked;
>  
>  		/*
>  		 * Folio is unmapped now so it cannot be newly pinned anymore.
> 


-- 
Cheers,

David