Re: [PATCH 03/10] mm/madvise: factor shared LRU folio handling

[Lorenzo Stoakes (ARM)]

On Tue, Sep 22, 2026 at 07:58:23PM -0400, Gregory Price wrote:
> The huge-PMD and PTE paths duplicate folio filtering, reference clearing,
> deactivation and pageout isolation. Keeping both copies synchronized
> obscures the page-table-specific control flow.
>
> Factor the common filtering and LRU operation into helpers. Keep the
> lock-before-reference sequence visible at each split site because an early
> reference can prevent split_folio() from succeeding.
>
> No functional change intended.
>
> Assisted-by: LLM
> Signed-off-by: Gregory Price (Meta) <gourry@xxxxxxxxxx>

LGTM and AFAICT all the code is the same as before just less horrifying so:

Reviewed-by: Lorenzo Stoakes (ARM) <ljs@xxxxxxxxxx>

> ---
>  mm/madvise.c | 90 +++++++++++++++++++++++++---------------------------
>  1 file changed, 43 insertions(+), 47 deletions(-)
>
> diff --git a/mm/madvise.c b/mm/madvise.c
> index 83d54ab385da8..c345fef23f15d 100644
> --- a/mm/madvise.c
> +++ b/mm/madvise.c
> @@ -361,6 +361,39 @@ static inline int madvise_folio_pte_batch(unsigned long addr, unsigned long end,
>  				     FPB_MERGE_YOUNG_DIRTY);
>  }
>
> +static inline void
> +madvise_lru_folio(struct folio *folio, bool pageout,
> +		struct list_head *folio_list)
> +{
> +	/*
> +	 * Clear references before deactivating or reclaiming the folio. This can
> +	 * make idle-page tracking miss recent accesses.
> +	 */
> +	folio_clear_referenced(folio);
> +	folio_test_clear_young(folio);
> +	if (folio_test_active(folio))
> +		folio_set_workingset(folio);
> +
> +	if (!pageout) {
> +		folio_deactivate(folio);
> +		return;
> +	}
> +
> +	if (!folio_isolate_lru(folio))
> +		return;
> +	if (folio_test_unevictable(folio))
> +		folio_putback_lru(folio);
> +	else
> +		list_add(&folio->lru, folio_list);
> +}
> +
> +static bool madvise_lru_folio_is_filtered(struct folio *folio,
> +		bool pageout_anon_only)
> +{
> +	return folio_maybe_mapped_shared(folio) ||
> +	       (pageout_anon_only && !folio_test_anon(folio));
> +}
> +
>  static int madvise_lru_pmd_entry(pmd_t *pmd, unsigned long addr,
>  		unsigned long end, struct mm_walk *walk)
>  {
> @@ -373,15 +406,14 @@ static int madvise_lru_pmd_entry(pmd_t *pmd, unsigned long addr,
>  	spinlock_t *ptl;
>  	struct folio *folio = NULL;
>  	LIST_HEAD(folio_list);
> -	bool pageout_anon_only_filter;
>  	unsigned int batch_count = 0;
> +	bool pageout_anon_only;
>  	int nr;
>
>  	if (fatal_signal_pending(current))
>  		return -EINTR;
> -
> -	pageout_anon_only_filter = pageout && !vma_is_anonymous(vma) &&
> -					!can_do_file_pageout(vma);
> +	pageout_anon_only = pageout && !vma_is_anonymous(vma) &&
> +				       !can_do_file_pageout(vma);
>
>  #ifdef CONFIG_TRANSPARENT_HUGEPAGE
>  	if (pmd_trans_huge(*pmd)) {
> @@ -407,11 +439,7 @@ static int madvise_lru_pmd_entry(pmd_t *pmd, unsigned long addr,
>  		if (folio_is_zone_device(folio))
>  			goto huge_unlock;
>
> -		/* Do not interfere with other mappings of this folio */
> -		if (folio_maybe_mapped_shared(folio))
> -			goto huge_unlock;
> -
> -		if (pageout_anon_only_filter && !folio_test_anon(folio))
> +		if (madvise_lru_folio_is_filtered(folio, pageout_anon_only))
>  			goto huge_unlock;
>
>  		if (next - addr != HPAGE_PMD_SIZE) {
> @@ -437,19 +465,7 @@ static int madvise_lru_pmd_entry(pmd_t *pmd, unsigned long addr,
>  			tlb_remove_pmd_tlb_entry(tlb, pmd, addr);
>  		}
>
> -		folio_clear_referenced(folio);
> -		folio_test_clear_young(folio);
> -		if (folio_test_active(folio))
> -			folio_set_workingset(folio);
> -		if (pageout) {
> -			if (folio_isolate_lru(folio)) {
> -				if (folio_test_unevictable(folio))
> -					folio_putback_lru(folio);
> -				else
> -					list_add(&folio->lru, &folio_list);
> -			}
> -		} else
> -			folio_deactivate(folio);
> +		madvise_lru_folio(folio, pageout, &folio_list);
>  huge_unlock:
>  		spin_unlock(ptl);
>  		if (pageout)
> @@ -502,9 +518,7 @@ static int madvise_lru_pmd_entry(pmd_t *pmd, unsigned long addr,
>  			if (nr < folio_nr_pages(folio)) {
>  				int err;
>
> -				if (folio_maybe_mapped_shared(folio))
> -					continue;
> -				if (pageout_anon_only_filter && !folio_test_anon(folio))
> +				if (madvise_lru_folio_is_filtered(folio, pageout_anon_only))
>  					continue;
>  				if (!folio_trylock(folio))
>  					continue;
> @@ -537,7 +551,7 @@ static int madvise_lru_pmd_entry(pmd_t *pmd, unsigned long addr,
>  		    folio_mapcount(folio) != folio_nr_pages(folio))
>  			continue;
>
> -		if (pageout_anon_only_filter && !folio_test_anon(folio))
> +		if (pageout_anon_only && !folio_test_anon(folio))
>  			continue;
>
>  		if (!pageout && pte_young(ptent)) {
> @@ -546,25 +560,7 @@ static int madvise_lru_pmd_entry(pmd_t *pmd, unsigned long addr,
>  			tlb_remove_tlb_entries(tlb, pte, nr, addr);
>  		}
>
> -		/*
> -		 * We are deactivating a folio for accelerating reclaiming.
> -		 * VM couldn't reclaim the folio unless we clear PG_young.
> -		 * As a side effect, it makes confuse idle-page tracking
> -		 * because they will miss recent referenced history.
> -		 */
> -		folio_clear_referenced(folio);
> -		folio_test_clear_young(folio);
> -		if (folio_test_active(folio))
> -			folio_set_workingset(folio);
> -		if (pageout) {
> -			if (folio_isolate_lru(folio)) {
> -				if (folio_test_unevictable(folio))
> -					folio_putback_lru(folio);
> -				else
> -					list_add(&folio->lru, &folio_list);
> -			}
> -		} else
> -			folio_deactivate(folio);
> +		madvise_lru_folio(folio, pageout, &folio_list);
>  	}
>
>  out:
> @@ -651,8 +647,8 @@ static long madvise_pageout(struct madvise_behavior *madv_behavior)
>  	 * owner nor write capable of the file. We allow private file mappings
>  	 * further to pageout dirty anon pages.
>  	 */
> -	if (!vma_is_anonymous(vma) && (!can_do_file_pageout(vma) &&
> -				(vma->vm_flags & VM_MAYSHARE)))
> +	if (!vma_is_anonymous(vma) && !can_do_file_pageout(vma) &&
> +	    (vma->vm_flags & VM_MAYSHARE))
>  		return 0;
>
>  	lru_add_drain();
> --
> 2.53.0-Meta
>

--
Cheers, Lorenzo