* [PATCH 2/3] igt/gem_userptr_blits: Shared memory allocations
2014-07-11 9:40 [PATCH 1/3] Wait for any pid in order to reap failure quicker Chris Wilson
@ 2014-07-11 9:40 ` Chris Wilson
2014-07-11 10:30 ` Tvrtko Ursulin
2014-07-11 9:40 ` [PATCH 3/3] igt/gem_userptr_blits: Verify that userptr bo work on unshared memory Chris Wilson
2014-07-11 11:56 ` [PATCH 1/3] Wait for any pid in order to reap failure quicker Tvrtko Ursulin
2 siblings, 1 reply; 6+ messages in thread
From: Chris Wilson @ 2014-07-11 9:40 UTC (permalink / raw)
To: intel-gfx
The forked tests allocate the bo (and thus for userptr, the memory) in
the parent and pass them to all children. The difference for userptr is
that we allocate system memory which the kernel then copies into each
child. As the children need to access the memory for their checks, it
does need to be shared - so allocate the userptr from shared memory!
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=80208
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
tests/gem_userptr_blits.c | 39 ++++++++++++++++++++++++++++++---------
1 file changed, 30 insertions(+), 9 deletions(-)
diff --git a/tests/gem_userptr_blits.c b/tests/gem_userptr_blits.c
index 14efdda..9cf681b 100644
--- a/tests/gem_userptr_blits.c
+++ b/tests/gem_userptr_blits.c
@@ -321,17 +321,32 @@ create_userptr(int fd, uint32_t val, uint32_t *ptr)
}
static void **handle_ptr_map;
-static unsigned int num_handle_ptr_map;
+static int *handle_size_map;
+static unsigned int num_handle_map;
-static void add_handle_ptr(uint32_t handle, void *ptr)
+static void reset_handle_ptr(void)
{
- if (handle >= num_handle_ptr_map) {
+ free(handle_ptr_map);
+ free(handle_size_map);
+ num_handle_map = 0;
+}
+
+static void add_handle_ptr(uint32_t handle, void *ptr, int size)
+{
+ if (handle >= num_handle_map) {
handle_ptr_map = realloc(handle_ptr_map,
(handle + 1000) * sizeof(void*));
- num_handle_ptr_map = handle + 1000;
+ igt_assert(handle_ptr_map);
+
+ handle_size_map = realloc(handle_size_map,
+ (handle + 1000) * sizeof(int));
+ igt_assert(handle_size_map);
+
+ num_handle_map = handle + 1000;
}
handle_ptr_map[handle] = ptr;
+ handle_size_map[handle] = size;
}
static void *get_handle_ptr(uint32_t handle)
@@ -341,10 +356,10 @@ static void *get_handle_ptr(uint32_t handle)
static void free_handle_ptr(uint32_t handle)
{
- igt_assert(handle < num_handle_ptr_map);
+ igt_assert(handle < num_handle_map);
igt_assert(handle_ptr_map[handle]);
- free(handle_ptr_map[handle]);
+ munmap(handle_ptr_map[handle], handle_size_map[handle]);
handle_ptr_map[handle] = NULL;
}
@@ -354,12 +369,15 @@ static uint32_t create_userptr_bo(int fd, int size)
uint32_t handle;
int ret;
- ret = posix_memalign(&ptr, PAGE_SIZE, size);
- igt_assert(ret == 0);
+ ptr = mmap(NULL, size,
+ PROT_READ | PROT_WRITE,
+ MAP_ANONYMOUS | MAP_SHARED,
+ -1, 0);
+ igt_assert(ptr != MAP_FAILED);
ret = gem_userptr(fd, (uint32_t *)ptr, size, 0, &handle);
igt_assert(ret == 0);
- add_handle_ptr(handle, ptr);
+ add_handle_ptr(handle, ptr, size);
return handle;
}
@@ -641,6 +659,7 @@ static void sigbus(int sig, siginfo_t *info, void *param)
MAP_ANONYMOUS | MAP_PRIVATE | MAP_FIXED, -1, 0);
if ((unsigned long)addr == ptr) {
memset(addr, counter, sizeof(linear));
+ munmap(addr, sizeof(linear));
return;
}
}
@@ -708,6 +727,8 @@ static int test_dmabuf(void)
close(fd1);
close(fd2);
+ reset_handle_ptr();
+
return 0;
}
--
2.0.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH 2/3] igt/gem_userptr_blits: Shared memory allocations
2014-07-11 9:40 ` [PATCH 2/3] igt/gem_userptr_blits: Shared memory allocations Chris Wilson
@ 2014-07-11 10:30 ` Tvrtko Ursulin
0 siblings, 0 replies; 6+ messages in thread
From: Tvrtko Ursulin @ 2014-07-11 10:30 UTC (permalink / raw)
To: Chris Wilson, intel-gfx
On 07/11/2014 10:40 AM, Chris Wilson wrote:
> The forked tests allocate the bo (and thus for userptr, the memory) in
> the parent and pass them to all children. The difference for userptr is
> that we allocate system memory which the kernel then copies into each
> child. As the children need to access the memory for their checks, it
> does need to be shared - so allocate the userptr from shared memory!
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=80208
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
> tests/gem_userptr_blits.c | 39 ++++++++++++++++++++++++++++++---------
> 1 file changed, 30 insertions(+), 9 deletions(-)
>
> diff --git a/tests/gem_userptr_blits.c b/tests/gem_userptr_blits.c
> index 14efdda..9cf681b 100644
> --- a/tests/gem_userptr_blits.c
> +++ b/tests/gem_userptr_blits.c
> @@ -321,17 +321,32 @@ create_userptr(int fd, uint32_t val, uint32_t *ptr)
> }
>
> static void **handle_ptr_map;
> -static unsigned int num_handle_ptr_map;
> +static int *handle_size_map;
> +static unsigned int num_handle_map;
>
> -static void add_handle_ptr(uint32_t handle, void *ptr)
> +static void reset_handle_ptr(void)
> {
> - if (handle >= num_handle_ptr_map) {
> + free(handle_ptr_map);
> + free(handle_size_map);
> + num_handle_map = 0;
> +}
> +
> +static void add_handle_ptr(uint32_t handle, void *ptr, int size)
> +{
> + if (handle >= num_handle_map) {
> handle_ptr_map = realloc(handle_ptr_map,
> (handle + 1000) * sizeof(void*));
> - num_handle_ptr_map = handle + 1000;
> + igt_assert(handle_ptr_map);
> +
> + handle_size_map = realloc(handle_size_map,
> + (handle + 1000) * sizeof(int));
> + igt_assert(handle_size_map);
> +
> + num_handle_map = handle + 1000;
> }
>
> handle_ptr_map[handle] = ptr;
> + handle_size_map[handle] = size;
> }
>
> static void *get_handle_ptr(uint32_t handle)
> @@ -341,10 +356,10 @@ static void *get_handle_ptr(uint32_t handle)
>
> static void free_handle_ptr(uint32_t handle)
> {
> - igt_assert(handle < num_handle_ptr_map);
> + igt_assert(handle < num_handle_map);
> igt_assert(handle_ptr_map[handle]);
>
> - free(handle_ptr_map[handle]);
> + munmap(handle_ptr_map[handle], handle_size_map[handle]);
> handle_ptr_map[handle] = NULL;
> }
>
> @@ -354,12 +369,15 @@ static uint32_t create_userptr_bo(int fd, int size)
> uint32_t handle;
> int ret;
>
> - ret = posix_memalign(&ptr, PAGE_SIZE, size);
> - igt_assert(ret == 0);
> + ptr = mmap(NULL, size,
> + PROT_READ | PROT_WRITE,
> + MAP_ANONYMOUS | MAP_SHARED,
> + -1, 0);
> + igt_assert(ptr != MAP_FAILED);
I wasn't sure about this straight away, but man page says "...on Linux,
the mapping will be created at a nearby page boundary..." so it's fine.
> ret = gem_userptr(fd, (uint32_t *)ptr, size, 0, &handle);
> igt_assert(ret == 0);
> - add_handle_ptr(handle, ptr);
> + add_handle_ptr(handle, ptr, size);
>
> return handle;
> }
> @@ -641,6 +659,7 @@ static void sigbus(int sig, siginfo_t *info, void *param)
> MAP_ANONYMOUS | MAP_PRIVATE | MAP_FIXED, -1, 0);
> if ((unsigned long)addr == ptr) {
> memset(addr, counter, sizeof(linear));
> + munmap(addr, sizeof(linear));
> return;
> }
> }
> @@ -708,6 +727,8 @@ static int test_dmabuf(void)
> close(fd1);
> close(fd2);
>
> + reset_handle_ptr();
> +
> return 0;
> }
>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 3/3] igt/gem_userptr_blits: Verify that userptr bo work on unshared memory
2014-07-11 9:40 [PATCH 1/3] Wait for any pid in order to reap failure quicker Chris Wilson
2014-07-11 9:40 ` [PATCH 2/3] igt/gem_userptr_blits: Shared memory allocations Chris Wilson
@ 2014-07-11 9:40 ` Chris Wilson
2014-07-11 10:45 ` Tvrtko Ursulin
2014-07-11 11:56 ` [PATCH 1/3] Wait for any pid in order to reap failure quicker Tvrtko Ursulin
2 siblings, 1 reply; 6+ messages in thread
From: Chris Wilson @ 2014-07-11 9:40 UTC (permalink / raw)
To: intel-gfx
If the parent passes a userptr to some private memory, we expect to
still be able to use the userptr in the child.
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
tests/gem_userptr_blits.c | 52 +++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 52 insertions(+)
diff --git a/tests/gem_userptr_blits.c b/tests/gem_userptr_blits.c
index 9cf681b..454db58 100644
--- a/tests/gem_userptr_blits.c
+++ b/tests/gem_userptr_blits.c
@@ -542,6 +542,55 @@ static int test_invalid_mapping(int fd)
return 0;
}
+static void test_forked_access(int fd)
+{
+ uint32_t handle1, handle2;
+ void *ptr1, *ptr2;
+ int ret;
+
+ ret = posix_memalign(&ptr1, PAGE_SIZE, PAGE_SIZE);
+ igt_assert(ret == 0);
+
+ ret = madvise(ptr1, PAGE_SIZE, MADV_DONTFORK);
+ igt_assert(ret == 0);
+
+ ret = gem_userptr(fd, ptr1, PAGE_SIZE, 0, &handle1);
+ igt_assert(ret == 0);
+
+ ret = posix_memalign(&ptr2, PAGE_SIZE, PAGE_SIZE);
+ igt_assert(ret == 0);
+
+ ret = madvise(ptr2, PAGE_SIZE, MADV_DONTFORK);
+ igt_assert(ret == 0);
+
+ ret = gem_userptr(fd, ptr2, PAGE_SIZE, 0, &handle2);
+ igt_assert(ret == 0);
+
+ memset(ptr1, 0x1, PAGE_SIZE);
+ memset(ptr2, 0x2, PAGE_SIZE);
+
+ igt_fork(child, 1) {
+ copy(fd, handle1, handle2, 0);
+ }
+ igt_waitchildren();
+
+ gem_userptr_sync(fd, handle1);
+ gem_userptr_sync(fd, handle2);
+
+ gem_close(fd, handle1);
+ gem_close(fd, handle2);
+
+ igt_assert(memcmp(ptr1, ptr2, PAGE_SIZE) == 0);
+
+ ret = madvise(ptr1, PAGE_SIZE, MADV_DOFORK);
+ igt_assert(ret == 0);
+ free(ptr1);
+
+ ret = madvise(ptr2, PAGE_SIZE, MADV_DOFORK);
+ igt_assert(ret == 0);
+ free(ptr2);
+}
+
static int test_forbidden_ops(int fd)
{
void *ptr;
@@ -1108,6 +1157,9 @@ int main(int argc, char **argv)
igt_subtest("invalid-mapping")
test_invalid_mapping(fd);
+ igt_subtest("forked-access")
+ test_forked_access(fd);
+
igt_subtest("forbidden-operations")
test_forbidden_ops(fd);
--
2.0.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH 3/3] igt/gem_userptr_blits: Verify that userptr bo work on unshared memory
2014-07-11 9:40 ` [PATCH 3/3] igt/gem_userptr_blits: Verify that userptr bo work on unshared memory Chris Wilson
@ 2014-07-11 10:45 ` Tvrtko Ursulin
0 siblings, 0 replies; 6+ messages in thread
From: Tvrtko Ursulin @ 2014-07-11 10:45 UTC (permalink / raw)
To: Chris Wilson, intel-gfx
On 07/11/2014 10:40 AM, Chris Wilson wrote:
> If the parent passes a userptr to some private memory, we expect to
> still be able to use the userptr in the child.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
> tests/gem_userptr_blits.c | 52 +++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 52 insertions(+)
>
> diff --git a/tests/gem_userptr_blits.c b/tests/gem_userptr_blits.c
> index 9cf681b..454db58 100644
> --- a/tests/gem_userptr_blits.c
> +++ b/tests/gem_userptr_blits.c
> @@ -542,6 +542,55 @@ static int test_invalid_mapping(int fd)
> return 0;
> }
>
> +static void test_forked_access(int fd)
> +{
> + uint32_t handle1, handle2;
> + void *ptr1, *ptr2;
> + int ret;
> +
> + ret = posix_memalign(&ptr1, PAGE_SIZE, PAGE_SIZE);
> + igt_assert(ret == 0);
> +
> + ret = madvise(ptr1, PAGE_SIZE, MADV_DONTFORK);
> + igt_assert(ret == 0);
> +
> + ret = gem_userptr(fd, ptr1, PAGE_SIZE, 0, &handle1);
> + igt_assert(ret == 0);
> +
> + ret = posix_memalign(&ptr2, PAGE_SIZE, PAGE_SIZE);
> + igt_assert(ret == 0);
> +
> + ret = madvise(ptr2, PAGE_SIZE, MADV_DONTFORK);
> + igt_assert(ret == 0);
> +
> + ret = gem_userptr(fd, ptr2, PAGE_SIZE, 0, &handle2);
> + igt_assert(ret == 0);
> +
> + memset(ptr1, 0x1, PAGE_SIZE);
> + memset(ptr2, 0x2, PAGE_SIZE);
> +
> + igt_fork(child, 1) {
> + copy(fd, handle1, handle2, 0);
> + }
> + igt_waitchildren();
> +
> + gem_userptr_sync(fd, handle1);
> + gem_userptr_sync(fd, handle2);
> +
> + gem_close(fd, handle1);
> + gem_close(fd, handle2);
> +
> + igt_assert(memcmp(ptr1, ptr2, PAGE_SIZE) == 0);
> +
> + ret = madvise(ptr1, PAGE_SIZE, MADV_DOFORK);
> + igt_assert(ret == 0);
> + free(ptr1);
> +
> + ret = madvise(ptr2, PAGE_SIZE, MADV_DOFORK);
> + igt_assert(ret == 0);
> + free(ptr2);
> +}
> +
> static int test_forbidden_ops(int fd)
> {
> void *ptr;
> @@ -1108,6 +1157,9 @@ int main(int argc, char **argv)
> igt_subtest("invalid-mapping")
> test_invalid_mapping(fd);
>
> + igt_subtest("forked-access")
> + test_forked_access(fd);
> +
> igt_subtest("forbidden-operations")
> test_forbidden_ops(fd);
>
>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/3] Wait for any pid in order to reap failure quicker
2014-07-11 9:40 [PATCH 1/3] Wait for any pid in order to reap failure quicker Chris Wilson
2014-07-11 9:40 ` [PATCH 2/3] igt/gem_userptr_blits: Shared memory allocations Chris Wilson
2014-07-11 9:40 ` [PATCH 3/3] igt/gem_userptr_blits: Verify that userptr bo work on unshared memory Chris Wilson
@ 2014-07-11 11:56 ` Tvrtko Ursulin
2 siblings, 0 replies; 6+ messages in thread
From: Tvrtko Ursulin @ 2014-07-11 11:56 UTC (permalink / raw)
To: Chris Wilson, intel-gfx
On 07/11/2014 10:40 AM, Chris Wilson wrote:
> When waiting for the forked tests, we can respond quicker to a failure
> (such as oom) by waiting for any child to exit rather than waiting for
> each child in order. Then when we see that a test failed, we can kill
> all other children before aborting.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
> lib/igt_core.c | 42 +++++++++++++++++++++++++++++++-----------
> 1 file changed, 31 insertions(+), 11 deletions(-)
>
> diff --git a/lib/igt_core.c b/lib/igt_core.c
> index 7ac7ebe..e5dc78b 100644
> --- a/lib/igt_core.c
> +++ b/lib/igt_core.c
> @@ -915,32 +915,52 @@ bool __igt_fork(void)
> */
> void igt_waitchildren(void)
> {
> + int err = 0;
> + int count;
> +
> assert(!test_child);
>
> - for (int nc = 0; nc < num_test_children; nc++) {
> + count = 0;
> + while (count < num_test_children) {
> int status = -1;
> - while (waitpid(test_children[nc], &status, 0) == -1 &&
> - errno == EINTR)
> - ;
> + pid_t pid;
> + int c;
>
> - if (status != 0) {
> + pid = wait(&status);
> + if (pid == -1)
> + continue;
Not sure if it would make sense to be more defensive and maybe assert on
some errors from wait(2) here (like ECHILD). But it is the same as
before the patch so doesn't really matter.
> +
> + for (c = 0; c < num_test_children; c++)
> + if (pid == test_children[c])
> + break;
> + if (c == num_test_children)
> + continue;
> +
> + if (err == 0 && status != 0) {
> if (WIFEXITED(status)) {
> printf("child %i failed with exit status %i\n",
> - nc, WEXITSTATUS(status));
> - igt_fail(WEXITSTATUS(status));
> + c, WEXITSTATUS(status));
> + err = WEXITSTATUS(status);
> } else if (WIFSIGNALED(status)) {
> printf("child %i died with signal %i, %s\n",
> - nc, WTERMSIG(status),
> + c, WTERMSIG(status),
> strsignal(WTERMSIG(status)));
> - igt_fail(99);
> + err = 128 + WTERMSIG(status);
> } else {
> - printf("Unhandled failure in child %i\n", nc);
> - abort();
> + printf("Unhandled failure [%d] in child %i\n", status, c);
> + err = 256;
> }
> +
> + for (c = 0; c < num_test_children; c++)
> + kill(test_children[c], SIGKILL);
> }
> +
> + count++;
> }
>
> num_test_children = 0;
> + if (err)
> + igt_fail(err);
> }
>
> /* exit handler code */
>
Looks fine.
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
^ permalink raw reply [flat|nested] 6+ messages in thread