Re: [PULL 3/5] softmmu/cpus: Only set parallel_cpus for SMP

[Claudio Fontana]

Hi Philippe,

On 9/8/20 9:56 AM, Philippe Mathieu-Daudé wrote:
> +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 :)

Yes, I am already testing without problems the cpus-refactoring series with 
this applied:

commit 53f6db772f1522025650441102b16be17773bdc9
Author: Claudio Fontana <cfontana@suse.de>
Date:   Tue Sep 8 10:59:07 2020 +0200

    accel/tcg: use current_machine as it is always set for softmmu
    
    current_machine is always set before accelerators are initialized,
    so use that instead of MACHINE(qdev_get_machine()).
    
    Signed-off-by: Claudio Fontana <cfontana@suse.de>

diff --git a/accel/tcg/tcg-cpus.c b/accel/tcg/tcg-cpus.c
index ec7158b55e..05af1168a2 100644
--- a/accel/tcg/tcg-cpus.c
+++ b/accel/tcg/tcg-cpus.c
@@ -484,7 +484,7 @@ static void tcg_start_vcpu_thread(CPUState *cpu)
          * 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;
             }



> 
> 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.


linux-user/main.c and bsd-user/main.c seem to use cpu_create() to create the 
cpus,
then they have their own cpu_loop(), they do not use any of the cpus.c code.

By contrast, softmmu/vl.c initializes current_machine in qemu_init(),
even before accelerator is chosen and configured.

So there is no chance currently that current_machine is not set correctly when
the accelerator vcpu startup code in is called.

Ciao,

CLaudio




> 
>>
>> 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);
>>>>>>>  
>>>>>>>
>>>>>>
>>>>>>
>>>>
>>>>
>>
>>