[Intel-gfx] [PATCH] tests/gem_userptr_blits: Race between object creation and multi-threaded mm ops

[Chris Wilson]

On Mon, Jul 14, 2014 at 11:03:26AM +0100, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> 
> 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 at intel.com>
> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> ---
>  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.

Oh, and igt_assert. But kill the asserts in mm_stress_thread() first.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre