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

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

Details

Message ID 1429788197-10817-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, 11:23 a.m.
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)
v3: use the flinked bo as batch (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 | 76 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 76 insertions(+)

Patch hide | download patch | download mbox

diff --git a/tests/gem_ppgtt.c b/tests/gem_ppgtt.c
index 5bf773c..882a884 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,63 @@  static void surfaces_check(dri_bo **bo, int count, uint32_t expected)
 	}
 }
 
+static uint64_t exec_and_get_offset(int fd, uint32_t batch)
+{
+	struct drm_i915_gem_execbuffer2 execbuf;
+	struct drm_i915_gem_exec_object2 exec[1];
+	uint32_t batch_data[2] = { MI_NOOP, MI_BATCH_BUFFER_END };
+
+	gem_write(fd, batch, 0, batch_data, sizeof(batch_data));
+
+	memset(exec, 0, sizeof(exec));
+	exec[0].handle = batch;
+	exec[0].relocation_count = 0;
+	exec[0].relocs_ptr = 0;
+
+	memset(&execbuf, 0, sizeof(execbuf));
+	execbuf.buffers_ptr = (uintptr_t)exec;
+	execbuf.buffer_count = 1;
+	execbuf.batch_len = sizeof(batch_data);
+	execbuf.flags = 0;
+
+	gem_execbuf(fd, &execbuf);
+
+	return exec[0].offset;
+}
+
+static void flink_and_close(void)
+{
+	uint32_t fd, fd2;
+	uint32_t 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();
+
+	flinked_bo = gem_open(fd2, name);
+	offset = exec_and_get_offset(fd2, flinked_bo);
+	gem_sync(fd2, flinked_bo);
+	gem_close(fd2, flinked_bo);
+
+	/* 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, new_bo);
+	gem_close(fd2, new_bo);
+
+	igt_assert_eq(offset, offset_new);
+
+	gem_close(fd, bo);
+	close(fd);
+	close(fd2);
+}
+
 #define N_CHILD 8
 int main(int argc, char **argv)
 {
@@ -229,5 +302,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 12:23:17PM +0100, daniele.ceraolospurio@intel.com wrote:
> 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)
> v3: use the flinked bo as batch (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>

Bikeshedding for fun aside,
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

> +static uint64_t exec_and_get_offset(int fd, uint32_t batch)
> +{
> +	struct drm_i915_gem_execbuffer2 execbuf;
> +	struct drm_i915_gem_exec_object2 exec[1];
> +	uint32_t batch_data[2] = { MI_NOOP, MI_BATCH_BUFFER_END };

uint32_t buf[2] = { MI_BATCH_BUFFER_END };

> +
> +	gem_write(fd, batch, 0, batch_data, sizeof(batch_data));
> +
> +	memset(exec, 0, sizeof(exec));
> +	exec[0].handle = batch;
> +	exec[0].relocation_count = 0;
> +	exec[0].relocs_ptr = 0;

We just memset(0) these two, so we don't need to clear them again.

> +	memset(&execbuf, 0, sizeof(execbuf));
> +	execbuf.buffers_ptr = (uintptr_t)exec;
> +	execbuf.buffer_count = 1;
> +	execbuf.batch_len = sizeof(batch_data);
> +	execbuf.flags = 0;
These two can also happily disappear.

> +	gem_execbuf(fd, &execbuf);

igt_assert_neq(exec[0].offset, -1)
-Chris
On 04/23/2015 12:23 PM, daniele.ceraolospurio@intel.com wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

I have least to do with the current version so best upgrade yourself as 
author now!

Regards,

Tvrtko
On 4/23/2015 12:36 PM, Chris Wilson wrote:
> On Thu, Apr 23, 2015 at 12:23:17PM +0100, daniele.ceraolospurio@intel.com wrote:
>> 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)
>> v3: use the flinked bo as batch (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>
>
> Bikeshedding for fun aside,
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
>
>> +static uint64_t exec_and_get_offset(int fd, uint32_t batch)
>> +{
>> +	struct drm_i915_gem_execbuffer2 execbuf;
>> +	struct drm_i915_gem_exec_object2 exec[1];
>> +	uint32_t batch_data[2] = { MI_NOOP, MI_BATCH_BUFFER_END };
>
> uint32_t buf[2] = { MI_BATCH_BUFFER_END };
>
>> +
>> +	gem_write(fd, batch, 0, batch_data, sizeof(batch_data));
>> +
>> +	memset(exec, 0, sizeof(exec));
>> +	exec[0].handle = batch;
>> +	exec[0].relocation_count = 0;
>> +	exec[0].relocs_ptr = 0;
>
> We just memset(0) these two, so we don't need to clear them again.
>
>> +	memset(&execbuf, 0, sizeof(execbuf));
>> +	execbuf.buffers_ptr = (uintptr_t)exec;
>> +	execbuf.buffer_count = 1;
>> +	execbuf.batch_len = sizeof(batch_data);
>> +	execbuf.flags = 0;
> These two can also happily disappear.

Ok for the execbuf.flags and for the other fixes, but why remove also 
execbuf.batch_len?

Thanks,
Daniele

>
>> +	gem_execbuf(fd, &execbuf);
>
> igt_assert_neq(exec[0].offset, -1)
> -Chris
>
On Thu, Apr 23, 2015 at 02:01:43PM +0100, Ceraolo Spurio, Daniele wrote:
> On 4/23/2015 12:36 PM, Chris Wilson wrote:
> >>+	memset(&execbuf, 0, sizeof(execbuf));
> >>+	execbuf.buffers_ptr = (uintptr_t)exec;
> >>+	execbuf.buffer_count = 1;
> >>+	execbuf.batch_len = sizeof(batch_data);
> >>+	execbuf.flags = 0;
> >These two can also happily disappear.
> 
> Ok for the execbuf.flags and for the other fixes, but why remove
> also execbuf.batch_len?

Then we get flagged as a no-op batch. It's just a piece of
sleight-of-hand.
-Chris