[PATCH v3 57/68] accel/tcg: Expose vcpu_[un]register() for MTTCG

Philippe Mathieu-Daudé posted 68 patches 7 months, 1 week ago
Maintainers: Richard Henderson <richard.henderson@linaro.org>, Paolo Bonzini <pbonzini@redhat.com>, Cameron Esfahani <dirty@apple.com>, Roman Bolshakov <rbolshakov@ddn.com>, Phil Dennis-Jordan <phil@philjordan.eu>, Fabiano Rosas <farosas@suse.de>, Laurent Vivier <lvivier@redhat.com>, Stefano Stabellini <sstabellini@kernel.org>, Anthony PERARD <anthony@xenproject.org>, Paul Durrant <paul@xen.org>, "Edgar E. Iglesias" <edgar.iglesias@gmail.com>, Warner Losh <imp@bsdimp.com>, Kyle Evans <kevans@freebsd.org>, "Alex Bennée" <alex.bennee@linaro.org>, "Philippe Mathieu-Daudé" <philmd@linaro.org>, "Dr. David Alan Gilbert" <dave@treblig.org>, Eduardo Habkost <eduardo@habkost.net>, Marcel Apfelbaum <marcel.apfelbaum@gmail.com>, Yanan Wang <wangyanan55@huawei.com>, Zhao Liu <zhao1.liu@intel.com>, Reinoud Zandijk <reinoud@netbsd.org>, Sunil Muthuswamy <sunilmut@microsoft.com>, Eric Blake <eblake@redhat.com>, Markus Armbruster <armbru@redhat.com>, Peter Xu <peterx@redhat.com>, David Hildenbrand <david@redhat.com>, Peter Maydell <peter.maydell@linaro.org>, Alexander Graf <agraf@csgraf.de>
There is a newer version of this series
[PATCH v3 57/68] accel/tcg: Expose vcpu_[un]register() for MTTCG
Posted by Philippe Mathieu-Daudé 7 months, 1 week ago
Allocate ForceRcuNotifier on the Heap.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 accel/tcg/tcg-accel-ops-mttcg.c | 30 ++++++++++++++++++++++++------
 1 file changed, 24 insertions(+), 6 deletions(-)

diff --git a/accel/tcg/tcg-accel-ops-mttcg.c b/accel/tcg/tcg-accel-ops-mttcg.c
index 4de506a80ca..2d31b00ee59 100644
--- a/accel/tcg/tcg-accel-ops-mttcg.c
+++ b/accel/tcg/tcg-accel-ops-mttcg.c
@@ -55,6 +55,27 @@ static void mttcg_force_rcu(Notifier *notify, void *data)
     async_run_on_cpu(cpu, do_nothing, RUN_ON_CPU_NULL);
 }
 
+static void *mttcg_vcpu_register(CPUState *cpu)
+{
+    MttcgForceRcuNotifier *force_rcu = g_new(MttcgForceRcuNotifier, 1);
+
+    force_rcu->notifier.notify = mttcg_force_rcu;
+    force_rcu->cpu = cpu;
+    rcu_add_force_rcu_notifier(&force_rcu->notifier);
+    tcg_register_thread();
+
+    return force_rcu;
+}
+
+static void mttcg_vcpu_unregister(CPUState *cpu, void *opaque)
+{
+    MttcgForceRcuNotifier *force_rcu = opaque;
+
+    rcu_remove_force_rcu_notifier(&force_rcu->notifier);
+
+    g_free(force_rcu);
+}
+
 /*
  * In the multi-threaded case each vCPU has its own thread. The TLS
  * variable current_cpu can be used deep in the code to find the
@@ -63,17 +84,14 @@ static void mttcg_force_rcu(Notifier *notify, void *data)
 
 void *mttcg_cpu_thread_routine(void *arg)
 {
-    MttcgForceRcuNotifier force_rcu;
+    MttcgForceRcuNotifier *force_rcu;
     CPUState *cpu = arg;
 
     assert(tcg_enabled());
     g_assert(!icount_enabled());
 
     rcu_register_thread();
-    force_rcu.notifier.notify = mttcg_force_rcu;
-    force_rcu.cpu = cpu;
-    rcu_add_force_rcu_notifier(&force_rcu.notifier);
-    tcg_register_thread();
+    force_rcu = mttcg_vcpu_register(cpu);
 
     bql_lock();
     qemu_thread_get_self(cpu->thread);
@@ -121,7 +139,7 @@ void *mttcg_cpu_thread_routine(void *arg)
 
     tcg_cpu_destroy(cpu);
     bql_unlock();
-    rcu_remove_force_rcu_notifier(&force_rcu.notifier);
+    mttcg_vcpu_unregister(cpu, force_rcu);
     rcu_unregister_thread();
     return NULL;
 }
-- 
2.49.0


Re: [PATCH v3 57/68] accel/tcg: Expose vcpu_[un]register() for MTTCG
Posted by Richard Henderson 7 months, 1 week ago
On 7/1/25 08:40, Philippe Mathieu-Daudé wrote:
> Allocate ForceRcuNotifier on the Heap.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   accel/tcg/tcg-accel-ops-mttcg.c | 30 ++++++++++++++++++++++++------
>   1 file changed, 24 insertions(+), 6 deletions(-)

Please document the motivation.

r~

> 
> diff --git a/accel/tcg/tcg-accel-ops-mttcg.c b/accel/tcg/tcg-accel-ops-mttcg.c
> index 4de506a80ca..2d31b00ee59 100644
> --- a/accel/tcg/tcg-accel-ops-mttcg.c
> +++ b/accel/tcg/tcg-accel-ops-mttcg.c
> @@ -55,6 +55,27 @@ static void mttcg_force_rcu(Notifier *notify, void *data)
>       async_run_on_cpu(cpu, do_nothing, RUN_ON_CPU_NULL);
>   }
>   
> +static void *mttcg_vcpu_register(CPUState *cpu)
> +{
> +    MttcgForceRcuNotifier *force_rcu = g_new(MttcgForceRcuNotifier, 1);
> +
> +    force_rcu->notifier.notify = mttcg_force_rcu;
> +    force_rcu->cpu = cpu;
> +    rcu_add_force_rcu_notifier(&force_rcu->notifier);
> +    tcg_register_thread();
> +
> +    return force_rcu;
> +}
> +
> +static void mttcg_vcpu_unregister(CPUState *cpu, void *opaque)
> +{
> +    MttcgForceRcuNotifier *force_rcu = opaque;
> +
> +    rcu_remove_force_rcu_notifier(&force_rcu->notifier);
> +
> +    g_free(force_rcu);
> +}
> +
>   /*
>    * In the multi-threaded case each vCPU has its own thread. The TLS
>    * variable current_cpu can be used deep in the code to find the
> @@ -63,17 +84,14 @@ static void mttcg_force_rcu(Notifier *notify, void *data)
>   
>   void *mttcg_cpu_thread_routine(void *arg)
>   {
> -    MttcgForceRcuNotifier force_rcu;
> +    MttcgForceRcuNotifier *force_rcu;
>       CPUState *cpu = arg;
>   
>       assert(tcg_enabled());
>       g_assert(!icount_enabled());
>   
>       rcu_register_thread();
> -    force_rcu.notifier.notify = mttcg_force_rcu;
> -    force_rcu.cpu = cpu;
> -    rcu_add_force_rcu_notifier(&force_rcu.notifier);
> -    tcg_register_thread();
> +    force_rcu = mttcg_vcpu_register(cpu);
>   
>       bql_lock();
>       qemu_thread_get_self(cpu->thread);
> @@ -121,7 +139,7 @@ void *mttcg_cpu_thread_routine(void *arg)
>   
>       tcg_cpu_destroy(cpu);
>       bql_unlock();
> -    rcu_remove_force_rcu_notifier(&force_rcu.notifier);
> +    mttcg_vcpu_unregister(cpu, force_rcu);
>       rcu_unregister_thread();
>       return NULL;
>   }


Re: [PATCH v3 57/68] accel/tcg: Expose vcpu_[un]register() for MTTCG
Posted by Philippe Mathieu-Daudé 7 months, 1 week ago
On 2/7/25 17:19, Richard Henderson wrote:
> On 7/1/25 08:40, Philippe Mathieu-Daudé wrote:
>> Allocate ForceRcuNotifier on the Heap.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   accel/tcg/tcg-accel-ops-mttcg.c | 30 ++++++++++++++++++++++++------
>>   1 file changed, 24 insertions(+), 6 deletions(-)
> 
> Please document the motivation.

 > [...] the structure is still not accessible from anywhere outside
 > of the function, and has the same lifetime as the function.

We need this to register the MTTCG thread in split_cpu_thread_routine():
https://lore.kernel.org/qemu-devel/20250620172751.94231-12-philmd@linaro.org/

Better to have AccelOpsClass::[un]register_thread_rcu() hooks?

>>
>> diff --git a/accel/tcg/tcg-accel-ops-mttcg.c b/accel/tcg/tcg-accel- 
>> ops-mttcg.c
>> index 4de506a80ca..2d31b00ee59 100644
>> --- a/accel/tcg/tcg-accel-ops-mttcg.c
>> +++ b/accel/tcg/tcg-accel-ops-mttcg.c
>> @@ -55,6 +55,27 @@ static void mttcg_force_rcu(Notifier *notify, void 
>> *data)
>>       async_run_on_cpu(cpu, do_nothing, RUN_ON_CPU_NULL);
>>   }
>> +static void *mttcg_vcpu_register(CPUState *cpu)
>> +{
>> +    MttcgForceRcuNotifier *force_rcu = g_new(MttcgForceRcuNotifier, 1);
>> +
>> +    force_rcu->notifier.notify = mttcg_force_rcu;
>> +    force_rcu->cpu = cpu;
>> +    rcu_add_force_rcu_notifier(&force_rcu->notifier);
>> +    tcg_register_thread();
>> +
>> +    return force_rcu;
>> +}
>> +
>> +static void mttcg_vcpu_unregister(CPUState *cpu, void *opaque)
>> +{
>> +    MttcgForceRcuNotifier *force_rcu = opaque;
>> +
>> +    rcu_remove_force_rcu_notifier(&force_rcu->notifier);
>> +
>> +    g_free(force_rcu);
>> +}
>> +
>>   /*
>>    * In the multi-threaded case each vCPU has its own thread. The TLS
>>    * variable current_cpu can be used deep in the code to find the
>> @@ -63,17 +84,14 @@ static void mttcg_force_rcu(Notifier *notify, void 
>> *data)
>>   void *mttcg_cpu_thread_routine(void *arg)
>>   {
>> -    MttcgForceRcuNotifier force_rcu;
>> +    MttcgForceRcuNotifier *force_rcu;
>>       CPUState *cpu = arg;
>>       assert(tcg_enabled());
>>       g_assert(!icount_enabled());
>>       rcu_register_thread();
>> -    force_rcu.notifier.notify = mttcg_force_rcu;
>> -    force_rcu.cpu = cpu;
>> -    rcu_add_force_rcu_notifier(&force_rcu.notifier);
>> -    tcg_register_thread();
>> +    force_rcu = mttcg_vcpu_register(cpu);
>>       bql_lock();
>>       qemu_thread_get_self(cpu->thread);
>> @@ -121,7 +139,7 @@ void *mttcg_cpu_thread_routine(void *arg)
>>       tcg_cpu_destroy(cpu);
>>       bql_unlock();
>> -    rcu_remove_force_rcu_notifier(&force_rcu.notifier);
>> +    mttcg_vcpu_unregister(cpu, force_rcu);
>>       rcu_unregister_thread();
>>       return NULL;
>>   }
> 


Re: [PATCH v3 57/68] accel/tcg: Expose vcpu_[un]register() for MTTCG
Posted by Richard Henderson 7 months, 1 week ago
On 7/2/25 12:25, Philippe Mathieu-Daudé wrote:
> On 2/7/25 17:19, Richard Henderson wrote:
>> On 7/1/25 08:40, Philippe Mathieu-Daudé wrote:
>>> Allocate ForceRcuNotifier on the Heap.
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>> ---
>>>   accel/tcg/tcg-accel-ops-mttcg.c | 30 ++++++++++++++++++++++++------
>>>   1 file changed, 24 insertions(+), 6 deletions(-)
>>
>> Please document the motivation.
> 
>  > [...] the structure is still not accessible from anywhere outside
>  > of the function, and has the same lifetime as the function.
> 
> We need this to register the MTTCG thread in split_cpu_thread_routine():
> https://lore.kernel.org/qemu-devel/20250620172751.94231-12-philmd@linaro.org/
> 
> Better to have AccelOpsClass::[un]register_thread_rcu() hooks?

I think this is more complex than it needs to be.  I think we don't actually need 
different implementations between mttcg and rr: we just need to make use of current_cpu.

I'll experiment with this.


r~