linux-kernel - RE: [PATCH v3 1/5] udmabuf: cancel mmap page fault, direct map it

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <IA0PR11MB718566320E42490E32C66C0CF8822@IA0PR11MB7185.namprd11.prod.outlook.com>
Date: Sat, 17 Aug 2024 00:53:18 +0000
From: "Kasireddy, Vivek" <vivek.kasireddy@...el.com>
To: Huan Yang <link@...o.com>, Gerd Hoffmann <kraxel@...hat.com>, Sumit Semwal
	<sumit.semwal@...aro.org>, Christian König
	<christian.koenig@....com>, "dri-devel@...ts.freedesktop.org"
	<dri-devel@...ts.freedesktop.org>, "linux-media@...r.kernel.org"
	<linux-media@...r.kernel.org>, "linaro-mm-sig@...ts.linaro.org"
	<linaro-mm-sig@...ts.linaro.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>
CC: "opensource.kernel@...o.com" <opensource.kernel@...o.com>
Subject: RE: [PATCH v3 1/5] udmabuf: cancel mmap page fault, direct map it

Hi Huan,

> 
> The current udmabuf mmap uses a page fault to populate the vma.
> 
> However, the current udmabuf has already obtained and pinned the folio
> upon completion of the creation.This means that the physical memory has
> already been acquired, rather than being accessed dynamically. The
> current page fault method only saves some page table memory.
> 
> As a result, the page fault has lost its purpose as a demanding
> page. Due to the fact that page fault requires trapping into kernel mode
> and filling in when accessing the corresponding virtual address in mmap,
> when creating a large size udmabuf, this represents a considerable
> overhead.
> 
> The current patch removes the page fault method of mmap and
> instead fills pfn directly when mmap is triggered.
> 
> Signed-off-by: Huan Yang <link@...o.com>
> Suggested-by: Vivek Kasireddy <vivek.kasireddy@...el.com>
> ---
>  drivers/dma-buf/udmabuf.c | 37 +++++++++++++++----------------------
>  1 file changed, 15 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c
> index 047c3cd2ceff..d39f9e1cd532 100644
> --- a/drivers/dma-buf/udmabuf.c
> +++ b/drivers/dma-buf/udmabuf.c
> @@ -38,36 +38,29 @@ struct udmabuf_folio {
>  	struct list_head list;
>  };
> 
> -static vm_fault_t udmabuf_vm_fault(struct vm_fault *vmf)
> -{
> -	struct vm_area_struct *vma = vmf->vma;
> -	struct udmabuf *ubuf = vma->vm_private_data;
> -	pgoff_t pgoff = vmf->pgoff;
> -	unsigned long pfn;
> -
> -	if (pgoff >= ubuf->pagecount)
> -		return VM_FAULT_SIGBUS;
> -
> -	pfn = folio_pfn(ubuf->folios[pgoff]);
> -	pfn += ubuf->offsets[pgoff] >> PAGE_SHIFT;
> -
> -	return vmf_insert_pfn(vma, vmf->address, pfn);
> -}
> -
> -static const struct vm_operations_struct udmabuf_vm_ops = {
> -	.fault = udmabuf_vm_fault,
> -};
So, what I was suggesting earlier is that it would be OK to populate the whole
vma after first fault because userspace can simply call mmap() but choose not
to use the returned pointer for various reasons. This is what Qemu's virtio-gpu
module does and in this case we'd be unnecessarily populating the vma.

Therefore, my request to you is to try to benchmark your userspace to see if
there is a significant difference in performance when you populate the vma
during mmap() vs doing it after first fault (which means moving the for loop
to udmabuf_vm_fault()).

> -
>  static int mmap_udmabuf(struct dma_buf *buf, struct vm_area_struct
> *vma)
>  {
>  	struct udmabuf *ubuf = buf->priv;
> +	unsigned long addr;
> +	unsigned long end;
> +	unsigned long pgoff;
> +	int ret;
Looks like ret's type needs to be vm_fault_t.

> 
>  	if ((vma->vm_flags & (VM_SHARED | VM_MAYSHARE)) == 0)
>  		return -EINVAL;
> 
> -	vma->vm_ops = &udmabuf_vm_ops;
> -	vma->vm_private_data = ubuf;
>  	vm_flags_set(vma, VM_PFNMAP | VM_DONTEXPAND |
> VM_DONTDUMP);
> +
> +	for (pgoff = vma->vm_pgoff, end = vma->vm_end, addr = vma-
I think initializing these variables above at the declaration time looks better
than initializing them in the for loop, IMO.

Thanks,
Vivek

> >vm_start;
> +	     addr < end; pgoff++, addr += PAGE_SIZE) {
> +		unsigned long pfn = folio_pfn(ubuf->folios[pgoff]);
> +
> +		pfn += ubuf->offsets[pgoff] >> PAGE_SHIFT;
> +		ret = vmf_insert_pfn(vma, addr, pfn);
> +		if (ret & VM_FAULT_ERROR)
> +			return vm_fault_to_errno(ret, 0);
> +	}
> +
>  	return 0;
>  }
> 
> --
> 2.45.2