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

On Mon, Aug 03, 2026 at 11:02:06AM -0700, Suren Baghdasaryan wrote:
> On Mon, Aug 3, 2026 at 3:50 AM Lorenzo Stoakes (ARM) <[email protected]> wrote:
> >
> > On Mon, Aug 03, 2026 at 09:48:19AM +0000, Alice Ryhl 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]>
> > >
> > > Reviewed-by: Alice Ryhl <[email protected]>
> >
> > Is there anything on the rust side that needs to be changed also?
>
> No. I think the corresponding function is rust_shrink_free_page() and
> it does not have mmap_fallback logic. IIUC, it just returns LRU_SKIP
> on failure.

OK great thanks :)

>
> >
> > --
> > Cheers, Lorenzo