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! :)
> ---
> 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. 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 :)
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.
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'.
> *
> * - 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
> 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.
> *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().'
> if (length != move_page_tables(&pmc))
> return -ENOMEM;
>
> --
> 2.43.0
>
Cheers, Lorenzo
