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@xxxxxxxxxx>
>
> 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
