Re: [PATCH 1/2] mm: always call rmap_walk() on locked folios

[David Hildenbrand <david@redhat.com>]

On 18.09.25 07:51, Lokesh Gidra wrote:
> Guarantee that rmap_walk() is called on locked folios so that threads
> changing folio->mapping and folio->index for non-KSM anon folios can
> serialize on fine-grained folio lock rather than anon_vma lock. Other
> folio types are already always locked before rmap_walk().
> 
> This is in preparation for removing anon_vma write-lock from
> UFFDIO_MOVE.

I think quite a big bunch of the cover letter belongs into this patch 
here. In essence, why is it okay, what could be problematic, history 
etc, etc.

> 
> CC: David Hildenbrand <david@redhat.com>
> CC: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> CC: Harry Yoo <harry.yoo@oracle.com>
> CC: Peter Xu <peterx@redhat.com>
> CC: Suren Baghdasaryan <surenb@google.com>
> CC: Barry Song <baohua@kernel.org>
> CC: SeongJae Park <sj@kernel.org>
> Signed-off-by: Lokesh Gidra <lokeshgidra@google.com>
> ---
>   mm/damon/ops-common.c | 16 ++++------------
>   mm/memory-failure.c   |  3 +++
>   mm/page_idle.c        |  8 ++------
>   mm/rmap.c             | 42 ++++++++++++------------------------------
>   4 files changed, 21 insertions(+), 48 deletions(-)
> 
> diff --git a/mm/damon/ops-common.c b/mm/damon/ops-common.c
> index 998c5180a603..f61d6dde13dc 100644
> --- a/mm/damon/ops-common.c
> +++ b/mm/damon/ops-common.c
> @@ -162,21 +162,17 @@ void damon_folio_mkold(struct folio *folio)
>   		.rmap_one = damon_folio_mkold_one,
>   		.anon_lock = folio_lock_anon_vma_read,
>   	};
> -	bool need_lock;
>   
>   	if (!folio_mapped(folio) || !folio_raw_mapping(folio)) {
>   		folio_set_idle(folio);
>   		return;
>   	}
>   
> -	need_lock = !folio_test_anon(folio) || folio_test_ksm(folio);
> -	if (need_lock && !folio_trylock(folio))
> +	if (!folio_trylock(folio))
>   		return;
>   
>   	rmap_walk(folio, &rwc);
> -
> -	if (need_lock)
> -		folio_unlock(folio);
> +	folio_unlock(folio);
>   
>   }
>   
> @@ -228,7 +224,6 @@ bool damon_folio_young(struct folio *folio)
>   		.rmap_one = damon_folio_young_one,
>   		.anon_lock = folio_lock_anon_vma_read,
>   	};
> -	bool need_lock;
>   
>   	if (!folio_mapped(folio) || !folio_raw_mapping(folio)) {
>   		if (folio_test_idle(folio))
> @@ -237,14 +232,11 @@ bool damon_folio_young(struct folio *folio)
>   			return true;
>   	}
>   
> -	need_lock = !folio_test_anon(folio) || folio_test_ksm(folio);
> -	if (need_lock && !folio_trylock(folio))
> +	if (!folio_trylock(folio))
>   		return false;
>   
>   	rmap_walk(folio, &rwc);
> -
> -	if (need_lock)
> -		folio_unlock(folio);
> +	folio_unlock(folio);
>   
>   	return accessed;
>   }
> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> index a24806bb8e82..f698df156bf8 100644
> --- a/mm/memory-failure.c
> +++ b/mm/memory-failure.c
> @@ -2143,7 +2143,10 @@ static void kill_procs_now(struct page *p, unsigned long pfn, int flags,
>   {
>   	LIST_HEAD(tokill);
>   
> +	folio_lock(folio);
>   	collect_procs(folio, p, &tokill, flags & MF_ACTION_REQUIRED);
> +	folio_unlock(folio);
> +
>   	kill_procs(&tokill, true, pfn, flags);
>   }

Is mf_generic_kill_procs()->collect_procs() problematic?

The dax_lock_folio/dax_unlock_folio part confuses me: I think it only 
locks an entry in the page cache but not the actual folio ("Lock the DAX 
entry corresponding to a folio")?

-- 
Cheers

David / dhildenb