Patchwork [01/22] Prevent abortion on multiple VCPU kicks

login
register
mail settings
Submitter Jan Kiszka
Date Jan. 27, 2011, 1:09 p.m.
Message ID <a7a2a912167d2ef699844c8c0e7b170b4d6f3fb1.1296133797.git.jan.kiszka@siemens.com>
Download mbox | patch
Permalink /patch/80692/
State New
Headers show

Comments

Jan Kiszka - Jan. 27, 2011, 1:09 p.m.
If we call qemu_cpu_kick more than once before the target was able to
process the signal, pthread_kill will fail, and qemu will abort. Prevent
this by avoiding the redundant signal.

This logic can be found in qemu-kvm as well.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 cpu-defs.h |    1 +
 cpus.c     |    6 +++++-
 2 files changed, 6 insertions(+), 1 deletions(-)
Avi Kivity - Jan. 31, 2011, 9:44 a.m.
On 01/27/2011 03:09 PM, Jan Kiszka wrote:
> If we call qemu_cpu_kick more than once before the target was able to
> process the signal, pthread_kill will fail, and qemu will abort. Prevent
> this by avoiding the redundant signal.
>

Doesn't fit with the manual page (or with the idea that signals are 
asynchronous):

NAME
        pthread_kill - send a signal to a thread


...

ERRORS
        ESRCH  No thread with the ID thread could be found.

        EINVAL An invalid signal was specified.
Jan Kiszka - Jan. 31, 2011, 11:19 a.m.
On 2011-01-31 10:44, Avi Kivity wrote:
> On 01/27/2011 03:09 PM, Jan Kiszka wrote:
>> If we call qemu_cpu_kick more than once before the target was able to
>> process the signal, pthread_kill will fail, and qemu will abort. Prevent
>> this by avoiding the redundant signal.
>>
> 
> Doesn't fit with the manual page (or with the idea that signals are 
> asynchronous):
> 
> NAME
>         pthread_kill - send a signal to a thread
> 
> 
> ...
> 
> ERRORS
>         ESRCH  No thread with the ID thread could be found.
> 
>         EINVAL An invalid signal was specified.
> 

Valid remark, but I was receiving EAGAIN for blocked RT signals. Don't
know if this is Linux-specific. A quick glance at the man pages did not
reveal if this is allowed or at least gray area.

However, even when selectively ignoring this, it's more efficient to
catch the redundant signaling in user space.

Jan
Avi Kivity - Jan. 31, 2011, 1:16 p.m.
On 01/31/2011 01:19 PM, Jan Kiszka wrote:
> On 2011-01-31 10:44, Avi Kivity wrote:
> >  On 01/27/2011 03:09 PM, Jan Kiszka wrote:
> >>  If we call qemu_cpu_kick more than once before the target was able to
> >>  process the signal, pthread_kill will fail, and qemu will abort. Prevent
> >>  this by avoiding the redundant signal.
> >>
> >
> >  Doesn't fit with the manual page (or with the idea that signals are
> >  asynchronous):
> >
> >  NAME
> >          pthread_kill - send a signal to a thread
> >
> >
> >  ...
> >
> >  ERRORS
> >          ESRCH  No thread with the ID thread could be found.
> >
> >          EINVAL An invalid signal was specified.
> >
>
> Valid remark, but I was receiving EAGAIN for blocked RT signals. Don't
> know if this is Linux-specific. A quick glance at the man pages did not
> reveal if this is allowed or at least gray area.
>

     } else if (!is_si_special(info)) {
         if (sig >= SIGRTMIN && info->si_code != SI_USER) {
             /*
              * Queue overflow, abort.  We may abort if the
              * signal was rt and sent by user using something
              * other than kill().
              */
             trace_signal_overflow_fail(sig, group, info);
             return -EAGAIN;
         }

> However, even when selectively ignoring this, it's more efficient to
> catch the redundant signaling in user space.

Yes.

Patch

diff --git a/cpu-defs.h b/cpu-defs.h
index 8d4bf86..db809ed 100644
--- a/cpu-defs.h
+++ b/cpu-defs.h
@@ -205,6 +205,7 @@  typedef struct CPUWatchpoint {
     uint32_t stopped; /* Artificially stopped */                        \
     struct QemuThread *thread;                                          \
     struct QemuCond *halt_cond;                                         \
+    int thread_kicked;                                                  \
     struct qemu_work_item *queued_work_first, *queued_work_last;        \
     const char *cpu_model_str;                                          \
     struct KVMState *kvm_state;                                         \
diff --git a/cpus.c b/cpus.c
index 4c9928e..ab6e40e 100644
--- a/cpus.c
+++ b/cpus.c
@@ -481,6 +481,7 @@  static void qemu_wait_io_event_common(CPUState *env)
         qemu_cond_signal(&qemu_pause_cond);
     }
     flush_queued_work(env);
+    env->thread_kicked = false;
 }
 
 static void qemu_tcg_wait_io_event(void)
@@ -648,7 +649,10 @@  void qemu_cpu_kick(void *_env)
 {
     CPUState *env = _env;
     qemu_cond_broadcast(env->halt_cond);
-    qemu_thread_signal(env->thread, SIG_IPI);
+    if (!env->thread_kicked) {
+        qemu_thread_signal(env->thread, SIG_IPI);
+        env->thread_kicked = true;
+    }
 }
 
 int qemu_cpu_self(void *_env)