Re: [Qemu-devel] [RFC v4 14/28] tcg: add kick timer for single-threaded vCPU emulation

[Alex Bennée]

Richard Henderson <address@hidden> writes:

> On 08/11/2016 08:24 AM, Alex Bennée wrote:
>> Currently we rely on the side effect of the main loop grabbing the
>> iothread_mutex to give any long running basic block chains a kick to
>> ensure the next vCPU is scheduled. As this code is being re-factored and
>> rationalised we now do it explicitly here.
>>
>> Signed-off-by: Alex Bennée <address@hidden>
>>
>> ---
>> v2
>>   - re-base fixes
>>   - get_ticks_per_sec() -> NANOSECONDS_PER_SEC
>> v3
>>   - add define for TCG_KICK_FREQ
>>   - fix checkpatch warning
>> v4
>>   - wrap next calc in inline qemu_tcg_next_kick() instead of macro
>> ---
>>  cpus.c | 32 ++++++++++++++++++++++++++++++++
>>  1 file changed, 32 insertions(+)
>>
>> diff --git a/cpus.c b/cpus.c
>> index b5b45b8..8c49d6c 100644
>> --- a/cpus.c
>> +++ b/cpus.c
>> @@ -1185,9 +1185,34 @@ static void deal_with_unplugged_cpus(void)
>>      }
>>  }
>>
>> +/* Single-threaded TCG
>> + *
>> + * In the single-threaded case each vCPU is simulated in turn. If
>> + * there is more than a single vCPU we create a simple timer to kick
>> + * the vCPU and ensure we don't get stuck in a tight loop in one vCPU.
>> + * This is done explicitly rather than relying on side-effects
>> + * elsewhere.
>> + */
>> +static void qemu_cpu_kick_no_halt(void);
>> +
>> +#define TCG_KICK_PERIOD (NANOSECONDS_PER_SECOND / 10)
>> +
>> +static inline int64_t qemu_tcg_next_kick(void)
>> +{
>> +    return qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + TCG_KICK_PERIOD;
>> +}
>> +
>> +static void kick_tcg_thread(void *opaque)
>> +{
>> +    QEMUTimer *self = *(QEMUTimer **) opaque;
>> +    timer_mod(self, qemu_tcg_next_kick());
>> +    qemu_cpu_kick_no_halt();
>> +}
>> +
>>  static void *qemu_tcg_cpu_thread_fn(void *arg)
>>  {
>>      CPUState *cpu = arg;
>> +    QEMUTimer *kick_timer;
>>
>>      rcu_register_thread();
>>
>> @@ -1211,6 +1236,13 @@ static void *qemu_tcg_cpu_thread_fn(void *arg)
>>          }
>>      }
>>
>> +    /* Set to kick if we have to do more than one vCPU */
>> +    if (CPU_NEXT(first_cpu)) {
>> +        kick_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,  kick_tcg_thread,
>> +                                  &kick_timer);
>
> I'm not especially keen on this pointer to local variable thing.  Perhaps 
> better as
>
>    kick_timer = timer_new_ns(..., NULL);
>    kick_timer->opaque = kick_timer;

Yeah it is a bit less magical that way.

>
> and avoid the indirection in kick_tcg_thread.
>
> Also, we should shut down the timer when the cpu is removed, surely?

That is an interesting point - but the timer is shared across all CPUs
so we need to keep kicking until there is only one CPU left. AFAICT the
current cpu hot plug code basically just suspends the vCPU forever
rather than trying to clean up all its associated resources.

>
>
> r~


--
Alex Bennée