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

[Alex Bennée]

Sergey Fedorov <address@hidden> writes:

> 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.

OK

>
>>
>>   - 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()? ;)

OK

>
>>
>>   - 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.

It's hard to tell, it is not heavily defended. The number of places that
check current_cpu != NULL is fairly limited.

>
>>
>> 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 <address@hidden>
>> Signed-off-by: Paolo Bonzini <address@hidden>
>> [AJB: Some fixes, conditionally, commit rewording]
>> Signed-off-by: Alex Bennée <address@hidden>
>>
> (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()"?

I've cleaned up this logic.

>
>> +    }
>> +
>> +    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.

Why, we already stop and kick the vCPU above so we will exit.

>
>>      }
>>
>>      while (!all_vcpus_paused()) {
>>
> (snip)
>
> Kind regards,
> Sergey


--
Alex Bennée