Re: [PATCH] mm: Always sanity check anon_vma first for per-vma locks

[Suren Baghdasaryan <surenb@google.com>]

[-- Attachment #1: Type: text/plain, Size: 1879 bytes --]

On Fri, Apr 12, 2024, 9:16 AM Matthew Wilcox <willy@infradead.org> 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.




>

[-- Attachment #2: Type: text/html, Size: 2786 bytes --]