Re: [PATCH v2] accel/tcg/tcg-accel-ops-rr: ensure fairness with icount

[Richard Henderson <richard.henderson@linaro.org>]

On 4/24/23 12:29, Jamie Iles wrote:
> +/*
> + * Calculate the number of CPUs that we will process in a single iteration of
> + * the main CPU thread loop so that we can fairly distribute the instruction
> + * count across CPUs.
> + *
> + * The CPU count is cached based on the CPU list generation ID to avoid
> + * iterating the list every time.
> + */
> +static int rr_cpu_count(void)
> +{
> +    static unsigned int last_gen_id = ~0;
> +    static int cpu_count;
> +    CPUState *cpu;
> +
> +    cpu_list_lock();
> +    if (cpu_list_generation_id_get() != last_gen_id) {
> +        cpu_count = 0;
> +        CPU_FOREACH(cpu) {
> +            ++cpu_count;
> +        }
> +        last_gen_id = cpu_list_generation_id_get();
> +    }
> +    cpu_list_unlock();
> +
> +    return cpu_count;
> +}

The read of cpu_count should be in the lock.

(Ideally we'd expose QEMU_LOCK_GUARD(&qemu_cpu_list_lock) which would make the read+return 
trivial.)


>   /*
>    * 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
> @@ -185,6 +212,9 @@ static void *rr_cpu_thread_fn(void *arg)
>       cpu->exit_request = 1;
>   
>       while (1) {
> +        int cpu_count = rr_cpu_count();
> +        int64_t cpu_budget = INT64_MAX;
> +
>           qemu_mutex_unlock_iothread();
>           replay_mutex_lock();
>           qemu_mutex_lock_iothread();
> @@ -197,6 +227,8 @@ static void *rr_cpu_thread_fn(void *arg)
>                * waking up the I/O thread and waiting for completion.
>                */
>               icount_handle_deadline();
> +
> +            cpu_budget = icount_percpu_budget(cpu_count);

I think you can move the call to rr_cpu_count() here, so that it only happens if icount is 
in use.

Why cpu_budget = INT64_MAX as opposed to 0 or 0xdeadbeef?  It's not set or used except for 
icount_enabled.


r~