Use per-vma locking in the shrinker's callback when reclaiming pages,
similar to the page installation logic. This minimizes contention with
unrelated vmas improving performance. The mmap_sem is still acquired if
the per-vma lock cannot be obtained.
Cc: Suren Baghdasaryan <surenb@google.com>
Suggested-by: Liam R. Howlett <Liam.Howlett@oracle.com>
Reviewed-by: Suren Baghdasaryan <surenb@google.com>
Signed-off-by: Carlos Llamas <cmllamas@google.com>
---
drivers/android/binder_alloc.c | 29 ++++++++++++++++++++++-------
1 file changed, 22 insertions(+), 7 deletions(-)
diff --git a/drivers/android/binder_alloc.c b/drivers/android/binder_alloc.c
index 339db88c1522..8c10c1a6f459 100644
--- a/drivers/android/binder_alloc.c
+++ b/drivers/android/binder_alloc.c
@@ -1128,19 +1128,28 @@ enum lru_status binder_alloc_free_page(struct list_head *item,
struct mm_struct *mm = alloc->mm;
struct vm_area_struct *vma;
unsigned long page_addr;
+ int mm_locked = 0;
size_t index;
if (!mmget_not_zero(mm))
goto err_mmget;
- if (!mmap_read_trylock(mm))
- goto err_mmap_read_lock_failed;
- if (!mutex_trylock(&alloc->mutex))
- goto err_get_alloc_mutex_failed;
index = page->index;
page_addr = alloc->vm_start + index * PAGE_SIZE;
- vma = vma_lookup(mm, page_addr);
+ /* attempt per-vma lock first */
+ 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;
+
/* ensure the vma corresponds to the binder mapping */
if (vma && !binder_alloc_is_mapped(alloc))
goto err_invalid_vma;
@@ -1163,7 +1172,10 @@ enum lru_status binder_alloc_free_page(struct list_head *item,
}
mutex_unlock(&alloc->mutex);
- mmap_read_unlock(mm);
+ if (mm_locked)
+ mmap_read_unlock(mm);
+ else
+ vma_end_read(vma);
mmput_async(mm);
__free_page(page);
@@ -1172,7 +1184,10 @@ enum lru_status binder_alloc_free_page(struct list_head *item,
err_invalid_vma:
mutex_unlock(&alloc->mutex);
err_get_alloc_mutex_failed:
- mmap_read_unlock(mm);
+ if (mm_locked)
+ mmap_read_unlock(mm);
+ else
+ vma_end_read(vma);
err_mmap_read_lock_failed:
mmput_async(mm);
err_mmget:
--
2.47.0.338.g60cca15819-googOn Tue, Nov 26, 2024 at 10:40 AM Carlos Llamas <cmllamas@google.com> wrote:
>
> Use per-vma locking in the shrinker's callback when reclaiming pages,
> similar to the page installation logic. This minimizes contention with
> unrelated vmas improving performance. The mmap_sem is still acquired if
> the per-vma lock cannot be obtained.
>
> Cc: Suren Baghdasaryan <surenb@google.com>
> Suggested-by: Liam R. Howlett <Liam.Howlett@oracle.com>
> Reviewed-by: Suren Baghdasaryan <surenb@google.com>
> Signed-off-by: Carlos Llamas <cmllamas@google.com>
> ---
> drivers/android/binder_alloc.c | 29 ++++++++++++++++++++++-------
> 1 file changed, 22 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/android/binder_alloc.c b/drivers/android/binder_alloc.c
> index 339db88c1522..8c10c1a6f459 100644
> --- a/drivers/android/binder_alloc.c
> +++ b/drivers/android/binder_alloc.c
> @@ -1128,19 +1128,28 @@ enum lru_status binder_alloc_free_page(struct list_head *item,
> struct mm_struct *mm = alloc->mm;
> struct vm_area_struct *vma;
> unsigned long page_addr;
> + int mm_locked = 0;
> size_t index;
>
> if (!mmget_not_zero(mm))
> goto err_mmget;
> - if (!mmap_read_trylock(mm))
> - goto err_mmap_read_lock_failed;
> - if (!mutex_trylock(&alloc->mutex))
> - goto err_get_alloc_mutex_failed;
>
> index = page->index;
> page_addr = alloc->vm_start + index * PAGE_SIZE;
>
> - vma = vma_lookup(mm, page_addr);
> + /* attempt per-vma lock first */
> + 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;
> +
> /* ensure the vma corresponds to the binder mapping */
You did add a clarifying comment I asked for in
https://lore.kernel.org/all/CAJuCfpESdY4L_sSwiCYVCX+5y1WOuAjLNPw35pEGzTSyoHFYPA@mail.gmail.com/
> if (vma && !binder_alloc_is_mapped(alloc))
> goto err_invalid_vma;
> @@ -1163,7 +1172,10 @@ enum lru_status binder_alloc_free_page(struct list_head *item,
> }
>
> mutex_unlock(&alloc->mutex);
> - mmap_read_unlock(mm);
> + if (mm_locked)
> + mmap_read_unlock(mm);
> + else
> + vma_end_read(vma);
> mmput_async(mm);
> __free_page(page);
>
> @@ -1172,7 +1184,10 @@ enum lru_status binder_alloc_free_page(struct list_head *item,
> err_invalid_vma:
> mutex_unlock(&alloc->mutex);
> err_get_alloc_mutex_failed:
> - mmap_read_unlock(mm);
> + if (mm_locked)
> + mmap_read_unlock(mm);
> + else
> + vma_end_read(vma);
> err_mmap_read_lock_failed:
> mmput_async(mm);
> err_mmget:
> --
> 2.47.0.338.g60cca15819-goog
>
On Tue, Nov 26, 2024 at 10:45 AM Suren Baghdasaryan <surenb@google.com> wrote:
>
> On Tue, Nov 26, 2024 at 10:40 AM Carlos Llamas <cmllamas@google.com> wrote:
> >
> > Use per-vma locking in the shrinker's callback when reclaiming pages,
> > similar to the page installation logic. This minimizes contention with
> > unrelated vmas improving performance. The mmap_sem is still acquired if
> > the per-vma lock cannot be obtained.
> >
> > Cc: Suren Baghdasaryan <surenb@google.com>
> > Suggested-by: Liam R. Howlett <Liam.Howlett@oracle.com>
> > Reviewed-by: Suren Baghdasaryan <surenb@google.com>
> > Signed-off-by: Carlos Llamas <cmllamas@google.com>
> > ---
> > drivers/android/binder_alloc.c | 29 ++++++++++++++++++++++-------
> > 1 file changed, 22 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/android/binder_alloc.c b/drivers/android/binder_alloc.c
> > index 339db88c1522..8c10c1a6f459 100644
> > --- a/drivers/android/binder_alloc.c
> > +++ b/drivers/android/binder_alloc.c
> > @@ -1128,19 +1128,28 @@ enum lru_status binder_alloc_free_page(struct list_head *item,
> > struct mm_struct *mm = alloc->mm;
> > struct vm_area_struct *vma;
> > unsigned long page_addr;
> > + int mm_locked = 0;
> > size_t index;
> >
> > if (!mmget_not_zero(mm))
> > goto err_mmget;
> > - if (!mmap_read_trylock(mm))
> > - goto err_mmap_read_lock_failed;
> > - if (!mutex_trylock(&alloc->mutex))
> > - goto err_get_alloc_mutex_failed;
> >
> > index = page->index;
> > page_addr = alloc->vm_start + index * PAGE_SIZE;
> >
> > - vma = vma_lookup(mm, page_addr);
> > + /* attempt per-vma lock first */
> > + 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;
> > +
> > /* ensure the vma corresponds to the binder mapping */
>
> You did add a clarifying comment I asked for in
> https://lore.kernel.org/all/CAJuCfpESdY4L_sSwiCYVCX+5y1WOuAjLNPw35pEGzTSyoHFYPA@mail.gmail.com/
s/did/did not
>
> > if (vma && !binder_alloc_is_mapped(alloc))
> > goto err_invalid_vma;
> > @@ -1163,7 +1172,10 @@ enum lru_status binder_alloc_free_page(struct list_head *item,
> > }
> >
> > mutex_unlock(&alloc->mutex);
> > - mmap_read_unlock(mm);
> > + if (mm_locked)
> > + mmap_read_unlock(mm);
> > + else
> > + vma_end_read(vma);
> > mmput_async(mm);
> > __free_page(page);
> >
> > @@ -1172,7 +1184,10 @@ enum lru_status binder_alloc_free_page(struct list_head *item,
> > err_invalid_vma:
> > mutex_unlock(&alloc->mutex);
> > err_get_alloc_mutex_failed:
> > - mmap_read_unlock(mm);
> > + if (mm_locked)
> > + mmap_read_unlock(mm);
> > + else
> > + vma_end_read(vma);
> > err_mmap_read_lock_failed:
> > mmput_async(mm);
> > err_mmget:
> > --
> > 2.47.0.338.g60cca15819-goog
> >
On Tue, Nov 26, 2024 at 10:46:03AM -0800, Suren Baghdasaryan wrote:
> On Tue, Nov 26, 2024 at 10:45 AM Suren Baghdasaryan <surenb@google.com> wrote:
> >
> > You did add a clarifying comment I asked for in
> > https://lore.kernel.org/all/CAJuCfpESdY4L_sSwiCYVCX+5y1WOuAjLNPw35pEGzTSyoHFYPA@mail.gmail.com/
>
> s/did/did not
Oh, I added the comment to patch 5/9 since it fits better there (sorry
that I forgot to mention this). Now the kerneldoc section reads:
+ * @mapped: whether the vm area is mapped, each binder instance is
+ * allowed a single mapping throughout its lifetime
... and the vma check now has the following comment:
+ /* ensure the vma corresponds to the binder mapping */
+ if (vma && !binder_alloc_is_mapped(alloc))
goto err_invalid_vma;
This was the feedback right?
On Tue, Nov 26, 2024 at 11:11 AM Carlos Llamas <cmllamas@google.com> wrote: > > On Tue, Nov 26, 2024 at 10:46:03AM -0800, Suren Baghdasaryan wrote: > > On Tue, Nov 26, 2024 at 10:45 AM Suren Baghdasaryan <surenb@google.com> wrote: > > > > > > You did add a clarifying comment I asked for in > > > https://lore.kernel.org/all/CAJuCfpESdY4L_sSwiCYVCX+5y1WOuAjLNPw35pEGzTSyoHFYPA@mail.gmail.com/ > > > > s/did/did not > > Oh, I added the comment to patch 5/9 since it fits better there (sorry > that I forgot to mention this). Now the kerneldoc section reads: > > + * @mapped: whether the vm area is mapped, each binder instance is > + * allowed a single mapping throughout its lifetime > > ... and the vma check now has the following comment: > > + /* ensure the vma corresponds to the binder mapping */ I think the above comment does not explain the race we are trying to avoid here. Something like this perhaps: /* * binder does not allow mapping of the same buffer more than once, therefore * alloc->vm_start could not have changed since the buffer can't be remapped. * Checking binder_alloc_is_mapped() ensures that the vma is mapped and still * covers the same area. */ > + if (vma && !binder_alloc_is_mapped(alloc)) > goto err_invalid_vma; > > This was the feedback right?
On Tue, Nov 26, 2024 at 12:05:58PM -0800, Suren Baghdasaryan wrote: > On Tue, Nov 26, 2024 at 11:11 AM Carlos Llamas <cmllamas@google.com> wrote: > > > > On Tue, Nov 26, 2024 at 10:46:03AM -0800, Suren Baghdasaryan wrote: > > > On Tue, Nov 26, 2024 at 10:45 AM Suren Baghdasaryan <surenb@google.com> wrote: > > > > > > > > You did add a clarifying comment I asked for in > > > > https://lore.kernel.org/all/CAJuCfpESdY4L_sSwiCYVCX+5y1WOuAjLNPw35pEGzTSyoHFYPA@mail.gmail.com/ > > > > > > s/did/did not > > > > Oh, I added the comment to patch 5/9 since it fits better there (sorry > > that I forgot to mention this). Now the kerneldoc section reads: > > > > + * @mapped: whether the vm area is mapped, each binder instance is > > + * allowed a single mapping throughout its lifetime > > > > ... and the vma check now has the following comment: > > > > + /* ensure the vma corresponds to the binder mapping */ > > I think the above comment does not explain the race we are trying to avoid here. > Something like this perhaps: > /* > * binder does not allow mapping of the same buffer more than once, therefore > * alloc->vm_start could not have changed since the buffer can't be remapped. > * Checking binder_alloc_is_mapped() ensures that the vma is mapped and still > * covers the same area. > */ Right, that is the message I tried to convey: (1) Each binder instance is allowed a single mapping throughout its lifetime (no re-mapping). (2) alloc->mapped gets cleared when this mapping is removed e.g. during vm_ops->close(). Putting 1 and 2 together... whenever binder looks up a vma it also checks alloc->mapped to verify its mapping is still opened and avoid poking into some other unrelated vma. I tried writing a concise explanation but I guess it was not enough. Note this unusual behavior is nothing new in binder and predates this patchset, but I agree it needs to be documented somewhere. I'll send out a new version attempting to document this better. It's a little tricky though, since the same vma validation pattern is in multiple places and obviously I don't want to duplicate the paragraph everywhere. Maybe I can inline a "binder_vma_check()" and put the explanation there. I'll think of something. Cheers, Carlos Llamas
© 2016 - 2026 Red Hat, Inc.