Re: [PATCH v3 2/8] binder: concurrent page installation

[David Hildenbrand <david@redhat.com>]

On 08.11.24 20:10, Carlos Llamas wrote:
> Allow multiple callers to install pages simultaneously by downgrading
> the mmap_sem to non-exclusive mode. Races to the same PTE are handled
> using get_user_pages_remote() to retrieve the already installed page.
> This method significantly reduces contention in the mmap semaphore.
> 
> To ensure safety, vma_lookup() is used (instead of alloc->vma) to avoid
> operating on an isolated VMA. In addition, zap_page_range_single() is
> called under the alloc->mutex to avoid racing with the shrinker.
> 
> Many thanks to Barry Song who posted a similar approach [1].
> 
> Link: https://lore.kernel.org/all/20240902225009.34576-1-21cnbao@gmail.com/ [1]
> Cc: David Hildenbrand <david@redhat.com>
> Cc: Barry Song <v-songbaohua@oppo.com>
> Cc: Suren Baghdasaryan <surenb@google.com>
> Cc: 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 | 64 +++++++++++++++++++++-------------
>   1 file changed, 40 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/android/binder_alloc.c b/drivers/android/binder_alloc.c
> index 7241bf4a3ff2..2ab520c285b3 100644
> --- a/drivers/android/binder_alloc.c
> +++ b/drivers/android/binder_alloc.c
> @@ -221,26 +221,14 @@ static int binder_install_single_page(struct binder_alloc *alloc,
>   				      struct binder_lru_page *lru_page,
>   				      unsigned long addr)
>   {
> +	struct vm_area_struct *vma;
>   	struct page *page;
> -	int ret = 0;
> +	long npages;
> +	int ret;
>   
>   	if (!mmget_not_zero(alloc->mm))
>   		return -ESRCH;
>   
> -	/*
> -	 * Protected with mmap_sem in write mode as multiple tasks
> -	 * might race to install the same page.
> -	 */
> -	mmap_write_lock(alloc->mm);
> -	if (binder_get_installed_page(lru_page))
> -		goto out;
> -
> -	if (!alloc->vma) {
> -		pr_err("%d: %s failed, no vma\n", alloc->pid, __func__);
> -		ret = -ESRCH;
> -		goto out;
> -	}
> -
>   	page = alloc_page(GFP_KERNEL | __GFP_HIGHMEM | __GFP_ZERO);
>   	if (!page) {
>   		pr_err("%d: failed to allocate page\n", alloc->pid);
> @@ -248,19 +236,47 @@ static int binder_install_single_page(struct binder_alloc *alloc,
>   		goto out;
>   	}
>   
> -	ret = vm_insert_page(alloc->vma, addr, page);
> -	if (ret) {
> +	mmap_read_lock(alloc->mm);
> +	vma = vma_lookup(alloc->mm, addr);
> +	if (!vma || vma != alloc->vma) {
> +		__free_page(page);
> +		pr_err("%d: %s failed, no vma\n", alloc->pid, __func__);
> +		ret = -ESRCH;
> +		goto unlock;
> +	}
> +
> +	ret = vm_insert_page(vma, addr, page);
> +	switch (ret) {
> +	case -EBUSY:
> +		/*
> +		 * EBUSY is ok. Someone installed the pte first but the
> +		 * lru_page->page_ptr has not been updated yet. Discard
> +		 * our page and look up the one already installed.
> +		 */
> +		ret = 0;
> +		__free_page(page);
> +		npages = get_user_pages_remote(alloc->mm, addr, 1, 0, &page, NULL);

This will trigger a page fault if we don't find what we expect (are 
races with e.g., MADV_DONTNEED possible?), is that really desired or not 
a problem?

-- 
Cheers,

David / dhildenb