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

[Frederic Konrad <fred.konrad@greensocs.com>]

On 07/07/2015 15:04, Alex Bennée wrote:
> fred.konrad@greensocs.com writes:
>
>> From: KONRAD Frederic <fred.konrad@greensocs.com>
>>
>> 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 <fred.konrad@greensocs.com>
>> ---
>>   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)