Re: [PATCH] drm/vgem: Added page prefaulting

[Andrzej Hajda <a.hajda@samsung.com>]

+CC: $(./script/get_maintainers.pl drivers/gpu/drm/vgem/vgem_drv.c)


On 20.08.2019 08:58, Szymon Andrzejuk wrote:
> Page fault handler inside vgem driver now preallocates pages in advance
> when fault occurs for the first time. Pages can be allocated in
> direction of increasing/decreasing addresses, depending on memory access
> profile. In case of random access no preallocation occurs.
>
> Synthetic benchmark showed over 8x bandwidth increase when copying data
> from mmapped vgem buffer with memcpy and ~160 times when accessing mapped
> buffer sequentially. Compiled with gcc 8.2.0 with -O2 flag.
> Unigine Heaven running on custom virgl vtest virtual GPU with vgem buffers
> sees ~17% FPS increase.
>
> This performance increase only occurs when accessing vgem buffer mapped
> using DRM_IOCTL_MODE_MAP_DUMB ioctl. When accessing buffer imported
> from prime fd the vgem page fault handler is not invoked. It's advised
> to use vector streaming copy instructions and avoid sequential accesses
> in this case. Streaming copy brings the performance to be on par with
> similar buffer allocated with memfd_create(2) syscall.
>
> Signed-off-by: Szymon Andrzejuk <s.andrzejuk@samsung.com>
> ---
>  drivers/gpu/drm/vgem/vgem_drv.c | 177 ++++++++++++++++++++++++++------
>  1 file changed, 143 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/gpu/drm/vgem/vgem_drv.c b/drivers/gpu/drm/vgem/vgem_drv.c
> index 11a8f99ba18c..739ba841e89c 100644
> --- a/drivers/gpu/drm/vgem/vgem_drv.c
> +++ b/drivers/gpu/drm/vgem/vgem_drv.c
> @@ -34,6 +34,7 @@
>  #include <linux/ramfs.h>
>  #include <linux/shmem_fs.h>
>  #include <linux/dma-buf.h>
> +#include <linux/pfn_t.h>
>  #include "vgem_drv.h"
>  
>  #define DRIVER_NAME	"vgem"
> @@ -50,8 +51,21 @@ static struct vgem_device {
>  static void vgem_gem_free_object(struct drm_gem_object *obj)
>  {
>  	struct drm_vgem_gem_object *vgem_obj = to_vgem_bo(obj);
> +	int i;
> +
> +	mutex_lock(&vgem_obj->pages_lock);
> +	if (vgem_obj->pages) {
> +		int num_pages = obj->size >> PAGE_SHIFT;
> +
> +		for (i = 0; i < num_pages; i++) {
> +			if (vgem_obj->pages[i])
> +				put_page(vgem_obj->pages[i]);
> +		}
> +		kvfree(vgem_obj->pages);
> +		vgem_obj->pages = NULL;
> +	}
> +	mutex_unlock(&vgem_obj->pages_lock);
>  
> -	kvfree(vgem_obj->pages);
>  	mutex_destroy(&vgem_obj->pages_lock);
>  
>  	if (obj->import_attach)
> @@ -61,6 +75,72 @@ static void vgem_gem_free_object(struct drm_gem_object *obj)
>  	kfree(vgem_obj);
>  }
>  
> +static int __vgem_alloc_page(struct page *page, struct vm_area_struct *vma,
> +							 unsigned long vaddr, int page_num)


unused page_num


> +{
> +	unsigned long pfn;
> +	int insert_ret;
> +
> +	pfn = page_to_pfn(page);
> +	insert_ret = vmf_insert_mixed(vma, vaddr, __pfn_to_pfn_t(pfn, PFN_DEV));
> +
> +	if (insert_ret & VM_FAULT_ERROR)
> +		return VM_FAULT_ERROR;


Is it OK to return mask? instead of insert_ret.


> +
> +	return 0;
> +}
> +
> +static int __vgem_read_mapping_page(struct drm_vgem_gem_object *obj,
> +									int page_num, struct page **page)
> +{
> +	int ret;
> +	struct page *mapped_page;
> +
> +	mapped_page = shmem_read_mapping_page(file_inode(obj->base.filp)->i_mapping,
> +										  page_num);
> +	if (IS_ERR(page)) {
> +		switch (PTR_ERR(page)) {
> +		case -ENOSPC:
> +		case -ENOMEM:
> +			ret = VM_FAULT_OOM;
> +			break;
> +		case -EBUSY:
> +			ret = VM_FAULT_RETRY;
> +			break;
> +		case -EFAULT:
> +		case -EINVAL:
> +			ret = VM_FAULT_SIGBUS;
> +			break;
> +		default:
> +			WARN_ON(PTR_ERR(page));
> +			ret = VM_FAULT_SIGBUS;
> +			break;
> +		}
> +
> +		return ret;
> +	}
> +
> +	*page = mapped_page;
> +	return 0;
> +}
> +
> +static int __vgem_prepare_single_page(struct drm_vgem_gem_object *obj,
> +									  struct vm_area_struct *vma,
> +									  int page_num, unsigned long vaddr)
> +{
> +	int ret;
> +
> +	ret = __vgem_read_mapping_page(obj, page_num, &obj->pages[page_num]);
> +	if (ret)
> +		return ret;
> +
> +	ret = __vgem_alloc_page(obj->pages[page_num], vma, vaddr, page_num);
> +	if (ret)
> +		return ret;


One can use return __vgem_alloc_page(...), up to you.


> +
> +	return 0;
> +}
> +
>  static vm_fault_t vgem_gem_fault(struct vm_fault *vmf)
>  {
>  	struct vm_area_struct *vma = vmf->vma;
> @@ -70,6 +150,8 @@ static vm_fault_t vgem_gem_fault(struct vm_fault *vmf)
>  	vm_fault_t ret = VM_FAULT_SIGBUS;
>  	loff_t num_pages;
>  	pgoff_t page_offset;
> +	int page_num, page_prep_ret;
> +	const int PREFAULT_PAGES = 8;
>  	page_offset = (vaddr - vma->vm_start) >> PAGE_SHIFT;
>  
>  	num_pages = DIV_ROUND_UP(obj->base.size, PAGE_SIZE);
> @@ -77,41 +159,60 @@ static vm_fault_t vgem_gem_fault(struct vm_fault *vmf)
>  	if (page_offset >= num_pages)
>  		return VM_FAULT_SIGBUS;
>  
> +	ret = VM_FAULT_NOPAGE;
> +
>  	mutex_lock(&obj->pages_lock);
> -	if (obj->pages) {
> -		get_page(obj->pages[page_offset]);
> -		vmf->page = obj->pages[page_offset];
> -		ret = 0;
> -	}
> -	mutex_unlock(&obj->pages_lock);
> -	if (ret) {
> -		struct page *page;
> -
> -		page = shmem_read_mapping_page(
> -					file_inode(obj->base.filp)->i_mapping,
> -					page_offset);
> -		if (!IS_ERR(page)) {
> -			vmf->page = page;
> -			ret = 0;
> -		} else switch (PTR_ERR(page)) {
> -			case -ENOSPC:
> -			case -ENOMEM:
> -				ret = VM_FAULT_OOM;
> -				break;
> -			case -EBUSY:
> -				ret = VM_FAULT_RETRY;
> -				break;
> -			case -EFAULT:
> -			case -EINVAL:
> -				ret = VM_FAULT_SIGBUS;
> -				break;
> -			default:
> -				WARN_ON(PTR_ERR(page));
> -				ret = VM_FAULT_SIGBUS;
> -				break;
> +
> +	if (num_pages > 1) {
> +		bool forward = true;
> +		bool random = false;
> +
> +		// Determine prefaulting direction. If adjacent pages are both
> +		// allocated/not allocated then we have random access.
> +		// Always try to prefault on first and last page.
> +		if (page_offset != 0 && page_offset != num_pages - 1) {
> +			struct page *next, *prev;
> +
> +			next = obj->pages[page_offset + 1];
> +			prev = obj->pages[page_offset - 1];
> +			if (!((uintptr_t)next ^ (uintptr_t)prev))
> +				random = true;
> +			else if (!prev)
> +				forward = false;
> +		} else {
> +			forward = (page_offset == 0);
>  		}


Quite complicated,  maybe sth like this:

    bool next, prev;

    next = obj->pages[page_offset + 1];

    prev = obj->pages[page_offset - 1];

    if (prev == next)

        random = true;


>  
> +		if (!random) {
> +			for (page_num = page_offset;
> +				forward ? page_num < page_offset + PREFAULT_PAGES && page_num < num_pages :
> +				page_offset - page_num < PREFAULT_PAGES && page_num >= 0;
> +				forward ? page_num++ : page_num--) {


Again complicated, try pre-calculate boundaries and:

start_page = ...;

end_page = ...;

step = forward ? 1 : -1;

for (page_num = start_page; page_num < end_page; page_num += step)

    ...


> +				if (!obj->pages[page_num]) {
> +					page_prep_ret = __vgem_prepare_single_page(obj, vma, page_num, vaddr);
> +					if (page_prep_ret) {
> +						ret = page_prep_ret;
> +						break;
> +					}
> +				} else {
> +					// random access, exit loop
> +					break;
> +				}
> +
> +				vaddr = vaddr + (forward ? 1 : -1) * PAGE_SIZE;


vaddr += step * PAGE_SIZE;


Regards

Andrzej


> +			}
> +		} else {
> +			page_prep_ret = __vgem_prepare_single_page(obj, vma, page_offset, vaddr);
> +			if (page_prep_ret)
> +				ret = page_prep_ret;
> +		}
> +	} else {
> +		page_prep_ret = __vgem_prepare_single_page(obj, vma, page_offset, vaddr);
> +		if (page_prep_ret)
> +			ret = page_prep_ret;
>  	}
> +
> +	mutex_unlock(&obj->pages_lock);
>  	return ret;
>  }
>  
> @@ -182,7 +283,7 @@ static struct drm_gem_object *vgem_gem_create(struct drm_device *dev,
>  					      unsigned long size)
>  {
>  	struct drm_vgem_gem_object *obj;
> -	int ret;
> +	int ret, num_pages;
>  
>  	obj = __vgem_gem_create(dev, size);
>  	if (IS_ERR(obj))
> @@ -193,6 +294,13 @@ static struct drm_gem_object *vgem_gem_create(struct drm_device *dev,
>  	if (ret)
>  		return ERR_PTR(ret);
>  
> +	mutex_lock(&obj->pages_lock);
> +
> +	num_pages = obj->base.size >> PAGE_SHIFT;
> +	obj->pages = kvcalloc(num_pages, sizeof(struct page *), GFP_KERNEL);
> +
> +	mutex_unlock(&obj->pages_lock);
> +
>  	return &obj->base;
>  }
>  
> @@ -262,7 +370,8 @@ static int vgem_mmap(struct file *filp, struct vm_area_struct *vma)
>  	/* Keep the WC mmaping set by drm_gem_mmap() but our pages
>  	 * are ordinary and not special.
>  	 */
> -	vma->vm_flags = flags | VM_DONTEXPAND | VM_DONTDUMP;
> +	vma->vm_flags = flags | VM_DONTEXPAND | VM_DONTDUMP | VM_MIXEDMAP;
> +
>  	return 0;
>  }
>  
>
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel