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

[Lorenzo Stoakes <ljs@kernel.org>]

On Tue, May 12, 2026 at 08:56:02AM +0000, Alice Ryhl wrote:
> 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()").

Fair :)

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

Ack thanks!

>
> Alice