Re: [PATCH v4 09/13] accel/all: Extract common_vcpu_thread_create()

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

On 3/23/22 10:17, Philippe Mathieu-Daudé wrote:
> From: Philippe Mathieu-Daudé <f4bug@amsat.org>
> 
> All accelerators implement a very similar create_vcpu_thread()
> handler. Extract the common part as common_vcpu_thread_create(),
> which only requires the AccelOpsClass::vcpu_thread_fn() routine
> and the accelerator AccelOpsClass::thread_name for debugging
> purpose.
> 
> The single exception is TCG/RR which re-use a single vCPU. Have
> it use the same logic by using the .precheck/.postcheck handlers.
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Too much all at once.
But I see now why you moved code in the direction you did in patch 5.

> @@ -279,28 +279,13 @@ bool rr_create_vcpu_thread_precheck(CPUState *cpu)
>       return !single_tcg_cpu_thread;
>   }
>   
> -void rr_start_vcpu_thread(CPUState *cpu)
> +void rr_create_vcpu_thread_postcheck(CPUState *cpu)
>   {
> -    char thread_name[VCPU_THREAD_NAME_SIZE];
>       static QemuCond *single_tcg_halt_cond;
> -    static QemuThread *single_tcg_cpu_thread;

Patch 8 is not really standalone, since you didn't move this variable.
I think it's probably a mistake to split out precheck and postcheck separately.

> -
> -    if (!single_tcg_cpu_thread) {
> +    if (! single_tcg_cpu_thread) {

Extraneous whitespace.

> @@ -30,7 +30,8 @@ struct AccelOpsClass {
>   
>       bool (*cpus_are_resettable)(void);
>   
> -    void (*create_vcpu_thread)(CPUState *cpu); /* MANDATORY NON-NULL */
> +    const char *thread_name;

Why don't we just get this name from the AccelClass?

> +static void common_vcpu_thread_create(CPUState *cpu)
> +{
> +    char thread_name[VCPU_THREAD_NAME_SIZE];
> +
> +    cpu->thread = g_new0(QemuThread, 1);
> +    cpu->halt_cond = g_new0(QemuCond, 1);
> +    qemu_cond_init(cpu->halt_cond);
> +    snprintf(thread_name, VCPU_THREAD_NAME_SIZE, "CPU %d/%s",
> +             cpu->cpu_index, cpus_accel->thread_name ?: "DUMMY");

Surely g_strdup_printf is better than the static size buffer.


r~