Re: [PATCH] mm/vma: avoid redundant file rmap tree re-insert on new_below=0 split

[Pedro Falcato <pfalcato@suse.de>]

On Fri, Sep 25, 2026 at 04:27:44PM +0100, Lorenzo Stoakes (ARM) wrote:
> > This change skips the re-insert for that case.  vma_prepare() no longer
> > removes vp->vma from the tree; instead vma_complete() detects that case and
> > only recomputes shared.rb_subtree_last up the ancestor chain.  Everything
> > else keeps the remove + re-insert path.
> 
> This could really do with a diagram and a simple explanation.
> 
> In general you should rewrite the entire commit message yourself and not
> use the LLM output at all.

+1 on this. Even with the Assisted-by, this needs to be understandable by
hoomans.

> 
> >
> > The case is detected by comparing vma_start_pgoff(vp->insert) against
> > vma_start_pgoff(vp->vma): only a new_below=0 split leaves the former
> > greater.  __split_vma() adjusts pgoff via vma_add_pgoff(new,
> > linear_page_delta(vma, addr)), and linear_page_delta() is
> > (addr - vm_start) >> PAGE_SHIFT with addr strictly inside the VMA, so the
> > delta is at least one page and the new VMA's pgoff is strictly greater.
> > For new_below=1 the two are initially equal and vp->vma's pgoff then
> > increases, so the comparison is false both before and after the caller's
> > endpoint updates, and the original path is taken.
> 
> This is useless description of the code in English, it's not telling me
> anything useful at all. Human beings don't have a large 'stack' with which
> to hold things in our minds.
> 
> Write with humans in mind please :)
> 
> >
> > Inferring the case this way keeps the change small: struct vma_prepare
> > gains no field and no caller changes.
> 
> I don't understand what inferring the case means? I think this can be
> dropped.
> 
> >
> > Moving the remove() out of vma_prepare() leaves vp->vma in the tree with a
> > possibly stale sort key across the caller's endpoint updates, so
> 
> I mean what?
> 
> What does a 'possibly stale sort key' mean? And what does 'across the
> caller's endpoint updates' mean? Which caller? What's an endpoint? What
> updates?
> 
> > vma_complete() re-keys it *before* inserting vp->adj_next: otherwise that
> > key-driven descent could place adj_next in the wrong subtree.
> 
> Re-keys what? WHat does re-key mean?
> 
> What you're saying here really makes me nervous. You're changing a very
> sensitive part of the kernel and talking about intentionally leaving stale
> state around.
> 
> This patch cannot possibly be considered for upstream until I fully
> understand exactly what this means and that you understand what you're
> doing.
> 
> >
> > Measured on v7.3-rc4, on a 2-socket 192C/384T system running UnixBench
> > execl (384 concurrent execve of the same binary), dropping the redundant
> > remove + re-insert yields ~14% higher throughput by shortening the
> > i_mmap_rwsem write-side critical section during the file VMA splits that
> > execve performs on the shared libraries.

For what it's worth, I'm vaguely accepting of a similar change, but this needs
to be _really_ well commented out, and ideally in file rmap code, _not_
spaghetti'd in VMAs. The interval tree is complicated and some bits are not
very intuitive. This needs to be robust. Not LLM'd into existence.

This also reminds me that I should reboot the sharded file rmap effort...

> 
> This is really unconvincing I'm sorry. Real numbers please with statistical
> evidence to back them.
> 
> Additionally I notice you have not made one comment referring to locking
> anywhere.
> 
> This part of the kernel has VERY subtle and sensitive locking
> requirements. I am not convinced you understand this, either.
> 
> >
> > Signed-off-by: Pan Deng <pan.deng@intel.com>
> 
> This patch feels like a hack. You are creating a whole new set of very
> fragile assumptions that have to be maintained throughout.
> 
> Again as above, a member of the core team should take this over.
> 
> > Reviewed-by: Tianyou Li <tianyou.li@intel.com>
> > Reviewed-by: Wangyang Guo <wangyang.guo@intel.com>
> > Reviewed-by: Zhiguo Zhou <zhiguo.zhou@intel.com>
> > Reviewed-by: Tim Chen <tim.c.chen@linux.intel.com>
> 
> Please don't do this.
> 
> Upstream is not interested in private reviews. Review tags upstream are
> based on review done in _public_.

Yeah, this too. It's just noise.

-- 
Pedro