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

On Thu, Aug 06, 2026 at 01:05:45PM -0700, Suren Baghdasaryan wrote:
> From: Dave Hansen <[email protected]>
> 
> 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 <[email protected]>
> Signed-off-by: Suren Baghdasaryan <[email protected]>
> Cc: Andrew Morton <[email protected]>
> Cc: "Liam R. Howlett" <[email protected]>
> Cc: Vlastimil Babka <[email protected]>
> Cc: Shakeel Butt <[email protected]>
> Cc: [email protected]
> Cc: Greg Kroah-Hartman <[email protected]>
> Cc: Arve HjønnevÃ¥g <[email protected]>
> Cc: Todd Kjos <[email protected]>
> Cc: Christian Brauner <[email protected]>
> Cc: Carlos Llamas <[email protected]>
> Cc: Alice Ryhl <[email protected]>
> Cc: "David S. Miller" <[email protected]>
> Cc: David Ahern <[email protected]>
> Cc: [email protected]
> ---
>  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?

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


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

>  	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,18 @@ 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);

This mutex_unlock() is now dead-code.

>  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;
> -- 
> 2.55.0.654.g21b8a5bc05-goog
>