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

[Suren Baghdasaryan]

On Fri, May 8, 2026 at 10:07 AM Lorenzo Stoakes <ljs@xxxxxxxxxx> wrote:
>
> On Wed, Apr 29, 2026 at 11:19:57AM -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 but left mmap_read_trylock() as a

s/but but/but

> > 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().

The fallback in most cases (I think in all cases except binder) is
mmap_read_lock()/mmap_read_lock_killable() so that we can sleep and
wait until the lock becomes available. Even get_mmap_lock_carefully()
at the end does mmap_read_lock_killable() if mmap_lock is contended.

The fallback in the binder is trylock because they don't want to block
global reclaim inside their shrinker. At least that's my
understanding.
CC'ing Carlos Llamas to chime in if we are incorrect here. An Ack from
a binder expert would be nice too.

> >
> > == 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.

Yep, makes sense. There is that sequence counter overflow case that
might cause lock_vma_under_rcu() to fail but that's a really rare
case, not worth considering.

> >
> > == 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>
>
> I mean this seems reasonable to me, I don't really understand why we'd fall back
> to mmap... try lock :)
>
> If semantically you're trylocking then as you say, lock_vma_under_rcu() already
> does that.
>
> Honestly I feel this could be submitted separate from the series.
>
> I'm not a binder guy, but this looks right to me so:
>
> Acked-by: Lorenzo Stoakes <ljs@xxxxxxxxxx> with nit below addressed.

Reviewed-by: Suren Baghdasaryan <surenb@xxxxxxxxxx>

>
> > Cc: Suren Baghdasaryan <surenb@xxxxxxxxxx>
> > Cc: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
> > Cc: "Liam R. Howlett" <Liam.Howlett@xxxxxxxxxx>
> > Cc: Lorenzo Stoakes <ljs@xxxxxxxxxx>
> > Cc: Vlastimil Babka <vbabka@xxxxxxxxxx>
> > Cc: Shakeel Butt <shakeel.butt@xxxxxxxxx>
> > Cc: linux-mm@xxxxxxxxx
> > ---
> >
> >  b/drivers/android/binder_alloc.c |   22 +++++-----------------
> >  1 file changed, 5 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-04-29 11:18:50.066607065 -0700
> > +++ b/drivers/android/binder_alloc.c  2026-04-29 11:18:50.069607180 -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;
>
> Man why are we using int instead of a bool in 2026 in the first place :P
>
> >       size_t index;
> >
> >       if (!mmget_not_zero(mm))
> > @@ -1151,15 +1150,10 @@ 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 */
> >       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 (!vma)
> > +             goto err_mmap_read_lock_failed;
>
> Nit, but we probably want to rename that to err_vma_lock_failed or something!
>
> >
> >       if (!mutex_trylock(&alloc->mutex))
> >               goto err_get_alloc_mutex_failed;
> > @@ -1191,10 +1185,7 @@ enum lru_status binder_alloc_free_page(s
> >       }
> >
> >       mutex_unlock(&alloc->mutex);
> > -     if (mm_locked)
> > -             mmap_read_unlock(mm);
> > -     else
> > -             vma_end_read(vma);
> > +     vma_end_read(vma);
> >       mmput_async(mm);
> >       binder_free_page(page_to_free);
> >
> > @@ -1203,10 +1194,7 @@ 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
> > -             vma_end_read(vma);
> > +     vma_end_read(vma);
> >  err_mmap_read_lock_failed:
> >       mmput_async(mm);
> >  err_mmget:
> > _