[libdrm] intel: Leak the userptr test bo

Submitted by Tvrtko Ursulin on April 17, 2015, 10:57 a.m.

Details

Message ID 1429268248-13186-1-git-send-email-tvrtko.ursulin@linux.intel.com
State New
Headers show

Not browsing as part of any series.

Commit Message

Tvrtko Ursulin April 17, 2015, 10:57 a.m.
From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

In order to use userptr, the kernel tracks the owner's mm with a
mmu_notifier. Setting that is very expensive - it involves taking all
mm_locks and a stop_machine(). This tracking lives only for as long as
the client is using userptr objects - so if the client allocates then
frees a userptr in a loop, we will be executing that heavyweight setup
everytime. To ammoritize this cost, just leak the test bo and the single
backing page we use for detecting userptr.

v2: Free the object and memory when bufmgr is destroyed.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
 intel/intel_bufmgr_gem.c | 39 ++++++++++++++++++++++++++++-----------
 1 file changed, 28 insertions(+), 11 deletions(-)

Patch hide | download patch | download mbox

diff --git a/intel/intel_bufmgr_gem.c b/intel/intel_bufmgr_gem.c
index f5d6130..969ade5 100644
--- a/intel/intel_bufmgr_gem.c
+++ b/intel/intel_bufmgr_gem.c
@@ -132,6 +132,11 @@  typedef struct _drm_intel_bufmgr_gem {
 	unsigned int has_vebox : 1;
 	bool fenced_relocs;
 
+	struct {
+		void *ptr;
+		uint32_t handle;
+	} userptr_active;
+
 	char *aub_filename;
 	FILE *aub_file;
 	uint32_t aub_offset;
@@ -943,7 +948,6 @@  has_userptr(drm_intel_bufmgr_gem *bufmgr_gem)
 	void *ptr;
 	long pgsz;
 	struct drm_i915_gem_userptr userptr;
-	struct drm_gem_close close_bo;
 
 	pgsz = sysconf(_SC_PAGESIZE);
 	assert(pgsz > 0);
@@ -970,15 +974,15 @@  retry:
 		return false;
 	}
 
-	memclear(close_bo);
-	close_bo.handle = userptr.handle;
-	ret = drmIoctl(bufmgr_gem->fd, DRM_IOCTL_GEM_CLOSE, &close_bo);
-	free(ptr);
-	if (ret) {
-		fprintf(stderr, "Failed to release test userptr object! (%d) "
-				"i915 kernel driver may not be sane!\n", errno);
-		return false;
-	}
+	/* We don't release the userptr bo here as we want to keep the
+	 * kernel mm tracking alive for our lifetime. The first time we
+	 * create a userptr object the kernel has to install a mmu_notifer
+	 * which is a heavyweight operation (e.g. it requires taking all
+	 * mm_locks and stop_machine()).
+	 */
+
+	bufmgr_gem->userptr_active.ptr = ptr;
+	bufmgr_gem->userptr_active.handle = userptr.handle;
 
 	return true;
 }
@@ -1805,7 +1809,8 @@  static void
 drm_intel_bufmgr_gem_destroy(drm_intel_bufmgr *bufmgr)
 {
 	drm_intel_bufmgr_gem *bufmgr_gem = (drm_intel_bufmgr_gem *) bufmgr;
-	int i;
+	struct drm_gem_close close_bo;
+	int i, ret;
 
 	free(bufmgr_gem->exec2_objects);
 	free(bufmgr_gem->exec_objects);
@@ -1829,6 +1834,18 @@  drm_intel_bufmgr_gem_destroy(drm_intel_bufmgr *bufmgr)
 		}
 	}
 
+	/* Release userptr bo kept hanging around for optimisation. */
+	if (bufmgr_gem->userptr_active.ptr) {
+		memclear(close_bo);
+		close_bo.handle = bufmgr_gem->userptr_active.handle;
+		ret = drmIoctl(bufmgr_gem->fd, DRM_IOCTL_GEM_CLOSE, &close_bo);
+		free(bufmgr_gem->userptr_active.ptr);
+		if (ret)
+			fprintf(stderr,
+				"Failed to release test userptr object! (%d) "
+				"i915 kernel driver may not be sane!\n", errno);
+	}
+
 	free(bufmgr);
 }
 

Comments

On Fri, Apr 17, 2015 at 11:57:28AM +0100, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> In order to use userptr, the kernel tracks the owner's mm with a
> mmu_notifier. Setting that is very expensive - it involves taking all
> mm_locks and a stop_machine(). This tracking lives only for as long as
> the client is using userptr objects - so if the client allocates then
> frees a userptr in a loop, we will be executing that heavyweight setup
> everytime. To ammoritize this cost, just leak the test bo and the single
> backing page we use for detecting userptr.
> 
> v2: Free the object and memory when bufmgr is destroyed.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
Reveiwed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
On Fri, Apr 17, 2015 at 12:22:00PM +0100, Chris Wilson wrote:
> On Fri, Apr 17, 2015 at 11:57:28AM +0100, Tvrtko Ursulin wrote:
> > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > 
> > In order to use userptr, the kernel tracks the owner's mm with a
> > mmu_notifier. Setting that is very expensive - it involves taking all
> > mm_locks and a stop_machine(). This tracking lives only for as long as
> > the client is using userptr objects - so if the client allocates then
> > frees a userptr in a loop, we will be executing that heavyweight setup
> > everytime. To ammoritize this cost, just leak the test bo and the single
> > backing page we use for detecting userptr.
> > 
> > v2: Free the object and memory when bufmgr is destroyed.
> > 
> > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Reveiwed-by: Chris Wilson <chris@chris-wilson.co.uk>

s/vei/vie/ and merged. Thanks for the patch and review (could it be time
to ask for push access?)