Re: [Qemu-devel] [RFC PATCH V6 08/18] cpu: remove exit_request global.

[Frederic Konrad]

On 07/07/2015 15:04, Alex Bennée wrote:
> address@hidden writes:
>
> > From: KONRAD Frederic <address@hidden>
> >
> > This removes exit_request global and adds a variable in CPUState for this.
> > Only the flag for the first cpu is used for the moment as we are still with one
> > TCG thread.
> >
> > Signed-off-by: KONRAD Frederic <address@hidden>
> > ---
> >   cpu-exec.c | 15 ---------------
> >   cpus.c     | 17 ++++++++++++++---
> >   2 files changed, 14 insertions(+), 18 deletions(-)
> >
> > diff --git a/cpu-exec.c b/cpu-exec.c
> > index 5d9b518..0644383 100644
> > --- a/cpu-exec.c
> > +++ b/cpu-exec.c
> > @@ -364,8 +364,6 @@ static void cpu_handle_debug_exception(CPUArchState *env)
> >   
> >   /* main execution loop */
> >   
> > -volatile sig_atomic_t exit_request;
> > -
> >   int cpu_exec(CPUArchState *env)
> >   {
> >       CPUState *cpu = ENV_GET_CPU(env);
> > @@ -394,20 +392,8 @@ int cpu_exec(CPUArchState *env)
> >   
> >       current_cpu = cpu;
> >   
> > -    /* As long as current_cpu is null, up to the assignment just above,
> > -     * requests by other threads to exit the execution loop are expected to
> > -     * be issued using the exit_request global. We must make sure that our
> > -     * evaluation of the global value is performed past the current_cpu
> > -     * value transition point, which requires a memory barrier as well as
> > -     * an instruction scheduling constraint on modern architectures.  */
> > -    smp_mb();
> > -
> >       rcu_read_lock();
> >   
> > -    if (unlikely(exit_request)) {
> > -        cpu->exit_request = 1;
> > -    }
> > -
> >       cc->cpu_exec_enter(cpu);
> >   
> >       /* Calculate difference between guest clock and host clock.
> > @@ -496,7 +482,6 @@ int cpu_exec(CPUArchState *env)
> >                       }
> >                   }
> >                   if (unlikely(cpu->exit_request)) {
> > -                    cpu->exit_request = 0;
> >                       cpu->exception_index = EXCP_INTERRUPT;
> >                       cpu_loop_exit(cpu);
> >                   }
> > diff --git a/cpus.c b/cpus.c
> > index 23c316c..2541c56 100644
> > --- a/cpus.c
> > +++ b/cpus.c
> > @@ -137,6 +137,8 @@ typedef struct TimersState {
> >   } TimersState;
> >   
> >   static TimersState timers_state;
> > +/* CPU associated to this thread. */
> > +static __thread CPUState *tcg_thread_cpu;
> >   
> >   int64_t cpu_get_icount_raw(void)
> >   {
> > @@ -661,12 +663,18 @@ static void cpu_handle_guest_debug(CPUState *cpu)
> >       cpu->stopped = true;
> >   }
> >   
> > +/**
> > + * cpu_signal
> > + * Signal handler when using TCG.
> > + */
> >   static void cpu_signal(int sig)
> >   {
> >       if (current_cpu) {
> >           cpu_exit(current_cpu);
> >       }
> > -    exit_request = 1;
> > +
> > +    /* FIXME: We might want to check if the cpu is running? */
> > +    tcg_thread_cpu->exit_request = true;
> I guess the potential problem is race conditions here? What happens if
> the cpu is signalled by two different threads for two different reasons?

Hmmm yes, I need to take a look at that and check all the reason why it 
should exit.

But maybe it's OK, the first time the cpu get the signal it will set 
exit_request..
and the second times as well?

> >   }
> >   
> >   #ifdef CONFIG_LINUX
> > @@ -1031,6 +1039,7 @@ static void *qemu_tcg_cpu_thread_fn(void *arg)
> >   {
> >       CPUState *cpu = arg;
> >   
> > +    tcg_thread_cpu = cpu;
> >       qemu_tcg_init_cpu_signals();
> >       qemu_thread_get_self(cpu->thread);
> >   
> > @@ -1393,7 +1402,8 @@ static void tcg_exec_all(void)
> >       if (next_cpu == NULL) {
> >           next_cpu = first_cpu;
> >       }
> > -    for (; next_cpu != NULL && !exit_request; next_cpu = CPU_NEXT(next_cpu)) {
> > +    for (; next_cpu != NULL && !first_cpu->exit_request;
> > +           next_cpu = CPU_NEXT(next_cpu)) {
> >           CPUState *cpu = next_cpu;
> >           CPUArchState *env = cpu->env_ptr;
> >   
> > @@ -1410,7 +1420,8 @@ static void tcg_exec_all(void)
> >               break;
> >           }
> >       }
> > -    exit_request = 0;
> > +
> > +    first_cpu->exit_request = 0;
> >   }
> >   
> >   void list_cpus(FILE *f, fprintf_function cpu_fprintf, const char *optarg)