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

[Harry Yoo <harry.yoo@oracle.com>]

On Wed, Sep 10, 2025 at 08:33:54AM -0700, Lokesh Gidra wrote:
> On Wed, Sep 10, 2025 at 3:10 AM Harry Yoo <harry.yoo@oracle.com> wrote:
> >
> > On Sun, Sep 07, 2025 at 09:49:49PM -0700, Lokesh Gidra wrote:
> > > Prior discussion about this can be found at [1].
> > >
> > > rmap_walk() requires all folios, except non-KSM anon, to be locked. This
> > > implies that when threads update folio->mapping to an anon_vma with
> > > different root (currently only done by UFFDIO MOVE), they have to
> > > serialize against rmap_walk() with write-lock on the anon_vma, hurting
> > > scalability. Furthermore, this necessitates rechecking anon_vma when
> > > pinning/locking an anon_vma (like in folio_lock_anon_vma_read()).
> > >
> > > This can be simplified quite a bit by ensuring that rmap_walk() is
> > > always called on locked folios. Among the few callers of rmap_walk() on
> > > unlocked anon folios, shrink_active_list()->folio_referenced() is the
> > > only performance critical one.
> > >
> > > shrink_active_list() doesn't act differently depending on what
> > > folio_referenced() returns for an anon folio. So returning 1 when it
> > > is contended, like in case of other folio types, wouldn't have any
> > > negative impact.
> > >
> > > Furthermore, as David pointed out in the previous discussion [2], this
> > > could potentially only affect R/O pages after fork as PG_anon_exclusive
> > > is not set. But, such folios are already isolated (prior to calling
> > > folio_referenced()) by grabbing a reference and clearing LRU, so
> > > do_wp_page()->wp_can_reuse_anon_folio() would not reuse such folios
> > > anyways.
> > >
> > > [1] https://lore.kernel.org/all/CA*EESO4Z6wtX7ZMdDHQRe5jAAS_bQ-POq5*4aDx5jh2DvY6UHg@mail.gmail.com
> > > [2] https://lore.kernel.org/all/dc92aef8-757f-4432-923e-70d92d13fb37@redhat.com
> > >
> > > 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/page_idle.c        |  8 ++------
> > >  mm/rmap.c             | 40 ++++++++++------------------------------
> > >  3 files changed, 16 insertions(+), 48 deletions(-)
> > >
> > > @@ -557,17 +554,6 @@ struct anon_vma *folio_lock_anon_vma_read(const struct folio *folio,
> > >       anon_vma = (struct anon_vma *) (anon_mapping - FOLIO_MAPPING_ANON);
> > >       root_anon_vma = READ_ONCE(anon_vma->root);
> > >       if (down_read_trylock(&root_anon_vma->rwsem)) {
> > > -             /*
> > > -              * folio_move_anon_rmap() might have changed the anon_vma as we
> > > -              * might not hold the folio lock here.
> > > -              */
> > > -             if (unlikely((unsigned long)READ_ONCE(folio->mapping) !=
> > > -                          anon_mapping)) {
> > > -                     up_read(&root_anon_vma->rwsem);
> > > -                     rcu_read_unlock();
> > > -                     goto retry;
> > > -             }
> > > -
> >
> > folio_lock_anon_vma_read() can be called without folio lock in a path:
> > memory_failure() -> kill_procs_now() -> collect_procs() ->
> > collect_procs_anon().
> >
>
> Thanks for catching this. Fell off the cracks for me.

No problem ;)

> > Not sure why collect_procs_{anon,ksm,file,fsdax} do not use rmap_walk()
> > functionality :/
> >
> > Should we take folio lock before calling kill_procs_now() in
> > memory_failure()?
> 
> To me it seems minimal (and sufficient) to put the collect_procs()
> call in kill_procs_now() inside folio lock's critical section.

Sounds sufficient to me.

-- 
Cheers,
Harry / Hyeonggon