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

[alvise rigo]

On Fri, Jul 17, 2015 at 3:45 PM, Alex Bennée <address@hidden> wrote:
>
> Alvise Rigo <address@hidden> 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 <address@hidden>
>> Suggested-by: Claudio Fontana <address@hidden>
>> Signed-off-by: Alvise Rigo <address@hidden>
>> ---
>>  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?

Yes, it will be more likely based on the plain async_run_on_cpu.

>
>
>>      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 my local branch I'm now using atomic_set/atomic_read, basically
what is defined in qemu/atomic.h. Is this enough?

Regards,
alvise

>
>
>>      /* 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