[Qemu-devel] [RFC][PATCH] Register Linux dyntick timer as per-thread signal

[Qemu-devel] [RFC][PATCH] Register Linux dyntick timer as per-thread signal

[Jan Kiszka]

Ingo Molnar pointed out that sending the timer signal to the whole
process, just blocking it everywhere, is suboptimal with an increasing
number of threads. QEMU is using this pattern so far.

But Linux provides a (non-portable) way to restrict the signal to a
single thread: Use SIGEV_THREAD_ID unless we are forced to emulate
signalfd via an additional thread. That case could theoretically be
optimized as well, but it doesn't look worth bothering.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 compatfd.c   |   11 +++++++++++
 compatfd.h   |    1 +
 qemu-timer.c |   12 +++++++++++-
 3 files changed, 23 insertions(+), 1 deletions(-)

diff --git a/compatfd.c b/compatfd.c
index 41586ce..31654c6 100644
--- a/compatfd.c
+++ b/compatfd.c
@@ -115,3 +115,14 @@ int qemu_signalfd(const sigset_t *mask)
 
     return qemu_signalfd_compat(mask);
 }
+
+bool qemu_signalfd_available(void)
+{
+#ifdef CONFIG_SIGNALFD
+    errno = 0;
+    syscall(SYS_signalfd, -1, NULL, _NSIG / 8);
+    return errno != ENOSYS;
+#else
+    return false;
+#endif
+}
diff --git a/compatfd.h b/compatfd.h
index fc37915..6b04877 100644
--- a/compatfd.h
+++ b/compatfd.h
@@ -39,5 +39,6 @@ struct qemu_signalfd_siginfo {
 };
 
 int qemu_signalfd(const sigset_t *mask);
+bool qemu_signalfd_available(void);
 
 #endif
diff --git a/qemu-timer.c b/qemu-timer.c
index 72066c7..09e6f17 100644
--- a/qemu-timer.c
+++ b/qemu-timer.c
@@ -803,6 +803,8 @@ static int64_t qemu_next_alarm_deadline(void)
 
 #if defined(__linux__)
 
+#include "compatfd.h"
+
 static int dynticks_start_timer(struct qemu_alarm_timer *t)
 {
     struct sigevent ev;
@@ -821,8 +823,16 @@ static int dynticks_start_timer(struct qemu_alarm_timer *t)
      */
     memset(&ev, 0, sizeof(ev));
     ev.sigev_value.sival_int = 0;
-    ev.sigev_notify = SIGEV_SIGNAL;
     ev.sigev_signo = SIGALRM;
+#ifdef SIGEV_THREAD_ID
+    if (qemu_signalfd_available()) {
+        ev.sigev_notify = SIGEV_THREAD_ID;
+        ev._sigev_un._tid = qemu_get_thread_id();
+    } else
+#endif /* SIGEV_THREAD_ID */
+    {
+        ev.sigev_notify = SIGEV_SIGNAL;
+    }
 
     if (timer_create(CLOCK_REALTIME, &ev, &host_timer)) {
         perror("timer_create");
-- 
1.7.1

Re: [Qemu-devel] [RFC][PATCH] Register Linux dyntick timer as per-thread signal

[Ingo Molnar]

* Jan Kiszka <jan.kiszka@siemens.com> wrote:

> Ingo Molnar pointed out that sending the timer signal to the whole
> process, just blocking it everywhere, is suboptimal with an increasing
> number of threads. QEMU is using this pattern so far.
> 
> But Linux provides a (non-portable) way to restrict the signal to a
> single thread: Use SIGEV_THREAD_ID unless we are forced to emulate
> signalfd via an additional thread. That case could theoretically be
> optimized as well, but it doesn't look worth bothering.

Would be nice to mention it in the changelog that the context and 
motivation for my remark was a patch sent for tools/kvm/ by Asias He:

  kvm tools: Block SIGALRM for vcpu thread using sig_block() helper

Thanks,

	Ingo

Re: [Qemu-devel] [RFC][PATCH] Register Linux dyntick timer as per-thread signal

[Richard Henderson]

On 06/16/2011 02:31 AM, Jan Kiszka wrote:
>      ev.sigev_value.sival_int = 0;
> -    ev.sigev_notify = SIGEV_SIGNAL;
>      ev.sigev_signo = SIGALRM;
> +#ifdef SIGEV_THREAD_ID
> +    if (qemu_signalfd_available()) {
> +        ev.sigev_notify = SIGEV_THREAD_ID;
> +        ev._sigev_un._tid = qemu_get_thread_id();
> +    } else
> +#endif /* SIGEV_THREAD_ID */
> +    {
> +        ev.sigev_notify = SIGEV_SIGNAL;
> +    }
>  

Rather than do the else-inside-ifdef thing, why not
leave the original setting of sigev_notify where it
was, and let the ifdef overwrite it?


r~

Re: [Qemu-devel] [RFC][PATCH] Register Linux dyntick timer as per-thread signal

[Alexandre Raymond]

Hi Jan,

On Thu, Jun 16, 2011 at 5:31 AM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> Ingo Molnar pointed out that sending the timer signal to the whole
> process, just blocking it everywhere, is suboptimal with an increasing
> number of threads. QEMU is using this pattern so far.

I am not familiar with this code, but don't you already need to block
SIGALRM properly in all threads for OSes != Linux ? If so, isn't this
patch redundant?

Alexandre

Re: [Qemu-devel] [RFC][PATCH] Register Linux dyntick timer as per-thread signal

[Jan Kiszka]

On 2011-06-16 17:24, Alexandre Raymond wrote:
> Hi Jan,
> 
> On Thu, Jun 16, 2011 at 5:31 AM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>> Ingo Molnar pointed out that sending the timer signal to the whole
>> process, just blocking it everywhere, is suboptimal with an increasing
>> number of threads. QEMU is using this pattern so far.
> 
> I am not familiar with this code, but don't you already need to block
> SIGALRM properly in all threads for OSes != Linux ? If so, isn't this
> patch redundant?

Yes, we still need to block for the sake of non-Linux UNIX or
pre-signalfd Linux. That blocking becomes in fact redundant in the
signalfd case, but that's both harmless and not worth optimizing. The
key is that per-thread signals do not care about other threads having
them blocked or not, they only deal with the target thread.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

[Qemu-devel] [PATCH v2] Register Linux dyntick timer as per-thread signal

[Jan Kiszka]

Derived from kvm-tool patch
http://thread.gmane.org/gmane.comp.emulators.kvm.devel/74309

Ingo Molnar pointed out that sending the timer signal to the whole
process, just blocking it everywhere, is suboptimal with an increasing
number of threads. QEMU is also using this pattern so far.

Linux provides a (non-portable) way to restrict the signal to a single
thread: We can use SIGEV_THREAD_ID unless we are forced to emulate
signalfd via an additional thread. That case could theoretically be
optimized as well, but it doesn't look worth bothering.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---

Changes in v2:
 - refactored dynticks_start_timer changes as suggested by Richard
   Henderson
 - added reference to original kvm-tool patch

 compatfd.c   |   11 +++++++++++
 compatfd.h   |    1 +
 qemu-timer.c |    8 ++++++++
 3 files changed, 20 insertions(+), 0 deletions(-)

diff --git a/compatfd.c b/compatfd.c
index 41586ce..31654c6 100644
--- a/compatfd.c
+++ b/compatfd.c
@@ -115,3 +115,14 @@ int qemu_signalfd(const sigset_t *mask)
 
     return qemu_signalfd_compat(mask);
 }
+
+bool qemu_signalfd_available(void)
+{
+#ifdef CONFIG_SIGNALFD
+    errno = 0;
+    syscall(SYS_signalfd, -1, NULL, _NSIG / 8);
+    return errno != ENOSYS;
+#else
+    return false;
+#endif
+}
diff --git a/compatfd.h b/compatfd.h
index fc37915..6b04877 100644
--- a/compatfd.h
+++ b/compatfd.h
@@ -39,5 +39,6 @@ struct qemu_signalfd_siginfo {
 };
 
 int qemu_signalfd(const sigset_t *mask);
+bool qemu_signalfd_available(void);
 
 #endif
diff --git a/qemu-timer.c b/qemu-timer.c
index 72066c7..743cf96 100644
--- a/qemu-timer.c
+++ b/qemu-timer.c
@@ -803,6 +803,8 @@ static int64_t qemu_next_alarm_deadline(void)
 
 #if defined(__linux__)
 
+#include "compatfd.h"
+
 static int dynticks_start_timer(struct qemu_alarm_timer *t)
 {
     struct sigevent ev;
@@ -822,6 +824,12 @@ static int dynticks_start_timer(struct qemu_alarm_timer *t)
     memset(&ev, 0, sizeof(ev));
     ev.sigev_value.sival_int = 0;
     ev.sigev_notify = SIGEV_SIGNAL;
+#ifdef SIGEV_THREAD_ID
+    if (qemu_signalfd_available()) {
+        ev.sigev_notify = SIGEV_THREAD_ID;
+        ev._sigev_un._tid = qemu_get_thread_id();
+    }
+#endif /* SIGEV_THREAD_ID */
     ev.sigev_signo = SIGALRM;
 
     if (timer_create(CLOCK_REALTIME, &ev, &host_timer)) {
-- 
1.7.1

Re: [Qemu-devel] [PATCH v2] Register Linux dyntick timer as per-thread signal

[Richard Henderson]

On 06/17/2011 02:25 AM, Jan Kiszka wrote:
> Derived from kvm-tool patch
> http://thread.gmane.org/gmane.comp.emulators.kvm.devel/74309
> 
> Ingo Molnar pointed out that sending the timer signal to the whole
> process, just blocking it everywhere, is suboptimal with an increasing
> number of threads. QEMU is also using this pattern so far.
> 
> Linux provides a (non-portable) way to restrict the signal to a single
> thread: We can use SIGEV_THREAD_ID unless we are forced to emulate
> signalfd via an additional thread. That case could theoretically be
> optimized as well, but it doesn't look worth bothering.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>

Reviewed-by: Richard Henderson <rth@twiddle.net>


r~

Re: [Qemu-devel] [PATCH v2] Register Linux dyntick timer as per-thread signal

[Anthony Liguori]

On 06/17/2011 04:25 AM, Jan Kiszka wrote:
> Derived from kvm-tool patch
> http://thread.gmane.org/gmane.comp.emulators.kvm.devel/74309
>
> Ingo Molnar pointed out that sending the timer signal to the whole
> process, just blocking it everywhere, is suboptimal with an increasing
> number of threads. QEMU is also using this pattern so far.
>
> Linux provides a (non-portable) way to restrict the signal to a single
> thread: We can use SIGEV_THREAD_ID unless we are forced to emulate
> signalfd via an additional thread. That case could theoretically be
> optimized as well, but it doesn't look worth bothering.
>
> Signed-off-by: Jan Kiszka<jan.kiszka@siemens.com>

Applied.  Thanks.

Regards,

Anthony Liguori

> ---
>
> Changes in v2:
>   - refactored dynticks_start_timer changes as suggested by Richard
>     Henderson
>   - added reference to original kvm-tool patch
>
>   compatfd.c   |   11 +++++++++++
>   compatfd.h   |    1 +
>   qemu-timer.c |    8 ++++++++
>   3 files changed, 20 insertions(+), 0 deletions(-)
>
> diff --git a/compatfd.c b/compatfd.c
> index 41586ce..31654c6 100644
> --- a/compatfd.c
> +++ b/compatfd.c
> @@ -115,3 +115,14 @@ int qemu_signalfd(const sigset_t *mask)
>
>       return qemu_signalfd_compat(mask);
>   }
> +
> +bool qemu_signalfd_available(void)
> +{
> +#ifdef CONFIG_SIGNALFD
> +    errno = 0;
> +    syscall(SYS_signalfd, -1, NULL, _NSIG / 8);
> +    return errno != ENOSYS;
> +#else
> +    return false;
> +#endif
> +}
> diff --git a/compatfd.h b/compatfd.h
> index fc37915..6b04877 100644
> --- a/compatfd.h
> +++ b/compatfd.h
> @@ -39,5 +39,6 @@ struct qemu_signalfd_siginfo {
>   };
>
>   int qemu_signalfd(const sigset_t *mask);
> +bool qemu_signalfd_available(void);
>
>   #endif
> diff --git a/qemu-timer.c b/qemu-timer.c
> index 72066c7..743cf96 100644
> --- a/qemu-timer.c
> +++ b/qemu-timer.c
> @@ -803,6 +803,8 @@ static int64_t qemu_next_alarm_deadline(void)
>
>   #if defined(__linux__)
>
> +#include "compatfd.h"
> +
>   static int dynticks_start_timer(struct qemu_alarm_timer *t)
>   {
>       struct sigevent ev;
> @@ -822,6 +824,12 @@ static int dynticks_start_timer(struct qemu_alarm_timer *t)
>       memset(&ev, 0, sizeof(ev));
>       ev.sigev_value.sival_int = 0;
>       ev.sigev_notify = SIGEV_SIGNAL;
> +#ifdef SIGEV_THREAD_ID
> +    if (qemu_signalfd_available()) {
> +        ev.sigev_notify = SIGEV_THREAD_ID;
> +        ev._sigev_un._tid = qemu_get_thread_id();
> +    }
> +#endif /* SIGEV_THREAD_ID */
>       ev.sigev_signo = SIGALRM;
>
>       if (timer_create(CLOCK_REALTIME,&ev,&host_timer)) {