Re: [Qemu-devel] [RFC PATCH v1 7/9] cpus: move icount preparation out of tcg_exec_cpu

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

Pavel Dovgalyuk <dovgaluk@ispras.ru> writes:

> I guess you are trying to fix the sympthoms of the case
> when iothread is trying to access instruction count.

In theory the main-loop should be sequenced before or after vCPU events
because of the BQL. I'm not sure why this is not currently the case.

> Maybe the solution is providing access to current_cpu for the iothread
> coupled with your patch 8?

Providing cross-thread access to CPU structures brings its own
challenges.

But it does occur to me we should probably ensure
timer_state.qemu_icount has appropriate barriers. This should be ensured
by the BQL but if it is ever accessed by 2 threads without a BQL
transition in-between then it is potentially racey.

>
> Pavel Dovgalyuk
>
>
>> -----Original Message-----
>> From: Alex Bennée [mailto:alex.bennee@linaro.org]
>> Sent: Monday, April 03, 2017 3:45 PM
>> To: dovgaluk@ispras.ru; rth@twiddle.net; pbonzini@redhat.com
>> Cc: peter.maydell@linaro.org; qemu-devel@nongnu.org; mttcg@listserver.greensocs.com;
>> fred.konrad@greensocs.com; a.rigo@virtualopensystems.com; cota@braap.org;
>> bobby.prani@gmail.com; nikunj@linux.vnet.ibm.com; Alex Bennée; Peter Crosthwaite
>> Subject: [RFC PATCH v1 7/9] cpus: move icount preparation out of tcg_exec_cpu
>>
>> As icount is only supported for single-threaded execution due to the
>> requirement for determinism let's remove it from the common
>> tcg_exec_cpu path.
>>
>> Also remove the additional fiddling which shouldn't be required as the
>> icount counters should all be rectified as you enter the loop.
>>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> ---
>>  cpus.c | 67 +++++++++++++++++++++++++++++++++++++++++++++---------------------
>>  1 file changed, 46 insertions(+), 21 deletions(-)
>>
>> diff --git a/cpus.c b/cpus.c
>> index 18b1746770..87638a75d2 100644
>> --- a/cpus.c
>> +++ b/cpus.c
>> @@ -1179,47 +1179,66 @@ static void handle_icount_deadline(void)
>>      }
>>  }
>>
>> -static int tcg_cpu_exec(CPUState *cpu)
>> +static void prepare_icount_for_run(CPUState *cpu)
>>  {
>> -    int ret;
>> -#ifdef CONFIG_PROFILER
>> -    int64_t ti;
>> -#endif
>> -
>> -#ifdef CONFIG_PROFILER
>> -    ti = profile_getclock();
>> -#endif
>>      if (use_icount) {
>>          int64_t count;
>>          int decr;
>> -        timers_state.qemu_icount -= (cpu->icount_decr.u16.low
>> -                                    + cpu->icount_extra);
>> -        cpu->icount_decr.u16.low = 0;
>> -        cpu->icount_extra = 0;
>> +
>> +        /* These should always be cleared by process_icount_data after
>> +         * each vCPU execution. However u16.high can be raised
>> +         * asynchronously by cpu_exit/cpu_interrupt/tcg_handle_interrupt
>> +         */
>> +        g_assert(cpu->icount_decr.u16.low == 0);
>> +        g_assert(cpu->icount_extra == 0);
>> +
>> +
>>          count = tcg_get_icount_limit();
>> +
>>          timers_state.qemu_icount += count;
>>          decr = (count > 0xffff) ? 0xffff : count;
>>          count -= decr;
>>          cpu->icount_decr.u16.low = decr;
>>          cpu->icount_extra = count;
>>      }
>> -    qemu_mutex_unlock_iothread();
>> -    cpu_exec_start(cpu);
>> -    ret = cpu_exec(cpu);
>> -    cpu_exec_end(cpu);
>> -    qemu_mutex_lock_iothread();
>> -#ifdef CONFIG_PROFILER
>> -    tcg_time += profile_getclock() - ti;
>> -#endif
>> +}
>> +
>> +static void process_icount_data(CPUState *cpu)
>> +{
>>      if (use_icount) {
>>          /* Fold pending instructions back into the
>>             instruction counter, and clear the interrupt flag.  */
>>          timers_state.qemu_icount -= (cpu->icount_decr.u16.low
>>                          + cpu->icount_extra);
>> +
>> +        /* We must be under BQL here as cpu_exit can tweak
>> +           icount_decr.u32 */
>> +        g_assert(qemu_mutex_iothread_locked());
>>          cpu->icount_decr.u32 = 0;
>>          cpu->icount_extra = 0;
>>          replay_account_executed_instructions();
>>      }
>> +}
>> +
>> +
>> +static int tcg_cpu_exec(CPUState *cpu)
>> +{
>> +    int ret;
>> +#ifdef CONFIG_PROFILER
>> +    int64_t ti;
>> +#endif
>> +
>> +#ifdef CONFIG_PROFILER
>> +    ti = profile_getclock();
>> +#endif
>> +    qemu_mutex_unlock_iothread();
>> +    cpu_exec_start(cpu);
>> +    ret = cpu_exec(cpu);
>> +    cpu_exec_end(cpu);
>> +    qemu_mutex_lock_iothread();
>> +#ifdef CONFIG_PROFILER
>> +    tcg_time += profile_getclock() - ti;
>> +#endif
>>      return ret;
>>  }
>>
>> @@ -1306,7 +1325,13 @@ static void *qemu_tcg_rr_cpu_thread_fn(void *arg)
>>
>>              if (cpu_can_run(cpu)) {
>>                  int r;
>> +
>> +                prepare_icount_for_run(cpu);
>> +
>>                  r = tcg_cpu_exec(cpu);
>> +
>> +                process_icount_data(cpu);
>> +
>>                  if (r == EXCP_DEBUG) {
>>                      cpu_handle_guest_debug(cpu);
>>                      break;
>> --
>> 2.11.0


--
Alex Bennée