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

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

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@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@intel.com>
>> Cc: Chris Wilson <chris@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.

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