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

On Mon, Aug 3, 2026 at 4:10=E2=80=AFAM Lorenzo Stoakes (ARM) <[email protected]=
g> wrote:
>
> On Sun, Aug 02, 2026 at 02:54:56PM -0700, Suren Baghdasaryan wrote:
> > From: Dave Hansen <[email protected]>
> >
> > tl;dr: lock_vma_under_rcu() is already a trylock. No need to do both
> > it and mmap_read_trylock().
> >
> > Long Version:
> >
> > =3D=3D Background =3D=3D
> >
> > 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().
> >
> > =3D=3D Problem =3D=3D
> >
> > 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.
> >
> > =3D=3D Solution =3D=3D
> >
> > 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 <[email protected]>
> > Signed-off-by: Suren Baghdasaryan <[email protected]>
> > Cc: Andrew Morton <[email protected]>
> > Cc: "Liam R. Howlett" <[email protected]>
> > Cc: Vlastimil Babka <[email protected]>
> > Cc: Shakeel Butt <[email protected]>
> > Cc: [email protected]
> > Cc: Greg Kroah-Hartman <[email protected]>
> > Cc: Arve Hj=C3=B8nnev=C3=A5g <[email protected]>
> > Cc: Todd Kjos <[email protected]>
> > Cc: Christian Brauner <[email protected]>
> > Cc: Carlos Llamas <[email protected]>
> > Cc: Alice Ryhl <[email protected]>
> > Cc: "David S. Miller" <[email protected]>
> > Cc: David Ahern <[email protected]>
> > Cc: [email protected]
> > ---
> >  drivers/android/binder_alloc.c | 29 +++++++++++++++--------------
> >  1 file changed, 15 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/android/binder_alloc.c b/drivers/android/binder_al=
loc.c
> > index e4488ad86a65..84104ba04e30 100644
> > --- a/drivers/android/binder_alloc.c
> > +++ b/drivers/android/binder_alloc.c
> > @@ -1142,7 +1142,6 @@ enum lru_status binder_alloc_free_page(struct lis=
t_head *item,
> >       struct vm_area_struct *vma;
> >       struct page *page_to_free;
> >       unsigned long page_addr;
> > -     int mm_locked =3D 0;
> >       size_t index;
> >
> >       if (!mmget_not_zero(mm))
> > @@ -1151,14 +1150,20 @@ enum lru_status binder_alloc_free_page(struct l=
ist_head *item,
> >       index =3D mdata->page_index;
> >       page_addr =3D 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'.
> > +      */
>
> This makes me wonder whether lock_vma_under_rcu() should really become
> vma_trylock() at some point in time? :)
>
> Or at least have 'trylock' in the name.

Makes sense. I think I'll postpone renames until after the series are
merged. Don't want to mix too many changes together.

>
> >       vma =3D 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 =3D 1;
> > -             vma =3D vma_lookup(mm, page_addr);
> > +             /*
> > +              * If the vma exists, we can't continue because we cannot
> > +              * remove the page from the vma. However, if the vma was
> > +              * unmapped, it's okay to continue.
> > +              */
> > +             if (binder_alloc_is_mapped(alloc))
> > +                     goto err_vma_lock_failed;
>
> Hmm, it seems a bit odd to me that you also have:
>
>         if (vma && !binder_alloc_is_mapped(alloc))
>                 goto err_invalid_vma;
>
> Below?
>
> So you have:
>
> Before:
>
>                                 |binder_alloc_is_mapped()?
>                                 |yes    no
>                         --------|-----------------
>         vma is mapped?  yes     |OK     abort
>                         no      |OK     OK
>
> Now:
>
>                                 |binder_alloc_is_mapped()?
>                                 |yes    no
>                         --------|-----------------
>         vma is mapped? maybe    |abort  OK
>                         yes     |OK     abort
>                         no      |OK     OK
>
> The 'maybe' is because the VMA trylock failed.

The case you are considering is lock_vma_under_rcu() failed for some
reason other than lock contention (say seqno overflow). In that case
we get vma=3D=3DNULL and binder_alloc_is_mapped() is called without any
lock (VMA or mmap lock) being held. I'm not sure if this is a real
problem since I see other places calling binder_alloc_is_mapped()
without locking. Alice, Carlos, is this a problem?

>
> So the issue is you might have a case where the VMA _is_ mapped but
> !binder_alloc_is_mapped(), which previously aborted because of the vma &&
> !binder_alloc_is_mapped() check.
>
> It seems like:
>
>         /*
>          * Since a binder_alloc can only be mapped once, we ensure
>          * the vma corresponds to this mapping by checking whether
>          * the binder_alloc is still mapped.
>          */
>         if (vma && !binder_alloc_is_mapped(alloc))
>                 goto err_invalid_vma;
>
> Is testing for a specific scenario 'we found a VMA but it turns out it's
> invalid' and aborting if so.
>
> So either this check should be removed or you should uncondtionally abort=
 if
> !vma I think?

I think this check is fine because it basically checks
binder_alloc_is_mapped() after stabilizing the address range.
Unconditionally aborting if !vma would prevent us from freeing the
page if the VMA was already unmapped. The ultimate question is whether
we can rely on binder_alloc_is_mapped() alone when freeing that page.
IOW, VMA might still be in the VMA tree but
binder_alloc_is_mapped()=3D=3Dfalse, can we free the page?

>
>
> >       }
> >
> >       if (!mutex_trylock(&alloc->mutex))
> > @@ -1191,9 +1196,7 @@ enum lru_status binder_alloc_free_page(struct lis=
t_head *item,
> >       }
> >
> >       mutex_unlock(&alloc->mutex);
> > -     if (mm_locked)
> > -             mmap_read_unlock(mm);
> > -     else
> > +     if (vma)
> >               vma_end_read(vma);
> >       mmput_async(mm);
> >       binder_free_page(page_to_free);
> > @@ -1203,11 +1206,9 @@ enum lru_status binder_alloc_free_page(struct li=
st_head *item,
> >  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);
> >  err_mmget:
> >       return LRU_SKIP;
> > --
> > 2.55.0.508.g3f0d502094-goog
> >
>
> --
> Cheers, Lorenzo