Re: [PATCH V1 1/2] docs/mm: explain when and why rmap locks need to be taken during mremap()

[Lorenzo Stoakes <lorenzo.stoakes@oracle.com>]

On Tue, Aug 26, 2025 at 03:58:47PM +0900, Harry Yoo wrote:
> While move_ptes() has a comment explaining why rmap locks are needed,
> Documentation/mm/process_addrs.rst does not. Without being aware of that
> comment, I spent hours figuring out how things could go wrong and why,
> in some cases, rmap locks can be safely skipped.
>
> Add a more comprehensive explanation to the documentation to save time
> for others.
>
> Signed-off-by: Harry Yoo <harry.yoo@oracle.com>

Again great, but needs some fix ups, see below!

> ---
>  Documentation/mm/process_addrs.rst | 32 ++++++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
>
> diff --git a/Documentation/mm/process_addrs.rst b/Documentation/mm/process_addrs.rst
> index be49e2a269e4..ee7c0dba339e 100644
> --- a/Documentation/mm/process_addrs.rst
> +++ b/Documentation/mm/process_addrs.rst
> @@ -744,6 +744,38 @@ You can observe this in the :c:func:`!mremap` implementation in the functions
>  :c:func:`!take_rmap_locks` and :c:func:`!drop_rmap_locks` which perform the rmap
>  side of lock acquisition, invoked ultimately by :c:func:`!move_page_tables`.
>

> +.. note:: If :c:func:`!mremap()` -> :c:func:`!move_ptes()` does not take rmap
> +          locks, :c:func:`!rmap_walk()` may miss a pte for the folio.
> +
> +          The problematic sequence is as follows:
> +
> +          1. :c:func:`!rmap_walk()` checks the destination VMA, finds no pte,
> +             and releases the page table lock.
> +          2. :c:func:`!move_ptes()` moves the page tables from the source to the
> +             destination.
> +          3. :c:func:`!rmap_walk()` checks the source VMA, finds no pte, and
> +             thus rmap walk misses it.
> +
> +          Taking rmap locks in :c:func:`!move_ptes()` ensures that
> +          :c:func:`!rmap_walk()` sees the pte in either the source or
> +          destination VMA.
> +
> +          There are two cases where rmap locks can be skipped:
> +
> +          1. If the source VMA is guaranteed to be visited before the
> +             destination VMA during rmap walk, :c:func:`!rmap_walk()` will
> +             encounter the pte in one of the two VMAs. VMAs associated with
> +             an anon_vma are organized in an interval tree, so the src->dst
> +             order is guaranteed when the source VMA's vm_pgoff precedes
> +             the destination VMA's vm_pgoff.
> +
> +          2. When :c:func:`!exec()` relocates a temporary stack VMA via
> +             :c:func:`!relocate_vma_down()`, there is no separate destination
> +             VMA. Instead, the source VMA is marked as a temporary stack and
> +             relocated. In this case, the folios belonging to the VMA cannot be
> +             migrated until the relocation is complete, avoiding the need to

Again, let's not say migrated, or refer to folios. 'In this case page
tables cannot be modified until...' I'd say something about rmap will check
vma_is_temporary_stack() so there's no need to acquire rmap locks in this
instance.

> +             acquire rmap locks for performance reasons.
> +

This is the wrong place for this.

Here I'm talking about the rmap lock _override_ for instances where we move >=
PMD, you're talking more generally.

So let's move things around a bit:

	Some functions manipulate page table levels above PMD (that is PUD,
	P4D and PGD page tables). Most notable of these is mremap(), which
	is capable of moving higher level page tables.

	-> insert here.

	In these instances, it is required that all locks are taken, that is the
	mmap lock, the VMA lock and the relevant rmap locks.

	^--- then here reword this to:

'In instances where higher level page tables are moved, it is required...'

>  VMA lock internals
>  ------------------
>
> --
> 2.43.0
>

Cheers, Lorenzo