Re: [PATCH v2 8/8] binder: use per-vma lock in page installation

From: Suren Baghdasaryan
Date: Thu Nov 07 2024 - 11:23:08 EST


On Wed, Nov 6, 2024 at 8:03 PM Carlos Llamas <cmllamas@xxxxxxxxxx> wrote:
>
> Use per-vma locking for concurrent page installations, this minimizes
> contention with unrelated vmas improving performance. The mmap_lock is
> still acquired when needed though, e.g. before get_user_pages_remote().
>
> Many thanks to Barry Song who posted a similar approach [1].
>
> Link: https://lore.kernel.org/all/20240902225009.34576-1-21cnbao@xxxxxxxxx/ [1]
> Cc: Nhat Pham <nphamcs@xxxxxxxxx>
> Cc: Johannes Weiner <hannes@xxxxxxxxxxx>
> Cc: Barry Song <v-songbaohua@xxxxxxxx>
> Cc: Suren Baghdasaryan <surenb@xxxxxxxxxx>
> Cc: Hillf Danton <hdanton@xxxxxxxx>
> Cc: Lorenzo Stoakes <lorenzo.stoakes@xxxxxxxxxx>
> Signed-off-by: Carlos Llamas <cmllamas@xxxxxxxxxx>
> ---
> drivers/android/binder_alloc.c | 85 +++++++++++++++++++++++-----------
> 1 file changed, 57 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/android/binder_alloc.c b/drivers/android/binder_alloc.c
> index 814435a2601a..debfa541e01b 100644
> --- a/drivers/android/binder_alloc.c
> +++ b/drivers/android/binder_alloc.c
> @@ -233,6 +233,56 @@ static inline bool binder_alloc_is_mapped(struct binder_alloc *alloc)
> return smp_load_acquire(&alloc->mapped);
> }
>
> +static struct page *binder_page_lookup(struct mm_struct *mm,

Maybe pass "struct binder_alloc" in both binder_page_lookup() and
binder_page_insert()?
I like how previous code stabilized mm with mmget_not_zero() once vs
now binder_page_lookup() and binder_page_insert() have to mmget/mmput
individually. Not a big deal but looked cleaner.

> + unsigned long addr)
> +{
> + struct page *page;
> + long ret;
> +
> + if (!mmget_not_zero(mm))
> + return NULL;
> +
> + mmap_read_lock(mm);
> + ret = get_user_pages_remote(mm, addr, 1, 0, &page, NULL);
> + mmap_read_unlock(mm);
> + mmput_async(mm);
> +
> + return ret > 0 ? page : NULL;
> +}
> +
> +static int binder_page_insert(struct binder_alloc *alloc,
> + unsigned long addr,
> + struct page *page)
> +{
> + struct mm_struct *mm = alloc->mm;
> + struct vm_area_struct *vma;
> + int ret = -ESRCH;
> +
> + if (!mmget_not_zero(mm))
> + return -ESRCH;
> +
> + /* attempt per-vma lock first */
> + vma = lock_vma_under_rcu(mm, addr);
> + if (!vma)
> + goto lock_mmap;
> +
> + if (binder_alloc_is_mapped(alloc))

I don't think you need this check here. lock_vma_under_rcu() ensures
that the VMA was not detached from the tree after locking the VMA, so
if you got a VMA it's in the tree and it can't be removed (because
it's locked). remove_vma()->vma_close()->vma->vm_ops->close() is
called after VMA gets detached from the tree and that won't happen
while VMA is locked. So, if lock_vma_under_rcu() returns a VMA,
binder_alloc_is_mapped() has to always return true. A WARN_ON() check
here to ensure that might be a better option.

> + ret = vm_insert_page(vma, addr, page);
> + vma_end_read(vma);
> + goto done;

I think the code would be more readable without these jumps:

vma = lock_vma_under_rcu(mm, addr);
if (vma) {
if (!WARN_ON(!binder_alloc_is_mapped(alloc)))
ret = vm_insert_page(vma, addr, page);
vma_end_read(vma);
} else {
/* fall back to mmap_lock */
mmap_read_lock(mm);
vma = vma_lookup(mm, addr);
if (vma && binder_alloc_is_mapped(alloc))
ret = vm_insert_page(vma, addr, page);
mmap_read_unlock(mm);
}
mmput_async(mm);
return ret;


> +
> +lock_mmap:
> + /* fall back to mmap_lock */
> + mmap_read_lock(mm);
> + vma = vma_lookup(mm, addr);
> + if (vma && binder_alloc_is_mapped(alloc))
> + ret = vm_insert_page(vma, addr, page);
> + mmap_read_unlock(mm);
> +done:
> + mmput_async(mm);
> + return ret;
> +}
> +
> static struct page *binder_page_alloc(struct binder_alloc *alloc,
> unsigned long index,
> unsigned long addr)
> @@ -254,31 +304,14 @@ static int binder_install_single_page(struct binder_alloc *alloc,
> unsigned long index,
> unsigned long addr)
> {
> - struct vm_area_struct *vma;
> struct page *page;
> - long npages;
> int ret;
>
> - if (!mmget_not_zero(alloc->mm))
> - return -ESRCH;
> -
> page = binder_page_alloc(alloc, index, addr);
> - if (!page) {
> - ret = -ENOMEM;
> - goto out;
> - }
> -
> - mmap_read_lock(alloc->mm);
> - vma = vma_lookup(alloc->mm, addr);
> - if (!vma || !binder_alloc_is_mapped(alloc)) {
> - mmap_read_unlock(alloc->mm);
> - __free_page(page);
> - pr_err("%d: %s failed, no vma\n", alloc->pid, __func__);
> - ret = -ESRCH;
> - goto out;
> - }
> + if (!page)
> + return -ENOMEM;
>
> - ret = vm_insert_page(vma, addr, page);
> + ret = binder_page_insert(alloc, addr, page);
> switch (ret) {
> case -EBUSY:
> /*
> @@ -288,12 +321,11 @@ static int binder_install_single_page(struct binder_alloc *alloc,
> */
> ret = 0;
> __free_page(page);
> - npages = get_user_pages_remote(alloc->mm, addr, 1, 0, &page, NULL);
> - if (npages <= 0) {
> + page = binder_page_lookup(alloc->mm, addr);
> + if (!page) {
> pr_err("%d: failed to find page at offset %lx\n",
> alloc->pid, addr - alloc->vm_start);
> - ret = -ESRCH;
> - break;
> + return -ESRCH;
> }
> fallthrough;
> case 0:
> @@ -304,12 +336,9 @@ static int binder_install_single_page(struct binder_alloc *alloc,
> __free_page(page);
> pr_err("%d: %s failed to insert page at offset %lx with %d\n",
> alloc->pid, __func__, addr - alloc->vm_start, ret);
> - ret = -ENOMEM;
> break;
> }
> - mmap_read_unlock(alloc->mm);
> -out:
> - mmput_async(alloc->mm);
> +
> return ret;
> }
>
> --
> 2.47.0.199.ga7371fff76-goog
>