Re: [Qemu-devel] [RFC PATCH V2 2/3] cpus: add a tcg_executing flag.

[Frederic Konrad]

On 13/07/2015 17:56, Alex Bennée wrote:
> address@hidden writes:
>
> > From: KONRAD Frederic <address@hidden>
> >
> > This flag indicates if the VCPU is currently executing TCG code.
> >
> > Signed-off-by: KONRAD Frederic <address@hidden>
> >
> > Changes V1 -> V2:
> >    * do both tcg_executing = 0 or 1 in cpu_exec().
> > ---
> >   cpu-exec.c        | 2 ++
> >   include/qom/cpu.h | 3 +++
> >   qom/cpu.c         | 1 +
> >   3 files changed, 6 insertions(+)
> >
> > diff --git a/cpu-exec.c b/cpu-exec.c
> > index 75694f3..2fdf89d 100644
> > --- a/cpu-exec.c
> > +++ b/cpu-exec.c
> > @@ -371,6 +371,7 @@ int cpu_exec(CPUState *cpu)
> >           cpu->halted = 0;
> >       }
> >   
> > +    cpu->tcg_executing = 1;
> >       current_cpu = cpu;
> >   
> >       /* As long as current_cpu is null, up to the assignment just above,
> > @@ -583,5 +584,6 @@ int cpu_exec(CPUState *cpu)
> >   
> >       /* fail safe : never use current_cpu outside cpu_exec() */
> >       current_cpu = NULL;
> > +    cpu->tcg_executing = 0;
> >       return ret;
> >   }
> > diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> > index efa9624..a2de536 100644
> > --- a/include/qom/cpu.h
> > +++ b/include/qom/cpu.h
> > @@ -226,6 +226,7 @@ struct kvm_run;
> >    * @stopped: Indicates the CPU has been artificially stopped.
> >    * @tcg_exit_req: Set to force TCG to stop executing linked TBs for this
> >    *           CPU and return to its top level loop.
> > + * @tcg_executing: This TCG thread is in cpu_exec().
> >    * @singlestep_enabled: Flags for single-stepping.
> >    * @icount_extra: Instructions until next timer event.
> >    * @icount_decr: Number of cycles left, with interrupt flag in high bit.
> > @@ -322,6 +323,8 @@ struct CPUState {
> >          (absolute value) offset as small as possible.  This reduces code
> >          size, especially for hosts without large memory offsets.  */
> >       volatile sig_atomic_t tcg_exit_req;
> > +
> > +    volatile int tcg_executing;
> My concern is on weakly ordered backends is volatile enough for this
> flag or do we need some sort of memory barrier when we update it? Does
> it just introduce an inefficiency that other threads may spin a few
> times waiting to find out the vCPU has halted?

I think it will just spin (see in flush_queued_safe_work in patch 3).
> If other threads are waiting for it to halt is there a mechanism that
> ensures we'll never start-up again until everything is done?
This flag is not supposed to do that, it's in the third patch as well.

It will check async_safe_work_pending before starting the execution.
We might have a race here, if the flush is triggered between
async_safe_work_pending and the tcg_executing flag set in cpu-exec.

    if (async_safe_work_pending()) {
        cpu->exit_request = 1;
        return 0;
    }

    if (cpu->halted) {
        if (!cpu_has_work(cpu)) {
            return EXCP_HALTED;
        }

        cpu->halted = 0;
    }

    cpu->tcg_executing = 1;

I need to check and fix that.

Fred

> >   };
> >   
> >   QTAILQ_HEAD(CPUTailQ, CPUState);
> > diff --git a/qom/cpu.c b/qom/cpu.c
> > index 4e12598..62663e5 100644
> > --- a/qom/cpu.c
> > +++ b/qom/cpu.c
> > @@ -249,6 +249,7 @@ static void cpu_common_reset(CPUState *cpu)
> >       cpu->icount_decr.u32 = 0;
> >       cpu->can_do_io = 0;
> >       cpu->exception_index = -1;
> > +    cpu->tcg_executing = 0;
> >       memset(cpu->tb_jmp_cache, 0, TB_JMP_CACHE_SIZE * sizeof(void *));
> >   }