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

On Mon, Aug 10, 2026 at 02:00:14PM -0700, Suren Baghdasaryan wrote:

On Mon, Aug 10, 2026 at 12:16 PM Carlos Llamas [off-list ref] wrote:

quoted

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

quoted

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?

Ok, I'll amend the commit log with an explanation for this change.
Are you ok with keeping the other comment, or do you want it removed?

Ok, let's keep the comment. You are right that it is not obvious what
the intend of the binder_alloc_is_mapped() check is.

Thanks,
Carlos Llamas