Re: [Qemu-devel] [RFC PATCH V6 06/18] tcg: remove tcg_halt_cond global variable.

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

On 26/06/2015 17:02, Paolo Bonzini wrote:
>
> On 26/06/2015 16:47, fred.konrad@greensocs.com wrote:
>> From: KONRAD Frederic <fred.konrad@greensocs.com>
>>
>> This removes tcg_halt_cond global variable.
>> We need one QemuCond per virtual cpu for multithread TCG.
>>
>> Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com>
>> ---
>>   cpus.c | 18 +++++++-----------
>>   1 file changed, 7 insertions(+), 11 deletions(-)
>>
>> diff --git a/cpus.c b/cpus.c
>> index 2d62a35..79383df 100644
>> --- a/cpus.c
>> +++ b/cpus.c
>> @@ -813,7 +813,6 @@ static unsigned iothread_requesting_mutex;
>>   static QemuThread io_thread;
>>   
>>   static QemuThread *tcg_cpu_thread;
>> -static QemuCond *tcg_halt_cond;
>>   
>>   /* cpu creation */
>>   static QemuCond qemu_cpu_cond;
>> @@ -919,15 +918,13 @@ static void qemu_wait_io_event_common(CPUState *cpu)
>>       cpu->thread_kicked = false;
>>   }
>>   
>> -static void qemu_tcg_wait_io_event(void)
>> +static void qemu_tcg_wait_io_event(CPUState *cpu)
>>   {
>> -    CPUState *cpu;
>> -
>>       while (all_cpu_threads_idle()) {
>>          /* Start accounting real time to the virtual clock if the CPUs
>>             are idle.  */
>>           qemu_clock_warp(QEMU_CLOCK_VIRTUAL);
>> -        qemu_cond_wait(tcg_halt_cond, &qemu_global_mutex);
>> +        qemu_cond_wait(cpu->halt_cond, &qemu_global_mutex);
>>       }
>>   
>>       while (iothread_requesting_mutex) {
>> @@ -1047,7 +1044,7 @@ static void *qemu_tcg_cpu_thread_fn(void *arg)
>>   
>>       /* wait for initial kick-off after machine start */
>>       while (first_cpu->stopped) {
>> -        qemu_cond_wait(tcg_halt_cond, &qemu_global_mutex);
>> +        qemu_cond_wait(first_cpu->halt_cond, &qemu_global_mutex);
>>   
>>           /* process any pending work */
>>           CPU_FOREACH(cpu) {
>> @@ -1068,7 +1065,7 @@ static void *qemu_tcg_cpu_thread_fn(void *arg)
>>                   qemu_clock_notify(QEMU_CLOCK_VIRTUAL);
>>               }
>>           }
>> -        qemu_tcg_wait_io_event();
>> +        qemu_tcg_wait_io_event(QTAILQ_FIRST(&cpus));
> Does this work (for non-multithreaded TCG) if tcg_thread_fn is waiting
> on the "wrong" condition variable?  For example if all CPUs are idle and
> the second CPU wakes up, qemu_tcg_wait_io_event won't be kicked out of
> the wait.
>
> I think you need to have a CPUThread struct like this:
>
>     struct CPUThread {
>         QemuThread thread;
>         QemuCond halt_cond;
>     };
>
> and in CPUState have a CPUThread * field instead of the thread and
> halt_cond fields.
>
> Then single-threaded TCG can point all CPUStates to the same instance of
> the struct, while multi-threaded TCG can point each CPUState to a
> different struct.
>
> Paolo

Hmm probably not, though we didn't pay attention to keep the non MTTCG 
working.
(which is probably not good).

>
>>       }
>>   
>>       return NULL;
>> @@ -1235,12 +1232,12 @@ static void qemu_tcg_init_vcpu(CPUState *cpu)
>>   
>>       tcg_cpu_address_space_init(cpu, cpu->as);
>>   
>> +    cpu->halt_cond = g_malloc0(sizeof(QemuCond));
>> +    qemu_cond_init(cpu->halt_cond);
>> +
>>       /* share a single thread for all cpus with TCG */
>>       if (!tcg_cpu_thread) {
>>           cpu->thread = g_malloc0(sizeof(QemuThread));
>> -        cpu->halt_cond = g_malloc0(sizeof(QemuCond));
>> -        qemu_cond_init(cpu->halt_cond);
>> -        tcg_halt_cond = cpu->halt_cond;
>>           snprintf(thread_name, VCPU_THREAD_NAME_SIZE, "CPU %d/TCG",
>>                    cpu->cpu_index);
>>           qemu_thread_create(cpu->thread, thread_name, qemu_tcg_cpu_thread_fn,
>> @@ -1254,7 +1251,6 @@ static void qemu_tcg_init_vcpu(CPUState *cpu)
>>           tcg_cpu_thread = cpu->thread;
>>       } else {
>>           cpu->thread = tcg_cpu_thread;
>> -        cpu->halt_cond = tcg_halt_cond;
>>       }