Re: [Qemu-devel] [RFC v3 17/19] tcg: enable thread-per-vCPU

[Sergey Fedorov <serge.fdrv@gmail.com>]

On 03/06/16 23:40, Alex Bennée wrote:
> There are a number of changes that occur at the same time here:
>
>   - tb_lock is no longer a NOP for SoftMMU
>
>   The tb_lock protects both translation and memory map structures. The
>   debug assert is updated to reflect this.

This could be a separate patch.

If we use tb_lock in system-mode to protect the structures protected by
mmap_lock in user-mode then maybe we can merge those two locks because,
as I remember, tb_lock in user-mode emulation is only held outside of
mmap_lock for patching TB for direct jumps.

>
>   - introduce a single vCPU qemu_tcg_cpu_thread_fn
>
>   One of these is spawned per vCPU with its own Thread and Condition
>   variables. qemu_tcg_single_cpu_thread_fn is the new name for the old
>   single threaded function.

So we have 'tcg_current_rr_cpu' and 'qemu_cpu_kick_rr_cpu() at this
moment, maybe name this function like qemu_tcg_rr_cpu_thread_fn()? ;)

>
>   - the TLS current_cpu variable is now live for the lifetime of MTTCG
>     vCPU threads. This is for future work where async jobs need to know
>     the vCPU context they are operating in.

This is important change because we set 'current_cpu' to NULL outside of
cpu_exec() before, I wonder why.

>
> The user to switch on multi-thread behaviour and spawn a thread
> per-vCPU. For a simple test like:
>
>   ./arm/run ./arm/locking-test.flat -smp 4 -accel tcg,thread=multi

It would be nice to mention that the simple test is from kvm_unit_tests.

>
> Will now use 4 vCPU threads and have an expected FAIL (instead of the
> unexpected PASS) as the default mode of the test has no protection when
> incrementing a shared variable.
>
> However we still default to a single thread for all vCPUs as individual
> front-end and back-ends need additional fixes to safely support:
>   - atomic behaviour
>   - tb invalidation
>   - memory ordering
>
> The function default_mttcg_enabled can be tweaked as support is added.
>
> Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> [AJB: Some fixes, conditionally, commit rewording]
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>
(snip)
> diff --git a/cpus.c b/cpus.c
> index 35374fd..419caa2 100644
> --- a/cpus.c
> +++ b/cpus.c
(snip)
> @@ -1042,9 +1039,7 @@ static void qemu_tcg_wait_io_event(CPUState *cpu)
>          qemu_cond_wait(cpu->halt_cond, &qemu_global_mutex);
>      }
>  
> -    CPU_FOREACH(cpu) {
> -        qemu_wait_io_event_common(cpu);
> -    }
> +    qemu_wait_io_event_common(cpu);

Is it okay for single-threaded CPU loop?

>  }
>  
>  static void qemu_kvm_wait_io_event(CPUState *cpu)
(snip)
> @@ -1331,6 +1324,69 @@ static void *qemu_tcg_cpu_thread_fn(void *arg)
>      return NULL;
>  }
>  
> +/* Multi-threaded TCG
> + *
> + * In the multi-threaded case each vCPU has its own thread. The TLS
> + * variable current_cpu can be used deep in the code to find the
> + * current CPUState for a given thread.
> + */
> +
> +static void *qemu_tcg_cpu_thread_fn(void *arg)
> +{
> +    CPUState *cpu = arg;
> +
> +    rcu_register_thread();
> +
> +    qemu_mutex_lock_iothread();
> +    qemu_thread_get_self(cpu->thread);
> +
> +    cpu->thread_id = qemu_get_thread_id();
> +    cpu->created = true;
> +    cpu->can_do_io = 1;
> +    current_cpu = cpu;
> +    qemu_cond_signal(&qemu_cpu_cond);
> +
> +    /* process any pending work */
> +    atomic_mb_set(&cpu->exit_request, 1);
> +
> +    while (1) {
> +        bool sleep = false;
> +
> +        if (cpu_can_run(cpu)) {
> +            int r = tcg_cpu_exec(cpu);
> +            switch (r) {
> +            case EXCP_DEBUG:
> +                cpu_handle_guest_debug(cpu);
> +                break;
> +            case EXCP_HALTED:
> +                /* during start-up the vCPU is reset and the thread is
> +                 * kicked several times. If we don't ensure we go back
> +                 * to sleep in the halted state we won't cleanly
> +                 * start-up when the vCPU is enabled.
> +                 */
> +                sleep = true;
> +                break;
> +            default:
> +                /* Ignore everything else? */
> +                break;
> +            }
> +        } else {
> +            sleep = true;
> +        }
> +
> +        handle_icount_deadline();
> +
> +        if (sleep) {
> +            qemu_cond_wait(cpu->halt_cond, &qemu_global_mutex);
> +        }
> +
> +        atomic_mb_set(&cpu->exit_request, 0);
> +        qemu_tcg_wait_io_event(cpu);

Do we really want to wait in qemu_tcg_wait_io_event() while
"all_cpu_threads_idle()"?

> +    }
> +
> +    return NULL;
> +}
> +
>  static void qemu_cpu_kick_thread(CPUState *cpu)
>  {
>  #ifndef _WIN32
> @@ -1355,7 +1411,7 @@ void qemu_cpu_kick(CPUState *cpu)
>      qemu_cond_broadcast(cpu->halt_cond);
>      if (tcg_enabled()) {
>          cpu_exit(cpu);
> -        /* Also ensure current RR cpu is kicked */
> +        /* NOP unless doing single-thread RR */
>          qemu_cpu_kick_rr_cpu();
>      } else {
>          qemu_cpu_kick_thread(cpu);
> @@ -1422,13 +1478,6 @@ void pause_all_vcpus(void)
>  
>      if (qemu_in_vcpu_thread()) {
>          cpu_stop_current();
> -        if (!kvm_enabled()) {
> -            CPU_FOREACH(cpu) {
> -                cpu->stop = false;
> -                cpu->stopped = true;
> -            }
> -            return;
> -        }

I think this change is incompatible with single-threaded CPU loop as well.

>      }
>  
>      while (!all_vcpus_paused()) {
>
(snip)

Kind regards,
Sergey