Sergey Fedorov <serge.f...@gmail.com> 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 <fred.kon...@greensocs.com>
>> Signed-off-by: Paolo Bonzini <pbonz...@redhat.com>
>> [AJB: Some fixes, conditionally, commit rewording]
>> Signed-off-by: Alex Bennée <alex.ben...@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()"?
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
