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

Thank you for the review Andrzej. I'll update the patch to v2 shortly, it should cover all your comments.

FYI, I'll be on holiday until September 16 so I might not be able to respond in the following days.

Regards, Szymon

On 03.09.2019 15:19, Andrzej Hajda wrote:

+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