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

[Lorenzo Stoakes]

On Wed, Apr 29, 2026 at 11:19:57AM -0700, Dave Hansen wrote:
>
> From: Dave Hansen <dave.hansen@xxxxxxxxxxxxxxx>
>
> 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 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@xxxxxxxxxxxxxxx>

I mean this seems reasonable to me, I don't really understand why we'd fall back
to mmap... try lock :)

If semantically you're trylocking then as you say, lock_vma_under_rcu() already
does that.

Honestly I feel this could be submitted separate from the series.

I'm not a binder guy, but this looks right to me so:

Acked-by: Lorenzo Stoakes <ljs@xxxxxxxxxx> with nit below addressed.

> Cc: Suren Baghdasaryan <surenb@xxxxxxxxxx>
> Cc: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
> Cc: "Liam R. Howlett" <Liam.Howlett@xxxxxxxxxx>
> Cc: Lorenzo Stoakes <ljs@xxxxxxxxxx>
> Cc: Vlastimil Babka <vbabka@xxxxxxxxxx>
> Cc: Shakeel Butt <shakeel.butt@xxxxxxxxx>
> Cc: linux-mm@xxxxxxxxx
> ---
>
>  b/drivers/android/binder_alloc.c |   22 +++++-----------------
>  1 file changed, 5 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-04-29 11:18:50.066607065 -0700
> +++ b/drivers/android/binder_alloc.c	2026-04-29 11:18:50.069607180 -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;

Man why are we using int instead of a bool in 2026 in the first place :P

>  	size_t index;
>
>  	if (!mmget_not_zero(mm))
> @@ -1151,15 +1150,10 @@ 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 */
>  	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 (!vma)
> +		goto err_mmap_read_lock_failed;

Nit, but we probably want to rename that to err_vma_lock_failed or something!

>
>  	if (!mutex_trylock(&alloc->mutex))
>  		goto err_get_alloc_mutex_failed;
> @@ -1191,10 +1185,7 @@ enum lru_status binder_alloc_free_page(s
>  	}
>
>  	mutex_unlock(&alloc->mutex);
> -	if (mm_locked)
> -		mmap_read_unlock(mm);
> -	else
> -		vma_end_read(vma);
> +	vma_end_read(vma);
>  	mmput_async(mm);
>  	binder_free_page(page_to_free);
>
> @@ -1203,10 +1194,7 @@ 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
> -		vma_end_read(vma);
> +	vma_end_read(vma);
>  err_mmap_read_lock_failed:
>  	mmput_async(mm);
>  err_mmget:
> _