Re: [PULL 3/5] softmmu/cpus: Only set parallel_cpus for SMP - Philippe Mathieu-Daudé

From: "Philippe Mathieu-Daudé" <f4bug@amsat.org>
To: Claudio Fontana <cfontana@suse.de>
Cc: peter.maydell@linaro.org,
	Richard Henderson <richard.henderson@linaro.org>,
	qemu-devel@nongnu.org, Paolo Bonzini <pbonzini@redhat.com>
Subject: Re: [PULL 3/5] softmmu/cpus: Only set parallel_cpus for SMP
Date: Tue, 8 Sep 2020 09:56:07 +0200	[thread overview]
Message-ID: <652516a1-6a63-89eb-8b04-728a95b76d5f@amsat.org> (raw)
In-Reply-To: <75c313df-da09-f3c2-5d34-0bfaad4290a9@suse.de>

+Laurent

On 9/8/20 9:09 AM, Claudio Fontana wrote:
> On 9/7/20 6:49 PM, Philippe Mathieu-Daudé wrote:
>> On 9/7/20 6:30 PM, Claudio Fontana wrote:
>>> On 9/7/20 12:20 PM, Philippe Mathieu-Daudé wrote:
>>>> On 9/7/20 12:05 PM, Claudio Fontana wrote:
>>>>> Hi Richard,
>>>>>
>>>>> currently rebasing on top of this one,
>>>>> just a question, why is the patch not directly using "current_machine"?
>>>>
>>>> For user mode?
>>>
>>> In user mode I'd not expect softmmu/cpus.c to be built at all...
>>
>> Which is why :) current_machine is NULL in user-mode.
> 
> Ciao Philippe,
> 
> then why does the patch change softmmu/cpus.c in a way that accounts for user mode?
> 
> The function that the patch changes is never called in user mode.
> 
> The patch could instead use current_machine without any concern of it being NULL, it will always be set in vl.c .

Better send a patch to prove your point :)

I have bad remember about the last time I tried to understand/modify
that part, because IIRC the user-mode creates some fake machine to
satisfy various QEMU generic code assumptions. Sincerely I now prefer
stay away from this; too many unmerged patches there.

> 
> Ciao,
> 
> Claudio
> 
>>
>>>
>>> Ciao,
>>>
>>> Claudio
>>>
>>>>
>>>>>
>>>>> Is using MACHINE(qdev_get_machine()) preferrable here?
>>>>>
>>>>> Thanks,
>>>>>
>>>>> Claudio
>>>>>
>>>>> On 9/3/20 11:40 PM, Richard Henderson wrote:
>>>>>> Do not set parallel_cpus if there is only one cpu instantiated.
>>>>>> This will allow tcg to use serial code to implement atomics.
>>>>>>
>>>>>> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>>>>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>>>>>> ---
>>>>>>  softmmu/cpus.c | 11 ++++++++++-
>>>>>>  1 file changed, 10 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/softmmu/cpus.c b/softmmu/cpus.c
>>>>>> index a802e899ab..e3b98065c9 100644
>>>>>> --- a/softmmu/cpus.c
>>>>>> +++ b/softmmu/cpus.c
>>>>>> @@ -1895,6 +1895,16 @@ static void qemu_tcg_init_vcpu(CPUState *cpu)
>>>>>>      if (!tcg_region_inited) {
>>>>>>          tcg_region_inited = 1;
>>>>>>          tcg_region_init();
>>>>>> +        /*
>>>>>> +         * If MTTCG, and we will create multiple cpus,
>>>>>> +         * then we will have cpus running in parallel.
>>>>>> +         */
>>>>>> +        if (qemu_tcg_mttcg_enabled()) {
>>>>>> +            MachineState *ms = MACHINE(qdev_get_machine());
>>>>>
>>>>> MachineState *ms = current_machine;
>>>>> ?
>>>>>
>>>>>
>>>>>> +            if (ms->smp.max_cpus > 1) {
>>>>>> +                parallel_cpus = true;
>>>>>> +            }
>>>>>> +        }
>>>>>>      }
>>>>>>  
>>>>>>      if (qemu_tcg_mttcg_enabled() || !single_tcg_cpu_thread) {
>>>>>> @@ -1904,7 +1914,6 @@ static void qemu_tcg_init_vcpu(CPUState *cpu)
>>>>>>  
>>>>>>          if (qemu_tcg_mttcg_enabled()) {
>>>>>>              /* create a thread per vCPU with TCG (MTTCG) */
>>>>>> -            parallel_cpus = true;
>>>>>>              snprintf(thread_name, VCPU_THREAD_NAME_SIZE, "CPU %d/TCG",
>>>>>>                   cpu->cpu_index);
>>>>>>  
>>>>>>
>>>>>
>>>>>
>>>
>>>
> 
> 