Re: [PATCH v2 2/5] binder: Make shrinker rely solely on per-VMA lock

[Suren Baghdasaryan]

On Thu, Jun 11, 2026 at 12:53 AM Alice Ryhl <aliceryhl@xxxxxxxxxx> wrote:
>
> On Wed, Jun 10, 2026 at 04:04:13PM -0700, Dave Hansen wrote:
> >
> > From: Dave Hansen <dave.hansen@xxxxxxxxxxxxxxx>
> >
> > tl;dr: lock_vma_under_rcu() is already a trylock. No need to do both
> > it and mmap_read_trylock().
> >
> > Long Version:
> >
> > == Background ==
> >
> > Historically, binder used an mmap_read_trylock() in its shrinker code.
> > This ensures that reclaim is not blocked on an mmap_lock. Commit
> > 95bc2d4a9020 ("binder: use per-vma lock in page reclaiming") added
> > support for the per-VMA lock, but left mmap_read_trylock() as a
> > fallback.
> >
> > This was presumably because the per-VMA locking can fail for several
> > reasons and most (all?) lock_vma_under_rcu() callers have a fallback
> > to mmap_read_trylock().
> >
> > == Problem ==
> >
> > The fallback is not worth the complexity here. lock_vma_under_rcu() is
> > essentially already a non-blocking trylock. The main reason it fails
> > is also the reason mmap_read_trylock() fails: something is holding
> > mmap_write_lock().
> >
> > The only remedy for a collision with mmap_write_lock() is to wait,
> > which this code can not do. So the "fallback" after
> > lock_vma_under_rcu() failure is not really a fallback: it is really
> > likely to just be retrying in vain. That retry in an of itself isn't
> > horrible. But it adds complexity.
> >
> > == Solution ==
> >
> > Now that per-VMA locks are universally available, lock_vma_under_rcu()
> > will not persistently fail. Rely on it alone and simplify the code.
> >
> > Full disclosure: I originally tried to do this with
> > lock_vma_under_rcu_wait(), but it did not fit well with the mmap_lock
> > trylock semantics. Claude caught this in a review and suggested the
> > approach in this path. It seemed sane to me. So, Suggesed-by: Claude,
> > I guess.
> >
> > Signed-off-by: Dave Hansen <dave.hansen@xxxxxxxxxxxxxxx>
> > Reviewed-by: Suren Baghdasaryan <surenb@xxxxxxxxxx>
> > Acked-by: Lorenzo Stoakes <ljs@xxxxxxxxxx>
> > Cc: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
> > Cc: "Liam R. Howlett" <Liam.Howlett@xxxxxxxxxx>
> > Cc: Vlastimil Babka <vbabka@xxxxxxxxxx>
> > Cc: Shakeel Butt <shakeel.butt@xxxxxxxxx>
> > Cc: linux-mm@xxxxxxxxx
> > Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> > Cc: Arve Hjønnevåg <arve@xxxxxxxxxxx>
> > Cc: Todd Kjos <tkjos@xxxxxxxxxxx>
> > Cc: Christian Brauner <christian@xxxxxxxxxx>
> > Cc: Carlos Llamas <cmllamas@xxxxxxxxxx>
> > Cc: Alice Ryhl <aliceryhl@xxxxxxxxxx>
> > Cc: "David S. Miller" <davem@xxxxxxxxxxxxx>
> > Cc: David Ahern <dsahern@xxxxxxxxxx>
> > Cc: netdev@xxxxxxxxxxxxxxx
> >
> > --
> >
> > Changes from v1:
> >  * Move forward even if 'vma' is NULL in binder_alloc_free_page().
> >    This can happen if the VMA is unmapped (Sashiko).
> >  * Rename goto label to be more accurate for new lock scheme
> >
> >
> > ---
>
> This seems to include the list of changes in the commit message instead
> of under the --- line.
>
> >  b/drivers/android/binder_alloc.c |   26 +++++++++-----------------
> >  1 file changed, 9 insertions(+), 17 deletions(-)
> >
> > diff -puN drivers/android/binder_alloc.c~binder-try-vma-lock drivers/android/binder_alloc.c
> > --- a/drivers/android/binder_alloc.c~binder-try-vma-lock      2026-06-10 15:57:55.274412018 -0700
> > +++ b/drivers/android/binder_alloc.c  2026-06-10 15:57:55.277412124 -0700
> > @@ -1142,7 +1142,6 @@ enum lru_status binder_alloc_free_page(s
> >       struct vm_area_struct *vma;
> >       struct page *page_to_free;
> >       unsigned long page_addr;
> > -     int mm_locked = 0;
> >       size_t index;
> >
> >       if (!mmget_not_zero(mm))
> > @@ -1151,15 +1150,12 @@ enum lru_status binder_alloc_free_page(s
> >       index = mdata->page_index;
> >       page_addr = alloc->vm_start + index * PAGE_SIZE;
> >
> > -     /* attempt per-vma lock first */
> > +     /*
> > +      * Attempt per-vma lock. This is essentially a
> > +      * "trylock". It can fail even if the VMA exists
> > +      * for 'page_addr'.
> > +      */
> >       vma = lock_vma_under_rcu(mm, page_addr);
> > -     if (!vma) {
> > -             /* fall back to mmap_lock */
> > -             if (!mmap_read_trylock(mm))
> > -                     goto err_mmap_read_lock_failed;
> > -             mm_locked = 1;
> > -             vma = vma_lookup(mm, page_addr);
> > -     }
> >
> >       if (!mutex_trylock(&alloc->mutex))
> >               goto err_get_alloc_mutex_failed;
> > @@ -1188,13 +1184,11 @@ enum lru_status binder_alloc_free_page(s
> >               zap_vma_range(vma, page_addr, PAGE_SIZE);
> >
> >               trace_binder_unmap_user_end(alloc, index);
> > +
> > +             vma_end_read(vma);
> >       }
> >
> >       mutex_unlock(&alloc->mutex);
> > -     if (mm_locked)
> > -             mmap_read_unlock(mm);
> > -     else
> > -             vma_end_read(vma);
> >       mmput_async(mm);
> >       binder_free_page(page_to_free);
> >
> > @@ -1203,11 +1197,9 @@ enum lru_status binder_alloc_free_page(s
> >  err_invalid_vma:
> >       mutex_unlock(&alloc->mutex);
> >  err_get_alloc_mutex_failed:
> > -     if (mm_locked)
> > -             mmap_read_unlock(mm);
> > -     else
> > +     if (vma)
> >               vma_end_read(vma);
> > -err_mmap_read_lock_failed:
> > +err_vma_lock_failed:
> >       mmput_async(mm);
>
> If the vma lookup fails because the mmap write lock is held, but the vma
> actually exists (has not been unmapped), then this code might "successfully"
> remove the page without invoking zap_vma_range(). This means that the
> page does not actually get freed and will just hang around forever until
> the process owning the vma exits or Binder needs this page and maps a
> new page on top of the page.

Yeah, I think if lock_vma_under_rcu() returns NULL you just need to
jump to err_mmap_read_lock_failed, like we currently do if
mmap_read_trylock() fails.

>
> Alice