[v2,i-g-t] tests/gem_ppgtt: Check for vm leaks with flink and ppgtt

Submitted by Daniele Ceraolo Spurio on April 23, 2015, 9:30 a.m.

Details

Message ID 1429781401-2008-1-git-send-email-daniele.ceraolospurio@intel.com
State New
Headers show

Not browsing as part of any series.

Commit Message

Daniele Ceraolo Spurio April 23, 2015, 9:30 a.m.
From: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Using imported objects should not leak i915 vmas (and vms).

In practice this simulates Xorg importing fbcon and leaking (or not) one vma
per Xorg startup cycle.

v2: use low-level ioctl wrappers and bo offset to check the leak (Chris)

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> (v2)
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 tests/gem_ppgtt.c | 99 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 99 insertions(+)

Patch hide | download patch | download mbox

diff --git a/tests/gem_ppgtt.c b/tests/gem_ppgtt.c
index 5bf773c..b865af3 100644
--- a/tests/gem_ppgtt.c
+++ b/tests/gem_ppgtt.c
@@ -48,6 +48,22 @@ 
 #define HEIGHT 512
 #define SIZE (HEIGHT*STRIDE)
 
+static bool uses_full_ppgtt(int fd)
+{
+	struct drm_i915_getparam gp;
+	int val = 0;
+
+	memset(&gp, 0, sizeof(gp));
+	gp.param = 18; /* HAS_ALIASING_PPGTT */
+	gp.value = &val;
+
+	if (drmIoctl(fd, DRM_IOCTL_I915_GETPARAM, &gp))
+		return 0;
+
+	errno = 0;
+	return val > 1;
+}
+
 static drm_intel_bo *create_bo(drm_intel_bufmgr *bufmgr,
 			       uint32_t pixel)
 {
@@ -200,6 +216,86 @@  static void surfaces_check(dri_bo **bo, int count, uint32_t expected)
 	}
 }
 
+
+static uint64_t exec_and_get_offset(int fd, uint32_t batch, uint32_t bo)
+{
+	struct drm_i915_gem_execbuffer2 execbuf;
+	struct drm_i915_gem_exec_object2 exec[2];
+	struct drm_i915_gem_relocation_entry reloc[1];
+	uint32_t buf[6], i = 0;
+
+	/* use a simple MI_STORE_DWORD_IMM to write something on the bo.
+	 * We just want to get a VMA
+	 */
+	buf[i++] = MI_STORE_DWORD_IMM | 2;
+	buf[i++] = 0;
+	buf[i++] = 0;
+	buf[i++] = 0xdeadbeef;
+
+	reloc->offset = 1 * sizeof(uint32_t);
+	reloc->delta = 0;
+	reloc->target_handle = bo;
+	reloc->read_domains = I915_GEM_DOMAIN_INSTRUCTION;
+	reloc->write_domain = I915_GEM_DOMAIN_INSTRUCTION;
+	reloc->presumed_offset = 0;
+
+	buf[i++] = 0;
+	buf[i++] = MI_BATCH_BUFFER_END;
+
+	gem_write(fd, batch, 0, buf, i * sizeof(uint32_t));
+
+	memset(exec, 0, sizeof(exec));
+	exec[0].handle = bo;
+	exec[1].handle = batch;
+	exec[1].relocation_count = 1;
+	exec[1].relocs_ptr = (uintptr_t)reloc;
+
+	memset(&execbuf, 0, sizeof(execbuf));
+	execbuf.buffers_ptr = (uintptr_t)exec;
+	execbuf.buffer_count = 2;
+	execbuf.batch_len = i * sizeof(uint32_t);
+	execbuf.flags = 0;
+
+	gem_execbuf(fd, &execbuf);
+
+	return exec[0].offset;
+}
+
+static void flink_and_close(void)
+{
+	uint32_t fd, fd2;
+	uint32_t batch, bo, flinked_bo, new_bo, name;
+	uint64_t offset, offset_new;
+
+	fd = drm_open_any();
+	igt_require(uses_full_ppgtt(fd));
+
+	bo = gem_create(fd, 4096);
+	name = gem_flink(fd, bo);
+
+	fd2 = drm_open_any();
+	batch = gem_create(fd2, 4096);
+
+	flinked_bo = gem_open(fd2, name);
+	offset = exec_and_get_offset(fd2, batch, flinked_bo);
+	gem_close(fd2, flinked_bo);
+	gem_sync(fd2, batch);
+
+	/* the flinked bo VMA should have been cleared now, so a new bo of the
+	 * same size should get the same offset
+	 */
+	new_bo = gem_create(fd2, 4096);
+	offset_new = exec_and_get_offset(fd2, batch, new_bo);
+	gem_close(fd2, new_bo);
+
+	igt_assert(offset == offset_new);
+
+	gem_close(fd2, batch);
+	gem_close(fd, bo);
+	close(fd);
+	close(fd2);
+}
+
 #define N_CHILD 8
 int main(int argc, char **argv)
 {
@@ -229,5 +325,8 @@  int main(int argc, char **argv)
 		surfaces_check(rcs, N_CHILD, 0x8000 / N_CHILD);
 	}
 
+	igt_subtest("flink-and-close-vma-leak")
+		flink_and_close();
+
 	igt_exit();
 }

Comments

On Thu, Apr 23, 2015 at 10:30:01AM +0100, daniele.ceraolospurio@intel.com wrote:
> From: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> 
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Using imported objects should not leak i915 vmas (and vms).
> 
> In practice this simulates Xorg importing fbcon and leaking (or not) one vma
> per Xorg startup cycle.
> 
> v2: use low-level ioctl wrappers and bo offset to check the leak (Chris)
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> (v2)
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>  tests/gem_ppgtt.c | 99 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 99 insertions(+)
> 
> diff --git a/tests/gem_ppgtt.c b/tests/gem_ppgtt.c
> index 5bf773c..b865af3 100644
> --- a/tests/gem_ppgtt.c
> +++ b/tests/gem_ppgtt.c
> @@ -48,6 +48,22 @@
>  #define HEIGHT 512
>  #define SIZE (HEIGHT*STRIDE)
>  
> +static bool uses_full_ppgtt(int fd)
> +{
> +	struct drm_i915_getparam gp;
> +	int val = 0;
> +
> +	memset(&gp, 0, sizeof(gp));
> +	gp.param = 18; /* HAS_ALIASING_PPGTT */
> +	gp.value = &val;
> +
> +	if (drmIoctl(fd, DRM_IOCTL_I915_GETPARAM, &gp))
> +		return 0;
> +
> +	errno = 0;
> +	return val > 1;
> +}
> +
>  static drm_intel_bo *create_bo(drm_intel_bufmgr *bufmgr,
>  			       uint32_t pixel)
>  {
> @@ -200,6 +216,86 @@ static void surfaces_check(dri_bo **bo, int count, uint32_t expected)
>  	}
>  }
>  
> +
> +static uint64_t exec_and_get_offset(int fd, uint32_t batch, uint32_t bo)
> +{
> +	struct drm_i915_gem_execbuffer2 execbuf;
> +	struct drm_i915_gem_exec_object2 exec[2];
> +	struct drm_i915_gem_relocation_entry reloc[1];
> +	uint32_t buf[6], i = 0;
> +
> +	/* use a simple MI_STORE_DWORD_IMM to write something on the bo.
> +	 * We just want to get a VMA
> +	 */
> +	buf[i++] = MI_STORE_DWORD_IMM | 2;
> +	buf[i++] = 0;
> +	buf[i++] = 0;
> +	buf[i++] = 0xdeadbeef;
> +
> +	reloc->offset = 1 * sizeof(uint32_t);
> +	reloc->delta = 0;
> +	reloc->target_handle = bo;
> +	reloc->read_domains = I915_GEM_DOMAIN_INSTRUCTION;
> +	reloc->write_domain = I915_GEM_DOMAIN_INSTRUCTION;
> +	reloc->presumed_offset = 0;

A relocation (and the STORE) is not required. The kernel will do all
reservations (of exec_object[]) before doing relocations. Not relocating
makes life more predictable (fewer error conditions may strike).

> +static void flink_and_close(void)
> +{
> +	uint32_t fd, fd2;
> +	uint32_t batch, bo, flinked_bo, new_bo, name;
> +	uint64_t offset, offset_new;
> +
> +	fd = drm_open_any();
> +	igt_require(uses_full_ppgtt(fd));

The test equally applies to !full-ppgtt. The bug we saw isn't possible,
but the interface expectations are the same.

> +	bo = gem_create(fd, 4096);
> +	name = gem_flink(fd, bo);
> +
> +	fd2 = drm_open_any();
> +	batch = gem_create(fd2, 4096);
> +
> +	flinked_bo = gem_open(fd2, name);
> +	offset = exec_and_get_offset(fd2, batch, flinked_bo);
> +	gem_close(fd2, flinked_bo);
> +	gem_sync(fd2, batch);
> +
> +	/* the flinked bo VMA should have been cleared now, so a new bo of the
> +	 * same size should get the same offset
> +	 */
> +	new_bo = gem_create(fd2, 4096);
> +	offset_new = exec_and_get_offset(fd2, batch, new_bo);
> +	gem_close(fd2, new_bo);
> +
> +	igt_assert(offset == offset_new);

igt_assert_eq

Nice test.
-Chris
On 4/23/2015 10:43 AM, Chris Wilson wrote:
> On Thu, Apr 23, 2015 at 10:30:01AM +0100, daniele.ceraolospurio@intel.com wrote:
>> From: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>>
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> Using imported objects should not leak i915 vmas (and vms).
>>
>> In practice this simulates Xorg importing fbcon and leaking (or not) one vma
>> per Xorg startup cycle.
>>
>> v2: use low-level ioctl wrappers and bo offset to check the leak (Chris)
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> (v2)
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> ---
>>   tests/gem_ppgtt.c | 99 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 99 insertions(+)
>>
>> diff --git a/tests/gem_ppgtt.c b/tests/gem_ppgtt.c
>> index 5bf773c..b865af3 100644
>> --- a/tests/gem_ppgtt.c
>> +++ b/tests/gem_ppgtt.c
>> @@ -48,6 +48,22 @@
>>   #define HEIGHT 512
>>   #define SIZE (HEIGHT*STRIDE)
>>
>> +static bool uses_full_ppgtt(int fd)
>> +{
>> +	struct drm_i915_getparam gp;
>> +	int val = 0;
>> +
>> +	memset(&gp, 0, sizeof(gp));
>> +	gp.param = 18; /* HAS_ALIASING_PPGTT */
>> +	gp.value = &val;
>> +
>> +	if (drmIoctl(fd, DRM_IOCTL_I915_GETPARAM, &gp))
>> +		return 0;
>> +
>> +	errno = 0;
>> +	return val > 1;
>> +}
>> +
>>   static drm_intel_bo *create_bo(drm_intel_bufmgr *bufmgr,
>>   			       uint32_t pixel)
>>   {
>> @@ -200,6 +216,86 @@ static void surfaces_check(dri_bo **bo, int count, uint32_t expected)
>>   	}
>>   }
>>
>> +
>> +static uint64_t exec_and_get_offset(int fd, uint32_t batch, uint32_t bo)
>> +{
>> +	struct drm_i915_gem_execbuffer2 execbuf;
>> +	struct drm_i915_gem_exec_object2 exec[2];
>> +	struct drm_i915_gem_relocation_entry reloc[1];
>> +	uint32_t buf[6], i = 0;
>> +
>> +	/* use a simple MI_STORE_DWORD_IMM to write something on the bo.
>> +	 * We just want to get a VMA
>> +	 */
>> +	buf[i++] = MI_STORE_DWORD_IMM | 2;
>> +	buf[i++] = 0;
>> +	buf[i++] = 0;
>> +	buf[i++] = 0xdeadbeef;
>> +
>> +	reloc->offset = 1 * sizeof(uint32_t);
>> +	reloc->delta = 0;
>> +	reloc->target_handle = bo;
>> +	reloc->read_domains = I915_GEM_DOMAIN_INSTRUCTION;
>> +	reloc->write_domain = I915_GEM_DOMAIN_INSTRUCTION;
>> +	reloc->presumed_offset = 0;
>
> A relocation (and the STORE) is not required. The kernel will do all
> reservations (of exec_object[]) before doing relocations. Not relocating
> makes life more predictable (fewer error conditions may strike).
>
>> +static void flink_and_close(void)
>> +{
>> +	uint32_t fd, fd2;
>> +	uint32_t batch, bo, flinked_bo, new_bo, name;
>> +	uint64_t offset, offset_new;
>> +
>> +	fd = drm_open_any();
>> +	igt_require(uses_full_ppgtt(fd));
>
> The test equally applies to !full-ppgtt. The bug we saw isn't possible,
> but the interface expectations are the same.

I've tried the test with aliasing ppgtt, but the flinked buffer gets the 
same offset as the original one, so the new_bo will get a different 
offset indipendently from the vma leak and the assert will always fail.
Are there any other ways to check the vma leak in !full-ppgtt mode?

Thanks,
Daniele

>
>> +	bo = gem_create(fd, 4096);
>> +	name = gem_flink(fd, bo);
>> +
>> +	fd2 = drm_open_any();
>> +	batch = gem_create(fd2, 4096);
>> +
>> +	flinked_bo = gem_open(fd2, name);
>> +	offset = exec_and_get_offset(fd2, batch, flinked_bo);
>> +	gem_close(fd2, flinked_bo);
>> +	gem_sync(fd2, batch);
>> +
>> +	/* the flinked bo VMA should have been cleared now, so a new bo of the
>> +	 * same size should get the same offset
>> +	 */
>> +	new_bo = gem_create(fd2, 4096);
>> +	offset_new = exec_and_get_offset(fd2, batch, new_bo);
>> +	gem_close(fd2, new_bo);
>> +
>> +	igt_assert(offset == offset_new);
>
> igt_assert_eq
>
> Nice test.
> -Chris
>
On Thu, Apr 23, 2015 at 11:14:12AM +0100, Ceraolo Spurio, Daniele wrote:
> On 4/23/2015 10:43 AM, Chris Wilson wrote:
> >On Thu, Apr 23, 2015 at 10:30:01AM +0100, daniele.ceraolospurio@intel.com wrote:
> >>From: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> >>
> >>From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >>
> >>Using imported objects should not leak i915 vmas (and vms).
> >>
> >>In practice this simulates Xorg importing fbcon and leaking (or not) one vma
> >>per Xorg startup cycle.
> >>
> >>v2: use low-level ioctl wrappers and bo offset to check the leak (Chris)
> >>
> >>Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >>Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> (v2)
> >>Cc: Chris Wilson <chris@chris-wilson.co.uk>
> >>Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >>---
> >>  tests/gem_ppgtt.c | 99 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >>  1 file changed, 99 insertions(+)
> >>
> >>diff --git a/tests/gem_ppgtt.c b/tests/gem_ppgtt.c
> >>index 5bf773c..b865af3 100644
> >>--- a/tests/gem_ppgtt.c
> >>+++ b/tests/gem_ppgtt.c
> >>@@ -48,6 +48,22 @@
> >>  #define HEIGHT 512
> >>  #define SIZE (HEIGHT*STRIDE)
> >>
> >>+static bool uses_full_ppgtt(int fd)
> >>+{
> >>+	struct drm_i915_getparam gp;
> >>+	int val = 0;
> >>+
> >>+	memset(&gp, 0, sizeof(gp));
> >>+	gp.param = 18; /* HAS_ALIASING_PPGTT */
> >>+	gp.value = &val;
> >>+
> >>+	if (drmIoctl(fd, DRM_IOCTL_I915_GETPARAM, &gp))
> >>+		return 0;
> >>+
> >>+	errno = 0;
> >>+	return val > 1;
> >>+}
> >>+
> >>  static drm_intel_bo *create_bo(drm_intel_bufmgr *bufmgr,
> >>  			       uint32_t pixel)
> >>  {
> >>@@ -200,6 +216,86 @@ static void surfaces_check(dri_bo **bo, int count, uint32_t expected)
> >>  	}
> >>  }
> >>
> >>+
> >>+static uint64_t exec_and_get_offset(int fd, uint32_t batch, uint32_t bo)
> >>+{
> >>+	struct drm_i915_gem_execbuffer2 execbuf;
> >>+	struct drm_i915_gem_exec_object2 exec[2];
> >>+	struct drm_i915_gem_relocation_entry reloc[1];
> >>+	uint32_t buf[6], i = 0;
> >>+
> >>+	/* use a simple MI_STORE_DWORD_IMM to write something on the bo.
> >>+	 * We just want to get a VMA
> >>+	 */
> >>+	buf[i++] = MI_STORE_DWORD_IMM | 2;
> >>+	buf[i++] = 0;
> >>+	buf[i++] = 0;
> >>+	buf[i++] = 0xdeadbeef;
> >>+
> >>+	reloc->offset = 1 * sizeof(uint32_t);
> >>+	reloc->delta = 0;
> >>+	reloc->target_handle = bo;
> >>+	reloc->read_domains = I915_GEM_DOMAIN_INSTRUCTION;
> >>+	reloc->write_domain = I915_GEM_DOMAIN_INSTRUCTION;
> >>+	reloc->presumed_offset = 0;
> >
> >A relocation (and the STORE) is not required. The kernel will do all
> >reservations (of exec_object[]) before doing relocations. Not relocating
> >makes life more predictable (fewer error conditions may strike).

Actually thought of another potential issue. The kernel will reserve
space in the GTT for an execbuffer in an arbitrary order. So kill the
separate batch entirely, and just use flinked objects (as the batch
buffer).

> >>+static void flink_and_close(void)
> >>+{
> >>+	uint32_t fd, fd2;
> >>+	uint32_t batch, bo, flinked_bo, new_bo, name;
> >>+	uint64_t offset, offset_new;
> >>+
> >>+	fd = drm_open_any();
> >>+	igt_require(uses_full_ppgtt(fd));
> >
> >The test equally applies to !full-ppgtt. The bug we saw isn't possible,
> >but the interface expectations are the same.
> 
> I've tried the test with aliasing ppgtt, but the flinked buffer gets
> the same offset as the original one, so the new_bo will get a
> different offset indipendently from the vma leak and the assert will
> always fail.
> Are there any other ways to check the vma leak in !full-ppgtt mode?

Oh right. Because we lazy-unbind and the object still exists within the
aliasing vm so the eviction is not forced.

I give in, I can't think of a way to test this on !full-ppgtt without
explicitly unbinding the vma, thereby breaking the neat test.
-Chris