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

On Mon, Aug 10, 2026 at 11:54:04AM -0700, Suren Baghdasaryan wrote:

On Mon, Aug 10, 2026 at 11:32 AM Carlos Llamas [off-list ref] wrote:

quoted

On Thu, Aug 06, 2026 at 01:05:45PM -0700, Suren Baghdasaryan wrote:

quoted

From: Dave Hansen <dave.hansen@linux.intel.com>

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.
The removal of the fallback does not affect NOMMU case because binder
driver depends on CONFIG_MMU.

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 <dave.hansen@linux.intel.com>
Signed-off-by: Suren Baghdasaryan <surenb@google.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: "Liam R. Howlett" <redacted>
Cc: Vlastimil Babka <vbabka@kernel.org>
Cc: Shakeel Butt <shakeel.butt@linux.dev>
Cc: linux-mm@kvack.org
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Arve Hjønnevåg <arve@android.com>
Cc: Todd Kjos <tkjos@android.com>
Cc: Christian Brauner <christian@brauner.io>
Cc: Carlos Llamas <cmllamas@google.com>
Cc: Alice Ryhl <aliceryhl@google.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: David Ahern <dsahern@kernel.org>
Cc: netdev@vger.kernel.org
---
 drivers/android/binder_alloc.c | 45 ++++++++++++++++------------------
 1 file changed, 21 insertions(+), 24 deletions(-)

diff --git a/drivers/android/binder_alloc.c b/drivers/android/binder_alloc.c
index e4488ad86a65..c13a588c37de 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,27 +1150,25 @@ 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'.
+      */

Do we need to explain how lock_vma_under_rcu() works here?

quoted

      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;

The comments seem redundant, the label is enough. This works:

        vma = lock_vma_under_rcu(mm, page_addr);
        if (!vma && binder_alloc_is_mapped(alloc))
                goto err_vma_lock_failed;

Yeah, for the binder maintainers what's happening here is probably
obvious, but when Alice explained the logic to me, this comment really
clarified what's going on, so I added it here. If you insist on
removing it, I'll do that of course.

quoted

quoted

      }

      if (!mutex_trylock(&alloc->mutex))
              goto err_get_alloc_mutex_failed;

-     /*
-      * 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;
-

This introduces an "extra" change. We'll now release pages without a
valid vma (e.g. after munmap()). Before, these pages were expected to be
released via close() in binder_alloc_deferred_release() later.

I don't see anything wrong with it. However, it does seem out of the
scope of this patch which just drops the mmap_read_lock() calls. Or at
least I don't see how this part is necessary.

This check came from this discussion:
https://lore.kernel.org/all/anGrFIYiPhjxWSkD@google.com/ (local)
It's added for consistency when handling cases where the original
Binder VMA is gone.

Oh sorry I missed that thread. Yes, I agree we can reclaim these pages
earlier if really needed but my point is this is unrelated to the change
done here IMO. I couldn't figure out why that was needed.

If you are keeping that in the same commit it's probably worth adding an
explanation to the commit log?

--
Carlos Llamas