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

[Frederic Konrad]

On 26/06/2015 17:02, Paolo Bonzini wrote:
> On 26/06/2015 16:47, address@hidden wrote:
> > From: KONRAD Frederic <address@hidden>
> >
> > This removes tcg_halt_cond global variable.
> > We need one QemuCond per virtual cpu for multithread TCG.
> >
> > Signed-off-by: KONRAD Frederic <address@hidden>
> > ---
> >   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;
> >       }