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

On Tue, Aug 4, 2026 at 2:12 AM Lorenzo Stoakes (ARM) <[email protected]> wrote:
>
> On Tue, Aug 04, 2026 at 09:04:20AM +0000, Alice Ryhl wrote:
> > On Mon, Aug 03, 2026 at 12:10:21PM +0100, Lorenzo Stoakes (ARM) 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:
> > > >
> > > > == 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 <[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ønnevåg <[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_alloc.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 list_head *item,
> > > >   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,14 +1150,20 @@ enum lru_status binder_alloc_free_page(struct list_head *item,
> > > >   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'.
> > > > +  */
> > >
> > > 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.
> > >
> > > >   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 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.
> > >
> > > 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?
> >
> > This check is quite important and can't just be removed. If you remove
> > it, there's no guarantee that the vma is one created by Binder. It might
> > as well be a VMA from a completely different driver/subsystem, which we
> > definitely should not be invoking zap_vma_range() on.
> >
> > In this case, Binder rules out that scenario by saying that the VMA
> > can be mapped exactly once, and once you unmap it or remap it or
> > anything like that, Binder sets the 'is_mapped' boolean to false and
> > refuses to perform any further VMA operations for this binder fd.
> >
> > So really this function needs to deal with three scenarios:
> >
> > 1. The original Binder VMA is still there and we acquired its lock.
> > 2. The original Binder VMA is still there, but we could not acquire its
> >    lock.
> > 3. The original Binder VMA is gone.
> >    - Subcase one: there is no VMA at that location anymore.
> >    - Subcase two: there is now another unrelated VMA at that location.
> >
> > In scenario one we can proceed with zapping the page. In scenario two we
> > must return LRU_SKIP because we are unable to zap the page. As for
> > scenario three, it's a scenario that is possible, but not something that
> > needs to work well. It doesn't matter that much whether such pages can
> > be reclaimed by the shrinker because userspace shouldn't create this
> > scenario to begin with.
> >
> > But you are right that we currently handle scenario 3 inconsistently. We
> > handle subcase one by having the shrinker proceed to free the page, and
> > just skip the zap_vma_range() call. And we handle subcase two by having
> > the shrinker return LRU_SKIP. Either behavior is acceptable to me, but I
> > agree that being consistent would be better.
> >
> > So what we could do is to remove this check, but then later wrap
> > zap_vma_range() in an 'is_mapped' check like this:
> >
> >       if (vma && binder_alloc_is_mapped(alloc)) {
> >               zap_vma_range(vma, page_addr, PAGE_SIZE);
> >       }
> >
> > This way we only LRU_SKIP in case two, and always handle case 3 by
> > removing the page from alloc->pages without touching the VMA.
>
> Ah sorry I replied to Suren not noticing you'd responded :)
>
> Thanks for the explanation, much appreciated! Makes sense.
>
> I'm being OCD about it (occupational hazard in kernel development :) but the
> inconsitency was concerning there. And agreed it's an edge case.
>
> Suren - that good for the respin?

Yes, I'll incorporate Alice's suffestion into the next version. Thanks!
>
> >
> > Alice
>
> --
> Cheers, Lorenzo