Re: [Qemu-devel] [RFC v3 08/13] exec.c: introduce a simple rendezvous support

["Alex Bennée" <alex.bennee@linaro.org>]

Alvise Rigo <a.rigo@virtualopensystems.com> writes:

> When a vCPU is about to set a memory page as exclusive, it needs to wait
> that all the running vCPUs finish to execute the current TB and to be aware
> of the exact moment when that happens. For this, add a simple rendezvous
> mechanism that will be used in softmmu_llsc_template.h to implement the
> ldlink operation.
>
> Suggested-by: Jani Kokkonen <jani.kokkonen@huawei.com>
> Suggested-by: Claudio Fontana <claudio.fontana@huawei.com>
> Signed-off-by: Alvise Rigo <a.rigo@virtualopensystems.com>
> ---
>  cpus.c            |  5 +++++
>  exec.c            | 45 +++++++++++++++++++++++++++++++++++++++++++++
>  include/qom/cpu.h | 16 ++++++++++++++++
>  3 files changed, 66 insertions(+)
>
> diff --git a/cpus.c b/cpus.c
> index aee445a..f4d938e 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -1423,6 +1423,11 @@ static int tcg_cpu_exec(CPUArchState *env)
>      qemu_mutex_unlock_iothread();
>      ret = cpu_exec(env);
>      cpu->tcg_executing = 0;
> +
> +    if (unlikely(cpu->pending_rdv)) {
> +        cpu_exit_do_rendezvous(cpu);
> +    }
> +

I'll ignore this stuff for now as I assume we can all use the async work
patch of Fred's?


>      qemu_mutex_lock_iothread();
>  #ifdef CONFIG_PROFILER
>      tcg_time += profile_getclock() - ti;
> diff --git a/exec.c b/exec.c
> index 964e922..51958ed 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -746,6 +746,51 @@ void cpu_breakpoint_remove_all(CPUState *cpu, int mask)
>      }
>  }
>  
> +/* Rendezvous implementation.
> + * The corresponding definitions are in include/qom/cpu.h. */
> +CpuExitRendezvous cpu_exit_rendezvous;
> +inline void cpu_exit_init_rendezvous(void)
> +{
> +    CpuExitRendezvous *rdv = &cpu_exit_rendezvous;
> +
> +    rdv->attendees = 0;
> +}
> +
> +inline void cpu_exit_rendezvous_add_attendee(CPUState *cpu)
> +{
> +    CpuExitRendezvous *rdv = &cpu_exit_rendezvous;
> +
> +    if (!cpu->pending_rdv) {
> +        cpu->pending_rdv = 1;
> +        atomic_inc(&rdv->attendees);
> +    }
> +}
> +
> +void cpu_exit_do_rendezvous(CPUState *cpu)
> +{
> +    CpuExitRendezvous *rdv = &cpu_exit_rendezvous;
> +
> +    atomic_dec(&rdv->attendees);
> +
> +    cpu->pending_rdv = 0;
> +}
> +
> +void cpu_exit_rendezvous_wait_others(CPUState *cpu)
> +{
> +    CpuExitRendezvous *rdv = &cpu_exit_rendezvous;
> +
> +    while (rdv->attendees) {
> +        g_usleep(TCG_RDV_POLLING_PERIOD);
> +    }
> +}
> +
> +void cpu_exit_rendezvous_release(void)
> +{
> +    CpuExitRendezvous *rdv = &cpu_exit_rendezvous;
> +
> +    rdv->attendees = 0;
> +}
> +
>  /* enable or disable single step mode. EXCP_DEBUG is returned by the
>     CPU loop after each instruction */
>  void cpu_single_step(CPUState *cpu, int enabled)
> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> index 8f3fe56..8d121b3 100644
> --- a/include/qom/cpu.h
> +++ b/include/qom/cpu.h
> @@ -201,6 +201,20 @@ typedef struct CPUWatchpoint {
>      QTAILQ_ENTRY(CPUWatchpoint) entry;
>  } CPUWatchpoint;
>  
> +/* Rendezvous support */
> +#define TCG_RDV_POLLING_PERIOD 10
> +typedef struct CpuExitRendezvous {
> +    volatile int attendees;
> +    QemuMutex lock;
> +} CpuExitRendezvous;
> +
> +extern CpuExitRendezvous cpu_exit_rendezvous;
> +void cpu_exit_init_rendezvous(void);
> +void cpu_exit_rendezvous_add_attendee(CPUState *cpu);
> +void cpu_exit_do_rendezvous(CPUState *cpu);
> +void cpu_exit_rendezvous_wait_others(CPUState *cpu);
> +void cpu_exit_rendezvous_release(void);
> +
>  struct KVMState;
>  struct kvm_run;
>  
> @@ -291,6 +305,8 @@ struct CPUState {
>  
>      void *opaque;
>  
> +    volatile int pending_rdv;
> +

I will however echo the "hmmm" on Fred's patch about the optimistic use
of volatile here. As Peter mentioned it is a red flag and I would prefer
explicit memory consistency behaviour used and documented.


>      /* In order to avoid passing too many arguments to the MMIO helpers,
>       * we store some rarely used information in the CPU context.
>       */

-- 
Alex Bennée