Re: [PATCH v2 2/5] binder: Make shrinker rely solely on per-VMA lock
From: Alice Ryhl <aliceryhl@google.com>
Date: 2026-06-11 07:53:47
Also in:
linux-mm, lkml
On Wed, Jun 10, 2026 at 04:04:13PM -0700, Dave Hansen wrote:
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.
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>
Reviewed-by: Suren Baghdasaryan <surenb@google.com>
Acked-by: Lorenzo Stoakes <ljs@kernel.org>
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
--
Changes from v1:
* Move forward even if 'vma' is NULL in binder_alloc_free_page().
This can happen if the VMA is unmapped (Sashiko).
* Rename goto label to be more accurate for new lock scheme
---This seems to include the list of changes in the commit message instead of under the --- line.
quoted hunk ↗ jump to hunk
b/drivers/android/binder_alloc.c | 26 +++++++++----------------- 1 file changed, 9 insertions(+), 17 deletions(-) diff -puN drivers/android/binder_alloc.c~binder-try-vma-lock drivers/android/binder_alloc.c--- a/drivers/android/binder_alloc.c~binder-try-vma-lock 2026-06-10 15:57:55.274412018 -0700 +++ b/drivers/android/binder_alloc.c 2026-06-10 15:57:55.277412124 -0700@@ -1142,7 +1142,6 @@ enum lru_status binder_alloc_free_page(s 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,15 +1150,12 @@ enum lru_status binder_alloc_free_page(s 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'. + */ 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 (!mutex_trylock(&alloc->mutex)) goto err_get_alloc_mutex_failed;@@ -1188,13 +1184,11 @@ enum lru_status binder_alloc_free_page(s zap_vma_range(vma, page_addr, PAGE_SIZE); trace_binder_unmap_user_end(alloc, index); + + vma_end_read(vma); } mutex_unlock(&alloc->mutex); - if (mm_locked) - mmap_read_unlock(mm); - else - vma_end_read(vma); mmput_async(mm); binder_free_page(page_to_free);@@ -1203,11 +1197,9 @@ enum lru_status binder_alloc_free_page(s err_invalid_vma: mutex_unlock(&alloc->mutex); err_get_alloc_mutex_failed: - if (mm_locked) - mmap_read_unlock(mm); - else + if (vma) vma_end_read(vma); -err_mmap_read_lock_failed: +err_vma_lock_failed: mmput_async(mm);
If the vma lookup fails because the mmap write lock is held, but the vma actually exists (has not been unmapped), then this code might "successfully" remove the page without invoking zap_vma_range(). This means that the page does not actually get freed and will just hang around forever until the process owning the vma exits or Binder needs this page and maps a new page on top of the page. Alice