* [PATCH i-g-t] lib/dummyload: Handle timeout in a new thread instead of signal handler
@ 2017-03-27 8:45 Ander Conselvan de Oliveira
2017-03-27 9:09 ` Chris Wilson
0 siblings, 1 reply; 5+ messages in thread
From: Ander Conselvan de Oliveira @ 2017-03-27 8:45 UTC (permalink / raw)
To: intel-gfx; +Cc: Ander Conselvan de Oliveira
Currently, the main thread needs to wakeup to run the signal handler
that ends a spin batch. When testing whether a function call succesfully
waits for a batch to complete, this behavior is undesired. It actually
invalidates the test.
Fix this by spawning a new thread to handle the timeout.
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
---
lib/igt_dummyload.c | 55 +++++++++++++++++------------------------------------
lib/igt_dummyload.h | 3 ++-
2 files changed, 19 insertions(+), 39 deletions(-)
diff --git a/lib/igt_dummyload.c b/lib/igt_dummyload.c
index 019c1fb..38b1930 100644
--- a/lib/igt_dummyload.c
+++ b/lib/igt_dummyload.c
@@ -24,6 +24,7 @@
#include "igt.h"
#include "igt_dummyload.h"
+#include <pthread.h>
#include <time.h>
#include <signal.h>
#include <sys/syscall.h>
@@ -166,6 +167,8 @@ igt_spin_batch_new(int fd, int engine, unsigned int dep_handle)
spin = calloc(1, sizeof(struct igt_spin));
igt_assert(spin);
+ pthread_mutex_init(&spin->mutex, NULL);
+
emit_recursive_batch(spin, fd, engine, dep_handle);
igt_assert(gem_bo_busy(fd, spin->handle));
@@ -174,27 +177,11 @@ igt_spin_batch_new(int fd, int engine, unsigned int dep_handle)
return spin;
}
-static void clear_sig_handler(int sig)
-{
- struct sigaction act;
-
- memset(&act, 0, sizeof(act));
- act.sa_handler = SIG_DFL;
- igt_assert(sigaction(sig, &act, NULL) == 0);
-}
-
-static void sig_handler(int sig, siginfo_t *info, void *arg)
+static void notify(union sigval arg)
{
- struct igt_spin *iter;
+ igt_spin_t *spin = arg.sival_ptr;
- igt_list_for_each(iter, &spin_list, link) {
- if (iter->signo == info->si_signo) {
- igt_spin_batch_end(iter);
- return;
- }
- }
-
- clear_sig_handler(sig);
+ igt_spin_batch_end(spin);
}
/**
@@ -208,43 +195,33 @@ static void sig_handler(int sig, siginfo_t *info, void *arg)
*/
void igt_spin_batch_set_timeout(igt_spin_t *spin, int64_t ns)
{
- static int spin_signo = 48; /* Midpoint of SIGRTMIN, SIGRTMAX */
timer_t timer;
struct sigevent sev;
- struct sigaction act, oldact;
struct itimerspec its;
+ pthread_mutex_lock(&spin->mutex);
+
igt_assert(ns > 0);
if (!spin)
return;
igt_assert(!spin->timer);
- /* SIGRTMAX is used by valgrind, SIGRTMAX - 1 by igt_fork_hang_detector */
- if (spin_signo >= SIGRTMAX - 2)
- spin_signo = SIGRTMIN;
- spin->signo = ++spin_signo;
-
memset(&sev, 0, sizeof(sev));
- sev.sigev_notify = SIGEV_SIGNAL | SIGEV_THREAD_ID;
- sev.sigev_notify_thread_id = gettid();
- sev.sigev_signo = spin->signo;
+ sev.sigev_notify = SIGEV_THREAD;
+ sev.sigev_value.sival_ptr = spin;
+ sev.sigev_notify_function = notify;
igt_assert(timer_create(CLOCK_MONOTONIC, &sev, &timer) == 0);
igt_assert(timer);
- memset(&oldact, 0, sizeof(oldact));
- memset(&act, 0, sizeof(act));
- act.sa_sigaction = sig_handler;
- act.sa_flags = SA_SIGINFO;
- igt_assert(sigaction(spin->signo, &act, &oldact) == 0);
- igt_assert(oldact.sa_sigaction == NULL);
-
memset(&its, 0, sizeof(its));
its.it_value.tv_sec = ns / NSEC_PER_SEC;
its.it_value.tv_nsec = ns % NSEC_PER_SEC;
igt_assert(timer_settime(timer, 0, &its, NULL) == 0);
spin->timer = timer;
+
+ pthread_mutex_unlock(&spin->mutex);
}
/**
@@ -258,11 +235,12 @@ void igt_spin_batch_end(igt_spin_t *spin)
if (!spin)
return;
+ pthread_mutex_lock(&spin->mutex);
+
*spin->batch = MI_BATCH_BUFFER_END;
__sync_synchronize();
- if (spin->signo)
- clear_sig_handler(spin->signo);
+ pthread_mutex_unlock(&spin->mutex);
}
/**
@@ -287,6 +265,7 @@ void igt_spin_batch_free(int fd, igt_spin_t *spin)
gem_munmap(spin->batch, BATCH_SIZE);
gem_close(fd, spin->handle);
+ pthread_mutex_destroy(&spin->mutex);
free(spin);
}
diff --git a/lib/igt_dummyload.h b/lib/igt_dummyload.h
index 2adfadf..8fdc3fe 100644
--- a/lib/igt_dummyload.h
+++ b/lib/igt_dummyload.h
@@ -31,11 +31,12 @@
#include "igt_aux.h"
typedef struct igt_spin {
+ pthread_mutex_t mutex;
unsigned int handle;
timer_t timer;
- int signo;
struct igt_list link;
uint32_t *batch;
+
} igt_spin_t;
igt_spin_t *igt_spin_batch_new(int fd, int engine, unsigned int dep_handle);
--
2.9.3
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH i-g-t] lib/dummyload: Handle timeout in a new thread instead of signal handler
2017-03-27 8:45 [PATCH i-g-t] lib/dummyload: Handle timeout in a new thread instead of signal handler Ander Conselvan de Oliveira
@ 2017-03-27 9:09 ` Chris Wilson
2017-03-27 11:08 ` Ander Conselvan de Oliveira
0 siblings, 1 reply; 5+ messages in thread
From: Chris Wilson @ 2017-03-27 9:09 UTC (permalink / raw)
To: Ander Conselvan de Oliveira; +Cc: intel-gfx
On Mon, Mar 27, 2017 at 11:45:07AM +0300, Ander Conselvan de Oliveira wrote:
> Currently, the main thread needs to wakeup to run the signal handler
> that ends a spin batch. When testing whether a function call succesfully
> waits for a batch to complete, this behavior is undesired. It actually
> invalidates the test.
>
> Fix this by spawning a new thread to handle the timeout.
Very neat!
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
> ---
> lib/igt_dummyload.c | 55 +++++++++++++++++------------------------------------
> lib/igt_dummyload.h | 3 ++-
> 2 files changed, 19 insertions(+), 39 deletions(-)
>
> diff --git a/lib/igt_dummyload.c b/lib/igt_dummyload.c
> index 019c1fb..38b1930 100644
> --- a/lib/igt_dummyload.c
> +++ b/lib/igt_dummyload.c
> @@ -24,6 +24,7 @@
>
> #include "igt.h"
> #include "igt_dummyload.h"
> +#include <pthread.h>
> #include <time.h>
> #include <signal.h>
> #include <sys/syscall.h>
> @@ -166,6 +167,8 @@ igt_spin_batch_new(int fd, int engine, unsigned int dep_handle)
> spin = calloc(1, sizeof(struct igt_spin));
> igt_assert(spin);
>
> + pthread_mutex_init(&spin->mutex, NULL);
> +
> emit_recursive_batch(spin, fd, engine, dep_handle);
> igt_assert(gem_bo_busy(fd, spin->handle));
>
> @@ -174,27 +177,11 @@ igt_spin_batch_new(int fd, int engine, unsigned int dep_handle)
> return spin;
> }
>
> -static void clear_sig_handler(int sig)
> -{
> - struct sigaction act;
> -
> - memset(&act, 0, sizeof(act));
> - act.sa_handler = SIG_DFL;
> - igt_assert(sigaction(sig, &act, NULL) == 0);
> -}
> -
> -static void sig_handler(int sig, siginfo_t *info, void *arg)
> +static void notify(union sigval arg)
> {
> - struct igt_spin *iter;
> + igt_spin_t *spin = arg.sival_ptr;
>
> - igt_list_for_each(iter, &spin_list, link) {
> - if (iter->signo == info->si_signo) {
> - igt_spin_batch_end(iter);
> - return;
> - }
> - }
> -
> - clear_sig_handler(sig);
> + igt_spin_batch_end(spin);
> }
>
> /**
> @@ -208,43 +195,33 @@ static void sig_handler(int sig, siginfo_t *info, void *arg)
> */
> void igt_spin_batch_set_timeout(igt_spin_t *spin, int64_t ns)
> {
> - static int spin_signo = 48; /* Midpoint of SIGRTMIN, SIGRTMAX */
> timer_t timer;
> struct sigevent sev;
> - struct sigaction act, oldact;
> struct itimerspec its;
>
> + pthread_mutex_lock(&spin->mutex);
> +
> igt_assert(ns > 0);
> if (!spin)
> return;
A mutex here is interesting, but only for detecting a race in setting
the timeout twice. Make it assert instead, and we then don't need a
mutex at all.
> igt_assert(!spin->timer);
>
> - /* SIGRTMAX is used by valgrind, SIGRTMAX - 1 by igt_fork_hang_detector */
> - if (spin_signo >= SIGRTMAX - 2)
> - spin_signo = SIGRTMIN;
> - spin->signo = ++spin_signo;
> -
> memset(&sev, 0, sizeof(sev));
> - sev.sigev_notify = SIGEV_SIGNAL | SIGEV_THREAD_ID;
> - sev.sigev_notify_thread_id = gettid();
> - sev.sigev_signo = spin->signo;
> + sev.sigev_notify = SIGEV_THREAD;
> + sev.sigev_value.sival_ptr = spin;
> + sev.sigev_notify_function = notify;
> igt_assert(timer_create(CLOCK_MONOTONIC, &sev, &timer) == 0);
> igt_assert(timer);
>
> - memset(&oldact, 0, sizeof(oldact));
> - memset(&act, 0, sizeof(act));
> - act.sa_sigaction = sig_handler;
> - act.sa_flags = SA_SIGINFO;
> - igt_assert(sigaction(spin->signo, &act, &oldact) == 0);
> - igt_assert(oldact.sa_sigaction == NULL);
> -
> memset(&its, 0, sizeof(its));
> its.it_value.tv_sec = ns / NSEC_PER_SEC;
> its.it_value.tv_nsec = ns % NSEC_PER_SEC;
> igt_assert(timer_settime(timer, 0, &its, NULL) == 0);
>
> spin->timer = timer;
> +
> + pthread_mutex_unlock(&spin->mutex);
> }
>
> /**
> @@ -258,11 +235,12 @@ void igt_spin_batch_end(igt_spin_t *spin)
> if (!spin)
> return;
>
> + pthread_mutex_lock(&spin->mutex);
> +
> *spin->batch = MI_BATCH_BUFFER_END;
> __sync_synchronize();
>
> - if (spin->signo)
> - clear_sig_handler(spin->signo);
> + pthread_mutex_unlock(&spin->mutex);
This doesn't require a mutex.
> }
>
> /**
> @@ -287,6 +265,7 @@ void igt_spin_batch_free(int fd, igt_spin_t *spin)
> gem_munmap(spin->batch, BATCH_SIZE);
>
> gem_close(fd, spin->handle);
> + pthread_mutex_destroy(&spin->mutex);
We need to cancel the timer (before the unmap).
> free(spin);
> }
>
> diff --git a/lib/igt_dummyload.h b/lib/igt_dummyload.h
> index 2adfadf..8fdc3fe 100644
> --- a/lib/igt_dummyload.h
> +++ b/lib/igt_dummyload.h
> @@ -31,11 +31,12 @@
> #include "igt_aux.h"
>
> typedef struct igt_spin {
> + pthread_mutex_t mutex;
> unsigned int handle;
> timer_t timer;
> - int signo;
> struct igt_list link;
> uint32_t *batch;
> +
Oi!
> } igt_spin_t;
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH i-g-t] lib/dummyload: Handle timeout in a new thread instead of signal handler
2017-03-27 9:09 ` Chris Wilson
@ 2017-03-27 11:08 ` Ander Conselvan de Oliveira
2017-03-27 11:23 ` Chris Wilson
0 siblings, 1 reply; 5+ messages in thread
From: Ander Conselvan de Oliveira @ 2017-03-27 11:08 UTC (permalink / raw)
To: intel-gfx; +Cc: Ander Conselvan de Oliveira
Currently, the main thread needs to wakeup to run the signal handler
that ends a spin batch. When testing whether a function call succesfully
waits for a batch to complete, this behavior is undesired. It actually
invalidates the test.
Fix this by spawning a new thread to handle the timeout.
v2: Get rid of mutexes. (Chris)
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
---
lib/igt_dummyload.c | 45 ++++++---------------------------------------
lib/igt_dummyload.h | 1 -
2 files changed, 6 insertions(+), 40 deletions(-)
diff --git a/lib/igt_dummyload.c b/lib/igt_dummyload.c
index 019c1fb..b25e023 100644
--- a/lib/igt_dummyload.c
+++ b/lib/igt_dummyload.c
@@ -174,27 +174,11 @@ igt_spin_batch_new(int fd, int engine, unsigned int dep_handle)
return spin;
}
-static void clear_sig_handler(int sig)
+static void notify(union sigval arg)
{
- struct sigaction act;
+ igt_spin_t *spin = arg.sival_ptr;
- memset(&act, 0, sizeof(act));
- act.sa_handler = SIG_DFL;
- igt_assert(sigaction(sig, &act, NULL) == 0);
-}
-
-static void sig_handler(int sig, siginfo_t *info, void *arg)
-{
- struct igt_spin *iter;
-
- igt_list_for_each(iter, &spin_list, link) {
- if (iter->signo == info->si_signo) {
- igt_spin_batch_end(iter);
- return;
- }
- }
-
- clear_sig_handler(sig);
+ igt_spin_batch_end(spin);
}
/**
@@ -208,10 +192,8 @@ static void sig_handler(int sig, siginfo_t *info, void *arg)
*/
void igt_spin_batch_set_timeout(igt_spin_t *spin, int64_t ns)
{
- static int spin_signo = 48; /* Midpoint of SIGRTMIN, SIGRTMAX */
timer_t timer;
struct sigevent sev;
- struct sigaction act, oldact;
struct itimerspec its;
igt_assert(ns > 0);
@@ -220,25 +202,13 @@ void igt_spin_batch_set_timeout(igt_spin_t *spin, int64_t ns)
igt_assert(!spin->timer);
- /* SIGRTMAX is used by valgrind, SIGRTMAX - 1 by igt_fork_hang_detector */
- if (spin_signo >= SIGRTMAX - 2)
- spin_signo = SIGRTMIN;
- spin->signo = ++spin_signo;
-
memset(&sev, 0, sizeof(sev));
- sev.sigev_notify = SIGEV_SIGNAL | SIGEV_THREAD_ID;
- sev.sigev_notify_thread_id = gettid();
- sev.sigev_signo = spin->signo;
+ sev.sigev_notify = SIGEV_THREAD;
+ sev.sigev_value.sival_ptr = spin;
+ sev.sigev_notify_function = notify;
igt_assert(timer_create(CLOCK_MONOTONIC, &sev, &timer) == 0);
igt_assert(timer);
- memset(&oldact, 0, sizeof(oldact));
- memset(&act, 0, sizeof(act));
- act.sa_sigaction = sig_handler;
- act.sa_flags = SA_SIGINFO;
- igt_assert(sigaction(spin->signo, &act, &oldact) == 0);
- igt_assert(oldact.sa_sigaction == NULL);
-
memset(&its, 0, sizeof(its));
its.it_value.tv_sec = ns / NSEC_PER_SEC;
its.it_value.tv_nsec = ns % NSEC_PER_SEC;
@@ -260,9 +230,6 @@ void igt_spin_batch_end(igt_spin_t *spin)
*spin->batch = MI_BATCH_BUFFER_END;
__sync_synchronize();
-
- if (spin->signo)
- clear_sig_handler(spin->signo);
}
/**
diff --git a/lib/igt_dummyload.h b/lib/igt_dummyload.h
index 2adfadf..41125e3 100644
--- a/lib/igt_dummyload.h
+++ b/lib/igt_dummyload.h
@@ -33,7 +33,6 @@
typedef struct igt_spin {
unsigned int handle;
timer_t timer;
- int signo;
struct igt_list link;
uint32_t *batch;
} igt_spin_t;
--
2.9.3
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH i-g-t] lib/dummyload: Handle timeout in a new thread instead of signal handler
2017-03-27 11:08 ` Ander Conselvan de Oliveira
@ 2017-03-27 11:23 ` Chris Wilson
2017-03-27 11:54 ` Ander Conselvan De Oliveira
0 siblings, 1 reply; 5+ messages in thread
From: Chris Wilson @ 2017-03-27 11:23 UTC (permalink / raw)
To: Ander Conselvan de Oliveira; +Cc: intel-gfx
On Mon, Mar 27, 2017 at 02:08:28PM +0300, Ander Conselvan de Oliveira wrote:
> Currently, the main thread needs to wakeup to run the signal handler
> that ends a spin batch. When testing whether a function call succesfully
> waits for a batch to complete, this behavior is undesired. It actually
> invalidates the test.
>
> Fix this by spawning a new thread to handle the timeout.
>
> v2: Get rid of mutexes. (Chris)
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
Lvgtm,
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
I hoping that this isn't implemented as a signal to the parent thread... :)
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH i-g-t] lib/dummyload: Handle timeout in a new thread instead of signal handler
2017-03-27 11:23 ` Chris Wilson
@ 2017-03-27 11:54 ` Ander Conselvan De Oliveira
0 siblings, 0 replies; 5+ messages in thread
From: Ander Conselvan De Oliveira @ 2017-03-27 11:54 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
On Mon, 2017-03-27 at 12:23 +0100, Chris Wilson wrote:
> On Mon, Mar 27, 2017 at 02:08:28PM +0300, Ander Conselvan de Oliveira wrote:
> > Currently, the main thread needs to wakeup to run the signal handler
> > that ends a spin batch. When testing whether a function call succesfully
> > waits for a batch to complete, this behavior is undesired. It actually
> > invalidates the test.
> >
> > Fix this by spawning a new thread to handle the timeout.
> >
> > v2: Get rid of mutexes. (Chris)
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
>
> Lvgtm,
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Patch pushed to master. Thanks for reviewing.
> I hoping that this isn't implemented as a signal to the parent thread... :)
At least glibc creates a separate thread for this:
https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/unix/sysv/linux/timer_c
reate.c;h=48a703c0651e929f12feb4f194675bcf71694981;hb=HEAD#l100
Ander
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-03-27 11:54 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-03-27 8:45 [PATCH i-g-t] lib/dummyload: Handle timeout in a new thread instead of signal handler Ander Conselvan de Oliveira
2017-03-27 9:09 ` Chris Wilson
2017-03-27 11:08 ` Ander Conselvan de Oliveira
2017-03-27 11:23 ` Chris Wilson
2017-03-27 11:54 ` Ander Conselvan De Oliveira
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.