[01/22] Prevent abortion on multiple VCPU kicks

Submitted by Jan Kiszka on Jan. 27, 2011, 1:09 p.m.

Details

Message ID a7a2a912167d2ef699844c8c0e7b170b4d6f3fb1.1296133797.git.jan.kiszka@siemens.com
State New
Headers show

Commit Message

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(-)

Comments

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 hide | download patch | download mbox

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)