[RFC PATCH 0/1] binder: switch alloc->mutex back to spinlock

[RFC PATCH 0/1] binder: switch alloc->mutex back to spinlock

[Bo Zhang]

Hi,

This patch switches the binder_alloc mutex back to a spinlock to reduce
transaction latency.

Background:

Commit 7710e2cca32e ("binder: switch alloc->mutex to spinlock_t")
originally converted the mutex to a spinlock for performance. It was
later reverted by commit 8b52c7261e04 in preparation for commit
d1716b4b78fb ("binder: concurrent page installation"), which states:

  "zap_page_range_single() is called under the alloc->mutex to avoid
   racing with the shrinker."

Analysis:

After inspection, holding the lock across zap_vma_range() in the shrinker
path is unnecessary. The page installation side does NOT hold alloc->mutex,
so the mutex provides no mutual exclusion between install and zap. The 
actual synchronization is guaranteed at the PTE lock level:

  1. vm_insert_page() acquires the PTE lock to set the PTE entry.
  2. zap_vma_range() acquires the PTE lock to clear the PTE entry.
  3. On race conditions, the install path calls binder_page_lookup
     which uses get_user_pages_remote() to atomically pin the page
     under PTE lock, preventing use-after-free regardless of zap timing.

The alloc lock only needs to protect buffer metadata (rb-trees, pages
array, LRU list operations), all of which are non-sleeping and complete
before zap_vma_range() is called.

By moving spin_unlock() before zap_vma_range() in the shrinker path, we
can safely convert back to a spinlock.

Performance (binderThroughputTest, Qualcomm SM8850, 2 workers, 10 runs):

                    mutex               spinlock
  throughput:    27k-59k iter/s       79k-84k iter/s  (~80% improvement)
  average:       0.031-0.068ms        0.022-0.023ms   (~45% reduction)
  P99:           0.088-0.148ms        0.050-0.062ms   (~55% reduction)
  variance:      high (2x spread)     low (stable)

The spinlock eliminates priority inversion where low-priority tasks
holding the mutex sleep, blocking high-priority binder transactions.

Looking forward to feedback on this analysis.

Bo Zhang (1):
  binder: switch alloc->mutex back to spinlock

 drivers/android/binder_alloc.c | 36 +++++++++++++++++-----------------
 drivers/android/binder_alloc.h |  8 ++++----
 2 files changed, 22 insertions(+), 22 deletions(-)

-- 
2.34.1

[RFC PATCH 1/1] binder: switch alloc->mutex back to spinlock

[Bo Zhang]

The alloc->mutex was restored by commit 8b52c7261e04 ("Revert binder:
switch alloc->mutex to spinlock_t") to allow zap_vma_range() to be
called under the lock in the shrinker path.

However, holding the lock across zap_vma_range() is unnecessary because
synchronization between page installation and zap is already guaranteed
by the PTE lock. Move spin_unlock() before zap_vma_range() and convert
the mutex back to a spinlock.

This eliminates priority inversion where a low-priority task holding the
mutex sleeps, blocking high-priority binder transactions.

Performance (binderThroughputTest, SM8850, 2 workers, 10 runs):

                    mutex               spinlock
  throughput:    27k-59k iter/s       79k-84k iter/s
  average:       0.031-0.068ms        0.022-0.023ms
  P99:           0.088-0.148ms        0.050-0.062ms

Signed-off-by: Bo Zhang <zhangbo56@xiaomi.com>
---
 drivers/android/binder_alloc.c | 36 +++++++++++++++++-----------------
 drivers/android/binder_alloc.h |  8 ++++----
 2 files changed, 22 insertions(+), 22 deletions(-)

diff --git a/drivers/android/binder_alloc.c b/drivers/android/binder_alloc.c
index e4488ad86a65..9775df3616aa 100644
--- a/drivers/android/binder_alloc.c
+++ b/drivers/android/binder_alloc.c
@@ -169,7 +169,7 @@ static struct binder_buffer *binder_alloc_prepare_to_free_locked(
 struct binder_buffer *binder_alloc_prepare_to_free(struct binder_alloc *alloc,
 						   unsigned long user_ptr)
 {
-	guard(mutex)(&alloc->mutex);
+	guard(spinlock)(&alloc->lock);
 	return binder_alloc_prepare_to_free_locked(alloc, user_ptr);
 }
 
@@ -676,10 +676,10 @@ struct binder_buffer *binder_alloc_new_buf(struct binder_alloc *alloc,
 	if (!next)
 		return ERR_PTR(-ENOMEM);
 
-	mutex_lock(&alloc->mutex);
+	spin_lock(&alloc->lock);
 	buffer = binder_alloc_new_buf_locked(alloc, next, size, is_async);
 	if (IS_ERR(buffer)) {
-		mutex_unlock(&alloc->mutex);
+		spin_unlock(&alloc->lock);
 		goto out;
 	}
 
@@ -687,7 +687,7 @@ struct binder_buffer *binder_alloc_new_buf(struct binder_alloc *alloc,
 	buffer->offsets_size = offsets_size;
 	buffer->extra_buffers_size = extra_buffers_size;
 	buffer->pid = current->tgid;
-	mutex_unlock(&alloc->mutex);
+	spin_unlock(&alloc->lock);
 
 	ret = binder_install_buffer_pages(alloc, buffer, size);
 	if (ret) {
@@ -872,9 +872,9 @@ void binder_alloc_free_buf(struct binder_alloc *alloc,
 		binder_alloc_clear_buf(alloc, buffer);
 		buffer->clear_on_free = false;
 	}
-	mutex_lock(&alloc->mutex);
+	spin_lock(&alloc->lock);
 	binder_free_buf_locked(alloc, buffer);
-	mutex_unlock(&alloc->mutex);
+	spin_unlock(&alloc->lock);
 }
 EXPORT_SYMBOL_IF_KUNIT(binder_alloc_free_buf);
 
@@ -967,7 +967,7 @@ void binder_alloc_deferred_release(struct binder_alloc *alloc)
 	struct binder_buffer *buffer;
 
 	buffers = 0;
-	mutex_lock(&alloc->mutex);
+	spin_lock(&alloc->lock);
 	BUG_ON(alloc->mapped);
 
 	while ((n = rb_first(&alloc->allocated_buffers))) {
@@ -1018,7 +1018,7 @@ void binder_alloc_deferred_release(struct binder_alloc *alloc)
 			page_count++;
 		}
 	}
-	mutex_unlock(&alloc->mutex);
+	spin_unlock(&alloc->lock);
 	kvfree(alloc->pages);
 	if (alloc->mm)
 		mmdrop(alloc->mm);
@@ -1043,7 +1043,7 @@ void binder_alloc_print_allocated(struct seq_file *m,
 	struct binder_buffer *buffer;
 	struct rb_node *n;
 
-	guard(mutex)(&alloc->mutex);
+	guard(spinlock)(&alloc->lock);
 	for (n = rb_first(&alloc->allocated_buffers); n; n = rb_next(n)) {
 		buffer = rb_entry(n, struct binder_buffer, rb_node);
 		seq_printf(m, "  buffer %d: %lx size %zd:%zd:%zd %s\n",
@@ -1069,7 +1069,7 @@ void binder_alloc_print_pages(struct seq_file *m,
 	int lru = 0;
 	int free = 0;
 
-	mutex_lock(&alloc->mutex);
+	spin_lock(&alloc->lock);
 	/*
 	 * Make sure the binder_alloc is fully initialized, otherwise we might
 	 * read inconsistent state.
@@ -1085,7 +1085,7 @@ void binder_alloc_print_pages(struct seq_file *m,
 				lru++;
 		}
 	}
-	mutex_unlock(&alloc->mutex);
+	spin_unlock(&alloc->lock);
 	seq_printf(m, "  pages: %d:%d:%d\n", active, lru, free);
 	seq_printf(m, "  pages high watermark: %zu\n", alloc->pages_high);
 }
@@ -1101,7 +1101,7 @@ int binder_alloc_get_allocated_count(struct binder_alloc *alloc)
 	struct rb_node *n;
 	int count = 0;
 
-	guard(mutex)(&alloc->mutex);
+	guard(spinlock)(&alloc->lock);
 	for (n = rb_first(&alloc->allocated_buffers); n != NULL; n = rb_next(n))
 		count++;
 	return count;
@@ -1161,8 +1161,8 @@ enum lru_status binder_alloc_free_page(struct list_head *item,
 		vma = vma_lookup(mm, page_addr);
 	}
 
-	if (!mutex_trylock(&alloc->mutex))
-		goto err_get_alloc_mutex_failed;
+	if (!spin_trylock(&alloc->lock))
+		goto err_get_alloc_lock_failed;
 
 	/*
 	 * Since a binder_alloc can only be mapped once, we ensure
@@ -1180,6 +1180,7 @@ enum lru_status binder_alloc_free_page(struct list_head *item,
 	trace_binder_unmap_kernel_end(alloc, index);
 
 	list_lru_isolate(lru, item);
+	spin_unlock(&alloc->lock);
 	spin_unlock(&lru->lock);
 
 	if (vma) {
@@ -1190,7 +1191,6 @@ enum lru_status binder_alloc_free_page(struct list_head *item,
 		trace_binder_unmap_user_end(alloc, index);
 	}
 
-	mutex_unlock(&alloc->mutex);
 	if (mm_locked)
 		mmap_read_unlock(mm);
 	else
@@ -1201,8 +1201,8 @@ enum lru_status binder_alloc_free_page(struct list_head *item,
 	return LRU_REMOVED_RETRY;
 
 err_invalid_vma:
-	mutex_unlock(&alloc->mutex);
-err_get_alloc_mutex_failed:
+	spin_unlock(&alloc->lock);
+err_get_alloc_lock_failed:
 	if (mm_locked)
 		mmap_read_unlock(mm);
 	else
@@ -1235,7 +1235,7 @@ VISIBLE_IF_KUNIT void __binder_alloc_init(struct binder_alloc *alloc,
 	alloc->pid = current->tgid;
 	alloc->mm = current->mm;
 	mmgrab(alloc->mm);
-	mutex_init(&alloc->mutex);
+	spin_lock_init(&alloc->lock);
 	INIT_LIST_HEAD(&alloc->buffers);
 	alloc->freelist = freelist;
 }
diff --git a/drivers/android/binder_alloc.h b/drivers/android/binder_alloc.h
index d6f1f6f2d00e..bea5a77bb6da 100644
--- a/drivers/android/binder_alloc.h
+++ b/drivers/android/binder_alloc.h
@@ -9,7 +9,7 @@
 #include <linux/rbtree.h>
 #include <linux/list.h>
 #include <linux/mm.h>
-#include <linux/rtmutex.h>
+#include <linux/spinlock.h>
 #include <linux/vmalloc.h>
 #include <linux/slab.h>
 #include <linux/list_lru.h>
@@ -80,7 +80,7 @@ static inline struct list_head *page_to_lru(struct page *p)
 
 /**
  * struct binder_alloc - per-binder proc state for binder allocator
- * @mutex:              protects binder_alloc fields
+ * @lock:               protects binder_alloc fields
  * @mm:                 copy of task->mm (invariant after open)
  * @vm_start:           base of per-proc address space mapped via mmap
  * @buffers:            list of all buffers for this proc
@@ -105,7 +105,7 @@ static inline struct list_head *page_to_lru(struct page *p)
  * struct binder_buffer objects used to track the user buffers
  */
 struct binder_alloc {
-	struct mutex mutex;
+	spinlock_t lock;
 	struct mm_struct *mm;
 	unsigned long vm_start;
 	struct list_head buffers;
@@ -156,7 +156,7 @@ void binder_alloc_print_pages(struct seq_file *m,
 static inline size_t
 binder_alloc_get_free_async_space(struct binder_alloc *alloc)
 {
-	guard(mutex)(&alloc->mutex);
+	guard(spinlock)(&alloc->lock);
 	return alloc->free_async_space;
 }
 
-- 
2.34.1

Re: [RFC PATCH 0/1] binder: switch alloc->mutex back to spinlock

[Alice Ryhl]

On Wed, Aug 05, 2026 at 11:27:51PM +0800, Bo Zhang wrote:
> Hi,
> 
> This patch switches the binder_alloc mutex back to a spinlock to reduce
> transaction latency.
> 
> Background:
> 
> Commit 7710e2cca32e ("binder: switch alloc->mutex to spinlock_t")
> originally converted the mutex to a spinlock for performance. It was
> later reverted by commit 8b52c7261e04 in preparation for commit
> d1716b4b78fb ("binder: concurrent page installation"), which states:
> 
>   "zap_page_range_single() is called under the alloc->mutex to avoid
>    racing with the shrinker."
> 
> Analysis:
> 
> After inspection, holding the lock across zap_vma_range() in the shrinker
> path is unnecessary. The page installation side does NOT hold alloc->mutex,
> so the mutex provides no mutual exclusion between install and zap. The 
> actual synchronization is guaranteed at the PTE lock level:
> 
>   1. vm_insert_page() acquires the PTE lock to set the PTE entry.
>   2. zap_vma_range() acquires the PTE lock to clear the PTE entry.
>   3. On race conditions, the install path calls binder_page_lookup
>      which uses get_user_pages_remote() to atomically pin the page
>      under PTE lock, preventing use-after-free regardless of zap timing.
> 
> The alloc lock only needs to protect buffer metadata (rb-trees, pages
> array, LRU list operations), all of which are non-sleeping and complete
> before zap_vma_range() is called.
> 
> By moving spin_unlock() before zap_vma_range() in the shrinker path, we
> can safely convert back to a spinlock.
> 
> Performance (binderThroughputTest, Qualcomm SM8850, 2 workers, 10 runs):
> 
>                     mutex               spinlock
>   throughput:    27k-59k iter/s       79k-84k iter/s  (~80% improvement)
>   average:       0.031-0.068ms        0.022-0.023ms   (~45% reduction)
>   P99:           0.088-0.148ms        0.050-0.062ms   (~55% reduction)
>   variance:      high (2x spread)     low (stable)
> 
> The spinlock eliminates priority inversion where low-priority tasks
> holding the mutex sleep, blocking high-priority binder transactions.
> 
> Looking forward to feedback on this analysis.

This will not work due to the following race condition between T1 and T2:

T1 binder_alloc_free_page() removes the page from alloc->pages
T2 binder_alloc_new_buf_locked() runs
T2 binder_install_single_page() gets -EBUSY
T2 binder_page_lookup() looks up the page in the vma
T1 binder_alloc_free_page() calls zap_vma_range()

In this scenario, T2 looks up a page that it expects to be "pinned" and
not removable by the shrinker, but the shrinker zaps it anyway.

Today this cannot happen because binder_alloc_new_buf_locked() runs
under the mutex, which ensures that zap_vma_range() finishes removing
the page before binder_install_single_page() runs.

Alice

Re: [RFC PATCH 0/1] binder: switch alloc->mutex back to spinlock

[Bo Zhang]

On Thu, Aug 06, 2026 at 09:01:19AM +0000, Alice Ryhl wrote:
> This will not work due to the following race condition between T1 and T2:
> 
> T1 binder_alloc_free_page() removes the page from alloc->pages
> T2 binder_alloc_new_buf_locked() runs
> T2 binder_install_single_page() gets -EBUSY
> T2 binder_page_lookup() looks up the page in the vma
> T1 binder_alloc_free_page() calls zap_vma_range()
> 
> In this scenario, T2 looks up a page that it expects to be "pinned" and
> not removable by the shrinker, but the shrinker zaps it anyway.
> 
> Today this cannot happen because binder_alloc_new_buf_locked() runs
> under the mutex, which ensures that zap_vma_range() finishes removing
> the page before binder_install_single_page() runs.

Hi Alice,

Thanks for pointing out this race. You're right that simply moving
spin_unlock() before zap_vma_range() opens a window where the install
side could GUP the old page that is about to be freed.

However, I think it could be resolved without keeping the lock across
zap_vma_range(). The solution might be:

The install side only calls vm_insert_page() when pages[index] == NULL.
And the shrinker sets pages[index] = NULL before zapping. So if
vm_insert_page() returns -EBUSY, we can check pages[index] to determine
which scenario we're in:

  1. pages[index] != NULL: Another installer won the race. It's safe to
     GUP the page, which is the normal concurrent install case.

  2. pages[index] == NULL: The shrinker cleared it but hasn't zapped
     the PTE yet. We must NOT GUP the old page. Return -EAGAIN and
     let the caller retry after the shrinker finishes.

Case 2 is safe because: the install side only attempts installation
when pages[index] == NULL, so if EBUSY occurs with pages[index] == NULL,
it must be a PTE left over from the page that the shrinker is currently
reclaiming. After the shrinker zaps and frees, the retry will find
pages[index] still NULL, call vm_insert_page() again, and succeed since
the PTE is now clear.

Case 1 cannot race with the shrinker because: if pages[index] != NULL
at the time of the EBUSY check, the page was just installed by another
thread and is not on the LRU freelist, so the shrinker won't touch it.

The change would look like:

  case -EBUSY:
      binder_free_page(page);
      if (!binder_get_installed_page(alloc, index)) {
          /* shrinker is reclaiming, retry */
          ret = -EAGAIN;
          break;
      }
      /* normal concurrent install, look up the winner's page */
      page = binder_page_lookup(alloc, addr);
      ...

And the caller retries on -EAGAIN with a cond_resched().

This also should not introduce any performance regression since the
-EAGAIN retry only triggers in the rare case where EBUSY coincides with
an active shrinker reclaim on the same page index, a window that lasts
only for the duration of a single zap_vma_range() call.

Does this approach look reasonable to you?

Bo