[PATCH i-g-t] lib/dummyload: Handle timeout in a new thread instead of signal handler

[PATCH i-g-t] lib/dummyload: Handle timeout in a new thread instead of signal handler

[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

Re: [PATCH i-g-t] lib/dummyload: Handle timeout in a new thread instead of signal handler

[Chris Wilson]

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

[PATCH i-g-t] lib/dummyload: Handle timeout in a new thread instead of signal handler

[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

Re: [PATCH i-g-t] lib/dummyload: Handle timeout in a new thread instead of signal handler

[Chris Wilson]

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

Re: [PATCH i-g-t] lib/dummyload: Handle timeout in a new thread instead of signal handler

[Ander Conselvan De Oliveira]

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