Re: [PATCH] rust_binder: use lock_vma_under_rcu() in shrinker

[Alice Ryhl <aliceryhl@google.com>]

On Mon, May 11, 2026 at 02:19:42PM +0100, Lorenzo Stoakes wrote:
> On Thu, May 07, 2026 at 11:07:47AM +0000, Alice Ryhl wrote:
> > The shrinker callback currently uses the mmap read trylock operation to
> > attempt to access the vma, but it's generally better to only lock the
> > vma instead of the whole mmap when you can.
> >
> > When lock_vma_under_rcu() fails, there is no reason to lock the mmap
> > lock instead because it's already a trylock operation that is allowed to
> > fail.
> >
> > Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> 
> This seems similar to Dave's patch [0], not sure if it was inspired by that? :)
> 
> [0]:https://lore.kernel.org/all/20260429181957.7511C256@davehans-spike.ostc.intel.com/

I was reminded by that change, but it's been on my list since commit
a0b9b0f1433c ("rust_binder: use lock_vma_under_rcu() in
use_page_slow()").

> In any case the general approach seems sane to me, as rust code I can't really
> review it properly, but aside from the comment below (presumably that's fine) it
> conceptually LGTM so:
> 
> Acked-by: Lorenzo Stoakes <ljs@kernel.org>
> 
> > ---
> >  drivers/android/binder/page_range.rs | 24 +++++++++++-------------
> >  1 file changed, 11 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/android/binder/page_range.rs b/drivers/android/binder/page_range.rs
> > index e54a90e62402..e82a5523804f 100644
> > --- a/drivers/android/binder/page_range.rs
> > +++ b/drivers/android/binder/page_range.rs
> > @@ -705,7 +705,7 @@ fn drop(self: Pin<&mut Self>) {
> >      let page;
> >      let page_index;
> >      let mm;
> > -    let mmap_read;
> > +    let vma_read;
> >      let mm_mutex;
> >      let vma_addr;
> >      let range_ptr;
> > @@ -728,17 +728,18 @@ fn drop(self: Pin<&mut Self>) {
> >              None => return LRU_SKIP,
> >          };
> >
> > -        mmap_read = match mm.mmap_read_trylock() {
> > -            Some(guard) => guard,
> > -            None => return LRU_SKIP,
> > -        };
> > -
> >          // We can't lock it normally here, since we hold the lru lock.
> >          let inner = match range.lock.try_lock() {
> >              Some(inner) => inner,
> >              None => return LRU_SKIP,
> >          };
> >
> > +        vma_addr = inner.vma_addr;
> > +        vma_read = match mm.lock_vma_under_rcu(vma_addr) {
> > +            Some(guard) => guard,
> > +            None => return LRU_SKIP,
> > +        };
> > +
> 
> One question here - are we good to do this _after_ locking the 'inner' lock
> above?

Well, it's a spinlock so unless lock_vma_under_rcu() can sleep it should
be fine. Though we also hold *another* spinlock here, so if it can
sleep, we couldn't take it before `inner` either.

Alice