[Qemu-devel] [RFC] Ensure SIGALRM causes a cpu_loop_exit

[Qemu-devel] [RFC] Ensure SIGALRM causes a cpu_loop_exit

[andrzej zaborowski]

Hi,
  There is a chance that when using "unix" or "dynticks" clock, the
signal arrives when no cpu is executing. The probability is high when
using dynticks and a timer is scheduled to expire very soon so that a
signal is delivered very soon after a previous signal. When that
happens cpu_single_env is zero and the signal handler does nothing.
This is not much problem with "unix" clocks or when not using
-nographic or when the guest OS uses interrupts, because a another
cpu_loop_exit will happen in not too long. If none of these conditions
is true the cpu loop starts spinning without a chance to exit and
process events. I used the following patch to prevent this but there's
probably a better way:

diff --git a/cpu-all.h b/cpu-all.h
index f4db592..c095e9c 100644
--- a/cpu-all.h
+++ b/cpu-all.h
@@ -706,6 +706,7 @@ void cpu_abort(CPUState *env, const char *fmt, ...)
     __attribute__ ((__noreturn__));
 extern CPUState *first_cpu;
 extern CPUState *cpu_single_env;
+extern int env_pending_request;
 extern int code_copy_enabled;

 #define CPU_INTERRUPT_EXIT   0x01 /* wants exit from main loop */
diff --git a/cpu-exec.c b/cpu-exec.c
index 1c7356a..af75731 100644
--- a/cpu-exec.c
+++ b/cpu-exec.c
@@ -258,6 +258,11 @@ int cpu_exec(CPUState *env1)

     cpu_single_env = env1;

+    if (env_pending_request) {
+        cpu_interrupt(env1, env_pending_request);
+        env_pending_request = 0;
+    }
+
     /* first we save global registers */
 #define SAVE_HOST_REGS 1
 #include "hostregs_helper.h"
diff --git a/exec.c b/exec.c
index 6384df2..a649d8f 100644
--- a/exec.c
+++ b/exec.c
@@ -96,6 +96,7 @@ CPUState *first_cpu;
 /* current CPU in the current thread. It is only valid inside
    cpu_exec() */
 CPUState *cpu_single_env;
+int env_pending_request;

 typedef struct PageDesc {
     /* list of TBs intersecting this ram page */
@@ -1194,6 +1195,12 @@ void cpu_interrupt(CPUState *env, int mask)
     TranslationBlock *tb;
     static int interrupt_lock;

+    /* cause an interrupt in the first cpu that tries to start running */
+    if (!env) {
+        env_pending_request |= mask;
+        return;
+    }
+
     env->interrupt_request |= mask;
     /* if the cpu is currently executing code, we must unlink it and
        all the potentially executing TB */
diff --git a/vl.c b/vl.c
index 864a044..ec2aa84 100644
--- a/vl.c
+++ b/vl.c
@@ -1184,15 +1184,14 @@ static void host_alarm_handler(int host_signum)
         SetEvent(data->host_alarm);
 #endif
         CPUState *env = cpu_single_env;
-        if (env) {
-            /* stop the currently executing cpu because a timer occured */
-            cpu_interrupt(env, CPU_INTERRUPT_EXIT);
+
+        /* stop the currently executing cpu because a timer occured */
+        cpu_interrupt(env, CPU_INTERRUPT_EXIT);
 #ifdef USE_KQEMU
-            if (env->kqemu_enabled) {
-                kqemu_cpu_interrupt(env);
-            }
-#endif
+        if (env && env->kqemu_enabled) {
+            kqemu_cpu_interrupt(env);
         }
+#endif
     }
 }

Regards

Re: [Qemu-devel] [RFC] Ensure SIGALRM causes a cpu_loop_exit

[Paul Brook]

>   There is a chance that when using "unix" or "dynticks" clock, the
> signal arrives when no cpu is executing.

I've seen similar stalls, but not managed to track down the source. Your 
analysis seems correct.

> +    /* cause an interrupt in the first cpu that tries to start running */
> +    if (!env) {
> +        env_pending_request | mask

IIUC We should assert that mask == CPU_INTERRUPT_EXIT. If we try to raise an 
actual interrupt without an active CPU then something else is wrong. In fact 
this probably means env_pending_request can be a simple boolean (indicating 
we want to break out of cpu_exec), rather than munging it into 
env->interrupt_request.

it took me a while to figure out exactly which race condition we're avoiding 
here. How adding a comment like:

/* There is a window for signals to arrive between main_loop checking for 
events and setting cpu_single_env here.  Check if this occurred and we need 
to exit back to the IO loop. */

> +    if (env_pending_request) {
> +        cpu_interrupt(env1, env_pending_request);
> +        env_pending_request = 0;
> +    }
> +

Re: [Qemu-devel] [RFC] Ensure SIGALRM causes a cpu_loop_exit

[andrzej zaborowski]

On 24/11/2007, Paul Brook <paul@codesourcery.com> wrote:
> > There is a chance that when using "unix" or "dynticks" clock, the
> > signal arrives when no cpu is executing.

How about this version, this one touches vl.c only:

--- a/vl.c
+++ b/vl.c
@@ -236,6 +236,10 @@ const char *prom_envs[MAX_PROM_ENVS];
 struct bt_piconet_s *local_piconet;
 struct modem_ops_s modem_ops;

+static CPUState *cur_cpu;
+static CPUState *next_cpu;
+static int event_pending;
+
 #define TFR(expr) do { if ((expr) != -1) break; } while (errno == EINTR)

 /***********************************************************/
@@ -1183,16 +1187,16 @@ static void host_alarm_handler(int host_signum)
         struct qemu_alarm_win32 *data = ((struct
qemu_alarm_timer*)dwUser)->priv;
         SetEvent(data->host_alarm);
 #endif
-        CPUState *env = cpu_single_env;
-        if (env) {
-            /* stop the currently executing cpu because a timer occured */
-            cpu_interrupt(env, CPU_INTERRUPT_EXIT);
+        CPUState *env = next_cpu;
+
+        /* stop the currently executing cpu because a timer occured */
+        cpu_interrupt(env, CPU_INTERRUPT_EXIT);
 #ifdef USE_KQEMU
-            if (env->kqemu_enabled) {
-                kqemu_cpu_interrupt(env);
-            }
-#endif
+        if (env->kqemu_enabled) {
+            kqemu_cpu_interrupt(env);
         }
+#endif
+        event_pending = 1;
     }
 }

@@ -7168,8 +7172,6 @@ void main_loop_wait(int timeout)

 }

-static CPUState *cur_cpu;
-
 static int main_loop(void)
 {
     int ret, timeout;
@@ -7179,15 +7181,13 @@ static int main_loop(void)
     CPUState *env;

     cur_cpu = first_cpu;
+    next_cpu = cur_cpu->next_cpu ?: first_cpu;
     for(;;) {
         if (vm_running) {

-            env = cur_cpu;
             for(;;) {
                 /* get next cpu */
-                env = env->next_cpu;
-                if (!env)
-                    env = first_cpu;
+                env = next_cpu;
 #ifdef CONFIG_PROFILER
                 ti = profile_getclock();
 #endif
@@ -7195,6 +7195,12 @@ static int main_loop(void)
 #ifdef CONFIG_PROFILER
                 qemu_time += profile_getclock() - ti;
 #endif
+                next_cpu = env->next_cpu ?: first_cpu;
+                if (event_pending) {
+                    ret = EXCP_INTERRUPT;
+                    event_pending = 0;
+                    break;
+                }
                 if (ret == EXCP_HLT) {
                     /* Give the next CPU a chance to run.  */
                     cur_cpu = env;

Re: [Qemu-devel] [RFC] Ensure SIGALRM causes a cpu_loop_exit

[Thiemo Seufer]

andrzej zaborowski wrote:
> On 24/11/2007, Paul Brook <paul@codesourcery.com> wrote:
> > > There is a chance that when using "unix" or "dynticks" clock, the
> > > signal arrives when no cpu is executing.
> 
> How about this version, this one touches vl.c only:

Any reason why this isn't in CVS?


Thiemo

> --- a/vl.c
> +++ b/vl.c
> @@ -236,6 +236,10 @@ const char *prom_envs[MAX_PROM_ENVS];
>  struct bt_piconet_s *local_piconet;
>  struct modem_ops_s modem_ops;
> 
> +static CPUState *cur_cpu;
> +static CPUState *next_cpu;
> +static int event_pending;
> +
>  #define TFR(expr) do { if ((expr) != -1) break; } while (errno == EINTR)
> 
>  /***********************************************************/
> @@ -1183,16 +1187,16 @@ static void host_alarm_handler(int host_signum)
>          struct qemu_alarm_win32 *data = ((struct
> qemu_alarm_timer*)dwUser)->priv;
>          SetEvent(data->host_alarm);
>  #endif
> -        CPUState *env = cpu_single_env;
> -        if (env) {
> -            /* stop the currently executing cpu because a timer occured */
> -            cpu_interrupt(env, CPU_INTERRUPT_EXIT);
> +        CPUState *env = next_cpu;
> +
> +        /* stop the currently executing cpu because a timer occured */
> +        cpu_interrupt(env, CPU_INTERRUPT_EXIT);
>  #ifdef USE_KQEMU
> -            if (env->kqemu_enabled) {
> -                kqemu_cpu_interrupt(env);
> -            }
> -#endif
> +        if (env->kqemu_enabled) {
> +            kqemu_cpu_interrupt(env);
>          }
> +#endif
> +        event_pending = 1;
>      }
>  }
> 
> @@ -7168,8 +7172,6 @@ void main_loop_wait(int timeout)
> 
>  }
> 
> -static CPUState *cur_cpu;
> -
>  static int main_loop(void)
>  {
>      int ret, timeout;
> @@ -7179,15 +7181,13 @@ static int main_loop(void)
>      CPUState *env;
> 
>      cur_cpu = first_cpu;
> +    next_cpu = cur_cpu->next_cpu ?: first_cpu;
>      for(;;) {
>          if (vm_running) {
> 
> -            env = cur_cpu;
>              for(;;) {
>                  /* get next cpu */
> -                env = env->next_cpu;
> -                if (!env)
> -                    env = first_cpu;
> +                env = next_cpu;
>  #ifdef CONFIG_PROFILER
>                  ti = profile_getclock();
>  #endif
> @@ -7195,6 +7195,12 @@ static int main_loop(void)
>  #ifdef CONFIG_PROFILER
>                  qemu_time += profile_getclock() - ti;
>  #endif
> +                next_cpu = env->next_cpu ?: first_cpu;
> +                if (event_pending) {
> +                    ret = EXCP_INTERRUPT;
> +                    event_pending = 0;
> +                    break;
> +                }
>                  if (ret == EXCP_HLT) {
>                      /* Give the next CPU a chance to run.  */
>                      cur_cpu = env;
> 
> 
> 

Re: [Qemu-devel] [RFC] Ensure SIGALRM causes a cpu_loop_exit

[andrzej zaborowski]

On 02/12/2007, Thiemo Seufer <ths@networkno.de> wrote:
> andrzej zaborowski wrote:
> > On 24/11/2007, Paul Brook <paul@codesourcery.com> wrote:
> > > > There is a chance that when using "unix" or "dynticks" clock, the
> > > > signal arrives when no cpu is executing.
> >
> > How about this version, this one touches vl.c only:
>
> Any reason why this isn't in CVS?

I wanted to make sure there were no objections :)
Now committed this in the form it was in the last message.
Regards