Re: [PATCH v2] mm/vma: don't remove VMA from rmap if pgoff unchanged

["Lorenzo Stoakes (ARM)" <ljs@kernel.org>]

On Thu, Oct 01, 2026 at 11:45:11PM +0800, Lance Yang wrote:
>
> On Wed, Sep 30, 2026 at 06:53:36PM +0100, Lorenzo Stoakes (ARM) wrote:
> >When updating a VMA, vma_prepare() unconditionally removes it from its rmap
> >interval trees under the rmap lock, and vma_complete() reinserts it before
> >releasing the lock.
> >
> >This is wholly unnecessary if its page offset (file rmap) or anonymous page
> >offset (anon rmap) is unchanged.
> >
> >So, track whether they will change in the newly introduced
> >vp->file_pgoff_unchanged and vp->anon_pgoff_unchanged fields, and use them
> >to determine whether to remove the VMA or not.
> >
> >The rmap lock keeps things safe as no rmap walks can concurrently occur
> >during the operation.
> >
> >Additionally, some architectures (arm, parisc, nios2, csky) have dcache
> >flush rmap walkers which take only flush_dcache_mmap_lock(), which is
> >likewise held across the operation.
> >
> >If the VMA remains in the tree, it's necessary to keep the augmented
> >rb_subtree_last field updated to reflect its changed range.
> >
> >Provide mapping_rmap_tree_[pre, post]_update() and
> >anon_rmap_tree_[pre, post]_update_vma() (replacing the existing logic in
> >the anonymous case) to handle both the changed and unchanged cases.
> >
> >For the anon rmap case, with CONFIG_DEBUG_VM_RB set, avc->cached_vma_last
> >is also updated when propagating in place.
> >
> >When performing a VMA shrink or a split where the VMA is the lower one, the
> >page offset cannot change, so set the flags unconditionally in these cases.
> >
> >When merging VMAs the page offset is unchanged only in some cases, so
> >update init_multi_vma_prep() to set the flags only if the page offsets
> >remain the same.
> >
> >Finally, while we're here, also update expand_upwards() similarly.
> >
> >These changes ultimately result in less rmap lock contention.
> >
> >Pan Deng reported results using the UnixBench/excel benchmark on a 2-socket
> >192 core, 384 thread x86-64 system for v7.3-rc4 with/without the patch
> >applied:
> >
> >Execl Throughput, index score:
> >
> >                    avg    %stdev      min        max
> >  v7.3-rc4        3511.5     0.44%    3494.5    3543.4
> >  + patch         4069.0     0.48%    4047.4    4109.5 (+15.9%)
> >
> >Average wait on file rmap lock in ms, 5 runs per kernel:
> >
> >               avg      %stdev     min       max
> >  v7.3-rc4    9.470     2.91%     9.070     9.820
> >  + patch     8.420     3.34%     8.100     8.770 (-11.1%)
> >
> >Profiling data obtained during the operation highlighted the file rmap lock
> >as the primary source of contention.
> >
> >Suggested-by: Pan Deng <pan.deng@intel.com>
> >Reviewed-by: Rik van Riel <riel@surriel.com>
> >Signed-off-by: Lorenzo Stoakes (ARM) <ljs@kernel.org>
> >---
>
> Wow, pretty cool stuff. That's a nice speedup!
>
> [...]
> >+static void anon_rmap_tree_update_inplace(struct anon_vma_chain *avc)
> >+{
> >+#ifdef CONFIG_DEBUG_VM_RB
> >+	avc->cached_vma_last = avc_last_pgoff(avc);
> >+#endif
> >+	/* Propagate all the way up the tree. */
>
> Nit: propagate() can stop early when rb_subtree_last is unchanged ...
>
> Maybe:
>
> /* Update the subtree maximum and propagate any changes up the tree. */

I think in this case it's ok to be a bit blurry about it :P it deciding not to
unnecessary work is fine but I don't want to put too much in there.

The point is as a simple sign or pointer to help somebody wondering wtf that's
for even if it's not quite the full story!

Hopefully that's ok? :)

>
> >+	__anon_rmap_tree_augment.propagate(&avc->rb, NULL);
> >+}
> >+
> [...]
>
> Acked-by: Lance Yang <lance.yang@linux.dev>

Thanks :)

>
> Hammered it with VMA churn (split/merge/mremap/madvise/fork) + concurrent
> rmap walks + hwpoison injection. Nothing complained :D
>
> Tested-by: Lance Yang <lance.yang@linux.dev>

Thanks, very much appreciated! :)

>
> Cheers, Lance

--
Cheers, Lorenzo