On 07/14/2014 11:34 AM, Chris Wilson wrote:
On Mon, Jul 14, 2014 at 11:03:26AM +0100, Tvrtko Ursulin wrote:
From: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
Userptr v23 was not thread safe against memory map operations and object
creation from separate threads. MMU notifier callback would get triggered
on a partially constructed object causing a NULL pointer dereference.
This test excercises that path a bit. In my testing it would trigger it
every time and easily, but unfortunately a test pass here does not guarantee
the absence of the race.
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
---
tests/Makefile.am | 2 ++
tests/gem_userptr_blits.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 56 insertions(+)
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 2878624..e207509 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -65,6 +65,8 @@ prime_self_import_CFLAGS = $(AM_CFLAGS) $(THREAD_CFLAGS)
prime_self_import_LDADD = $(LDADD) -lpthread
gen7_forcewake_mt_CFLAGS = $(AM_CFLAGS) $(THREAD_CFLAGS)
gen7_forcewake_mt_LDADD = $(LDADD) -lpthread
+gem_userptr_blits_CFLAGS = $(AM_CFLAGS) $(THREAD_CFLAGS)
+gem_userptr_blits_LDADD = $(LDADD) -lpthread
gem_wait_render_timeout_LDADD = $(LDADD) -lrt
kms_flip_LDADD = $(LDADD) -lrt -lpthread
diff --git a/tests/gem_userptr_blits.c b/tests/gem_userptr_blits.c
index 2eb127f..0213868 100644
--- a/tests/gem_userptr_blits.c
+++ b/tests/gem_userptr_blits.c
@@ -47,6 +47,7 @@
#include <sys/time.h>
#include <sys/mman.h>
#include <signal.h>
+#include <pthread.h>
#include "drm.h"
#include "i915_drm.h"
@@ -1107,6 +1108,56 @@ static int test_unmap_cycles(int fd, int expected)
return 0;
}
+static volatile int stop_mm_stress_thread;
+static void *mm_stress_thread(void *data)
+{
+ void *ptr;
+ int ret;
+
+ while (!stop_mm_stress_thread) {
+ ptr = mmap(NULL, PAGE_SIZE, PROT_READ | PROT_WRITE,
+ MAP_ANONYMOUS | MAP_PRIVATE, -1, 0);
+ assert(ptr != MAP_FAILED);
+ ret = munmap(ptr, PAGE_SIZE);
+ assert(ret == 0);
+ }
+
+ return NULL;
+}
+
+static int test_stress_mm(int fd)
+{
+ int ret;
+ pthread_t t;
+ unsigned int loops = 100000;
+ uint32_t handle;
+ void *ptr;
+
+ assert(posix_memalign(&ptr, PAGE_SIZE, PAGE_SIZE) == 0);
+
+ ret = pthread_create(&t, NULL, mm_stress_thread, NULL);
+ assert(ret == 0);
+
+ while (loops--) {
+ ret = gem_userptr(fd, ptr, PAGE_SIZE, 0, &handle);
+ assert(ret == 0);
+
+ gem_close(fd, handle);
+ }
+
+ stop_mm_stress_thread = 1;
+
+ free(ptr);
+
+ ret = pthread_cancel(t);
You don't have any cancellation points in the loop. (mmap may or may not
be, it is not required to be.)
But rather than use a global, just pass a pointer to a local struct.
It doesn't need both a cancellation point and a flag. Should I just add
pthread_testcancel in the loop and not have any flag at all?
Oh, and igt_assert. But kill the asserts in mm_stress_thread() first.
Why remove completely? My thinking was to use assert vs igt_assert to
distinguish between assumptions about system behaviour, and igt_assert
for assertions about tested functionality.
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
