From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tvrtko Ursulin Subject: Re: [PATCH] tests/gem_userptr_blits: Race between object creation and multi-threaded mm ops Date: Mon, 14 Jul 2014 11:44:26 +0100 Message-ID: <53C3B48A.2050306@linux.intel.com> References: <1405332206-6292-1-git-send-email-tvrtko.ursulin@linux.intel.com> <20140714103416.GB2319@nuc-i3427.alporthouse.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by gabe.freedesktop.org (Postfix) with ESMTP id E1E2D6E414 for ; Mon, 14 Jul 2014 03:44:28 -0700 (PDT) In-Reply-To: <20140714103416.GB2319@nuc-i3427.alporthouse.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Chris Wilson , Intel-gfx@lists.freedesktop.org, Tvrtko Ursulin List-Id: intel-gfx@lists.freedesktop.org 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 >> >> 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 >> Cc: Chris Wilson >> --- >> 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 >> #include >> #include >> +#include >> >> #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