Skip to contentSkip to conversation

Register Linux dyntick timer as per-thread signal

RFC PATCH Superseded Jan Kiszka 5 participants 8 messages Jun 16, 2011 – Jul 23, 2011 KVM

lore
Share & tools
lore

Jan Kiszka

author
#0 lore raw

RFC PATCH 1/1 Register Linux dyntick timer as per-thread signal

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 <[email protected]>
---
 compatfd.c   |   11 +++++++++++
 compatfd.h   |    1 +
 qemu-timer.c |   12 +++++++++++-
 3 files changed, 23 insertions(+), 1 deletions(-)
compatfd.c +11 -0
index 41586ce..31654c6 100644
@@ -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
+}
compatfd.h +1 -0
index fc37915..6b04877 100644
@@ -39,5 +39,6 @@ struct qemu_signalfd_siginfo {
 };
 
 int qemu_signalfd(const sigset_t *mask);
+bool qemu_signalfd_available(void);
 
 #endif
qemu-timer.c +11 -1
index 72066c7..09e6f17 100644
@@ -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");
Signature · 1 line
1.7.1
#1 lore raw
Quoted 8 lines · 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.

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

#2 lore raw
In reply to Jan Kiszka

qemu-timer.c static int dynticks_start_timer(struct qemu_alarm_timer *t) · line 835

Quoted 13 lines · Jan Kiszka
     ev.sigev_value.sival_int = 0;
qemu-timer.c +9 -1
-    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~

#3 lore raw
In reply to Jan Kiszka

Hi Jan,

Quoted 3 lines · 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.

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

author
#4 lore raw
Quoted 10 lines · Alexandre Raymond

Hi Jan,

Quoted 3 lines · 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.

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

Signature · 2 lines
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux
author
#5 lore raw

PATCH v2 1/1 Register Linux dyntick timer as per-thread signal

In reply to 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 <[email protected]>

---

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(-)
compatfd.c +11 -0
index 41586ce..31654c6 100644
@@ -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
+}
compatfd.h +1 -0
index fc37915..6b04877 100644
@@ -39,5 +39,6 @@ struct qemu_signalfd_siginfo {
 };
 
 int qemu_signalfd(const sigset_t *mask);
+bool qemu_signalfd_available(void);
 
 #endif
qemu-timer.c +8 -0
index 72066c7..743cf96 100644
@@ -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)) {
Signature · 1 line
1.7.1
Reviewed-by on v2 1/1
#6 lore raw
Quoted 7 lines · 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.

Quoted 6 lines

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 <[email protected]>
Reviewed-by Richard Henderson <[email protected]>

r~

Applied
#7 lore raw
In reply to Jan Kiszka
Quoted 7 lines · 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.

Quoted 6 lines

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<[email protected]>

Applied. Thanks.

Regards,

Anthony Liguori

Quoted 68 lines

---

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(-)
compatfd.c +11 -0
index 41586ce..31654c6 100644
@@ -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
+}
compatfd.h +1 -0
index fc37915..6b04877 100644
@@ -39,5 +39,6 @@ struct qemu_signalfd_siginfo {
  };
 
  int qemu_signalfd(const sigset_t *mask);
+bool qemu_signalfd_available(void);
 
  #endif
qemu-timer.c +8 -0
index 72066c7..743cf96 100644
@@ -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)) {