On Fri, Apr 12, 2024, 9:16 AM Matthew Wilcox <willy@xxxxxxxxxxxxx> wrote:
On Fri, Apr 12, 2024 at 06:06:46AM -0700, Suren Baghdasaryan wrote:
> > On Fri, Apr 12, 2024 at 04:14:16AM +0100, Matthew Wilcox wrote:
> > > - /*
> > > - * find_mergeable_anon_vma uses adjacent vmas which are not locked.
> > > - * This check must happen after vma_start_read(); otherwise, a
> > > - * concurrent mremap() with MREMAP_DONTUNMAP could dissociate the VMA
> > > - * from its anon_vma.
> > > - */
> > > - if (unlikely(vma_is_anonymous(vma) && !vma->anon_vma))
> > > - goto inval_end_read;
> > > -
> > > /* Check since vm_start/vm_end might change before we lock the VMA */
> > > if (unlikely(address < vma->vm_start || address >= vma->vm_end))
> > > goto inval_end_read;
> > >
> > > That takes a few insns out of the page fault path (good!) at the cost
> > > of one extra trip around the fault handler for the first fault on an
> > > anon vma. It makes the file & anon paths more similar to each other
> > > (good!)
> > >
> > > We'd need some data to be sure it's really a win, but less code is
> > > always good.
>
> I agree, if we make this change we should keep this comment and maybe
> move it into vmf_anon_prepare()
Most of the comment makes no sense if you move it out of
lock_vma_under_rcu(). It's justifying where it needs to be in that
function. If it's no longer in that function, there's not much left of
the comment. What part do you think is valuable and needs to be retained?
Unless vmf_anon_prepare() already explains why vma->anon_vma poses a problem for per-vma locks, we should have an explanation there. This comment would serve that purpose IMO.
