github-mirror/linux
1
0
Fork 0
mirror of https://github.com/torvalds/linux.git synced 2026-11-06 21:58:01 +01:00
Code Issues Actions Packages Projects Releases Wiki Activity Graph

binder: make shrinker rely solely on per-VMA lock

Browse Source 
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
95bc2d4a90 ("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.

While at it we also make the handling of the cases where the original
binder VMA is gone consistent. There are two cases to consider when
Binder VMA is gone:
1. there is no VMA at that location anymore.
2. there is now another unrelated VMA at that location.

Before this change we handle case 1 by having the shrinker proceed to free
the page, and just skip the zap_vma_range() call.  And we handle case 2 by
having the shrinker return LRU_SKIP.  While either behavior is acceptable,
we need to handle them in a consistent way.  Handle both cases by freeing
the page without touching the VMA (skipping the zap_vma_range()).

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.

Link: https://lore.kernel.org/20260831203056.838265-3-surenb@google.com
Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
Signed-off-by: Suren Baghdasaryan <surenb@google.com>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
Reviewed-by: Alice Ryhl <aliceryhl@google.com>
Acked-by: Lorenzo Stoakes (ARM) <ljs@kernel.org>
Acked-by: Carlos Llamas <cmllamas@google.com>
Cc: Liam R. Howlett <liam@infradead.org>
Cc: Vlastimil Babka <vbabka@kernel.org>
Cc: Shakeel Butt <shakeel.butt@linux.dev>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Todd Kjos <tkjos@android.com>
Cc: Christian Brauner <christian@brauner.io>
Cc: David S. Miller <davem@davemloft.net>
Cc: David Ahern <dsahern@kernel.org>
Cc: Arve Hjønnevåg <arve@android.com>
Cc: David Hildenbrand (Arm) <david@kernel.org>
This commit is contained in:
Dave Hansen 2026-08-31 13:30:53 -07:00 committed by Andrew Morton
parent 47e725702b
commit 5542d17a21
1 changed files with 21 additions and 25 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

46
drivers/android/binder_alloc.c
View File

@ -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'.
*/
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;
}
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;
trace_binder_unmap_kernel_start(alloc, index);
page_to_free = alloc->pages[index];
@ -1182,7 +1179,12 @@ enum lru_status binder_alloc_free_page(struct list_head *item,
list_lru_isolate(lru, item);
spin_unlock(&lru->lock);
if (vma) {
/*
* 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)) {
trace_binder_unmap_user_start(alloc, index);
zap_vma_range(vma, page_addr, PAGE_SIZE);
@ -1191,23 +1193,17 @@ enum lru_status binder_alloc_free_page(struct list_head *item,
}
mutex_unlock(&alloc->mutex);
if (mm_locked)
mmap_read_unlock(mm);
else
if (vma)
vma_end_read(vma);
mmput_async(mm);
binder_free_page(page_to_free);
return LRU_REMOVED_RETRY;
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);
err_mmget:
return LRU_SKIP;