Message-ID: <aK6rHBaQ-gpzgEVt@hyeyoo>
Date: Wed, 27 Aug 2025 15:52:12 +0900
From: Harry Yoo <harry.yoo@...cle.com>
To: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
Cc: Andrew Morton <akpm@...ux-foundation.org>,
        Suren Baghdasaryan <surenb@...gle.com>,
        "Liam R . Howlett" <Liam.Howlett@...cle.com>,
        David Hildenbrand <david@...hat.com>, Kees Cook <kees@...nel.org>,
        Vlastimil Babka <vbabka@...e.cz>,
        Shakeel Butt <shakeel.butt@...ux.dev>, Mike Rapoport <rppt@...nel.org>,
        Michal Hocko <mhocko@...e.com>, Jonathan Corbet <corbet@....net>,
        Jann Horn <jannh@...gle.com>, Pedro Falcato <pfalcato@...e.de>,
        Rik van Riel <riel@...riel.com>, linux-mm@...ck.org,
        linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH V1 2/2] mm: document when rmap locks can be skipped when
 setting need_rmap_locks

On Tue, Aug 26, 2025 at 10:46:24AM +0100, Lorenzo Stoakes wrote:
> On Tue, Aug 26, 2025 at 03:58:48PM +0900, Harry Yoo wrote:
> > While move_ptes() explains when rmap locks can be skipped, when reading
> > the code setting pmc.need_rmap_locks it is not immediately obvious when
> > need_rmap_locks can be false. Add a brief explanation in copy_vma() and
> > relocate_vma_down(), and add a pointer to the comment in move_ptes().
> >
> > Meanwhile, fix and improve the comment in move_ptes().
> >
> > Signed-off-by: Harry Yoo <harry.yoo@...cle.com>
> 
> This is great thanks! :)

You're welcome!

> > ---
> >  mm/mremap.c   | 4 +++-
> >  mm/vma.c      | 7 +++++++
> >  mm/vma_exec.c | 5 +++++
> >  3 files changed, 15 insertions(+), 1 deletion(-)
> >
> > diff --git a/mm/mremap.c b/mm/mremap.c
> > index e618a706aff5..86adb872bea0 100644
> > --- a/mm/mremap.c
> > +++ b/mm/mremap.c
> > @@ -218,8 +218,10 @@ static int move_ptes(struct pagetable_move_control *pmc,
> >  	 * When need_rmap_locks is false, we use other ways to avoid
> >  	 * such races:
> >  	 *
> > -	 * - During exec() shift_arg_pages(), we use a specially tagged vma
> > +	 * - During exec() relocate_vma_down(), we use a specially tagged vma
> >  	 *   which rmap call sites look for using vma_is_temporary_stack().
> > +	 *   Folios mapped in the temporary stack vma cannot be migrated until
> > +	 *   the relocation is complete.
> 
> Can we actually move this comment over to move_page_tables()? As this is
> relevant to the whole operation.

Sounds good, will do.

> Also could you put a comment referencing this
> comment in copy_vma_and_data() as this is where we actually determine whether
> this is the case or not in _most cases_.
>
> Let's just get all the 'need rmap locks' and 'corner cases where it's fine
> anyway' in one place that is logical :)

Will do.

> Also could you put a comment in copy_vma() over in mm/vma.c saying 'see the
> comment in mm/mremap.c' or even risk mentioning the function name (risky as code
> changes but still :P) e.g. 'see comment in move_page_tables()' or something.

Will take a risk and do "See the comment in move_page_tables()" :)

> I'm confused by the 'folios mapped' and 'migrate' bits - and I think people will
> be confused by that.
> 
> I think better to say 'page tables for the temporary stack cannot be adjusted
> until the relocation is complete'.

But is that correct?

Out of all rmap users, only try_to_migrate() cares about
VM_STACK_INCOMPLETE_SETUP via invalid_migration_vma().

I'm not sure what prevents from try_to_unmap() from unmapping it while
it's relocated?

Looks like it's always been like this since a8bef8ff6ea1 ("mm: migration:
avoid race between shift_arg_pages() and rmap_walk() during migration by
not migrating temporary stacks")

> >  	 *
> >  	 * - During mremap(), new_vma is often known to be placed after vma
> >  	 *   in rmap traversal order. This ensures rmap will always observe
> 
> This whole bit after could really do with some ASCII diagrams btw :)) ;) but you
> know maybe out of scope here.
> 
> > diff --git a/mm/vma.c b/mm/vma.c
> > index 3b12c7579831..3da49f79e9ba 100644
> > --- a/mm/vma.c
> > +++ b/mm/vma.c
> > @@ -1842,6 +1842,11 @@ struct vm_area_struct *copy_vma(struct vm_area_struct **vmap,
> >  	vmg.next = vma_iter_next_rewind(&vmi, NULL);
> >  	new_vma = vma_merge_new_range(&vmg);
> >
> > +	/*
> > +	 * rmap locks can be skipped as long as new_vma is traversed
> > +	 * after vma during rmap walk (new_vma->vm_pgoff >= vma->vm_pgoff).
> > +	 * See the comment in move_ptes().
> > +	 */
> 
> Obv. would prefer this to say 'move_page_tables()' as mentioned above :P

Will do.

> >  	if (new_vma) {
> >  		/*
> >  		 * Source vma may have been merged into new_vma
> > @@ -1879,6 +1884,8 @@ struct vm_area_struct *copy_vma(struct vm_area_struct **vmap,
> >  			new_vma->vm_ops->open(new_vma);
> >  		if (vma_link(mm, new_vma))
> >  			goto out_vma_link;
> > +
> > +		/* new_vma->pg_off is always >= vma->pg_off if not merged */
> 
> Err, new_vma is NULL? :) I'm not sure this comment is too useful.

Sometimes the line between "worth commenting" and "too much comment" is
vague to me :) I'll remove it. Thanks.

> >  		*need_rmap_locks = false;
> >  	}
> >  	return new_vma;
> > diff --git a/mm/vma_exec.c b/mm/vma_exec.c
> > index 922ee51747a6..a895dd39ac46 100644
> > --- a/mm/vma_exec.c
> > +++ b/mm/vma_exec.c
> > @@ -63,6 +63,11 @@ int relocate_vma_down(struct vm_area_struct *vma, unsigned long shift)
> >  	 * process cleanup to remove whatever mess we made.
> >  	 */
> >  	pmc.for_stack = true;
> > +	/*
> > +	 * pmc.need_rmap_locks is false since rmap locks can be safely skipped
> > +	 * because migration is disabled for this vma during relocation.
> > +	 * See the comment in move_ptes().
> > +	 */
> 
> Let's reword this also, people will get confused about migration here.
> 
> 'pmc.need_rmap_locks is false since rmap explicitly checks for
> vma_is_temporary_stack() and thus extra care does not need to be taken here
> during stack relocation. See the comment in move_page_tables().'

This looks good! except for one thing, not all rmap users check for
vma_is_temporary_stack().

> >  	if (length != move_page_tables(&pmc))
> >  		return -ENOMEM;
> >
> > --
> > 2.43.0
> >
> 
> Cheers, Lorenzo

-- 
Cheers,
Harry / Hyeonggon

Powered by blists - more mailing lists