Re: [RFC PATCH-for-8.2?] accel/tcg: Implement tcg_unregister_thread()

[Alex Bennée]

Miguel Luis <miguel.luis@oracle.com> writes:

> Hi!
>
> On 04/12/2023 18:40, Philippe Mathieu-Daudé wrote:
>> Unplugging vCPU triggers the following assertion in
>> tcg_register_thread():
>>
>>  796 void tcg_register_thread(void)
>>  797 {
>>  ...
>>  812     /* Claim an entry in tcg_ctxs */
>>  813     n = qatomic_fetch_inc(&tcg_cur_ctxs);
>>  814     g_assert(n < tcg_max_ctxs);
>>
>> Implement and use tcg_unregister_thread() so when a
>> vCPU is unplugged, the tcg_cur_ctxs refcount is
>> decremented.
>
>
> I've had addressed this issue before (posted at [1]) and had exercised
> it with vCPU hotplug/unplug patches for ARM although I was not sure about what
> would be needed to be done regarding plugins on the context of
> tcg_unregister_thread. I guess they would need to be freed also?

Good catch. They should indeed.

>
>
>> Reported-by: Michal Suchánek <msuchanek@suse.de>
>> Suggested-by: Stefan Hajnoczi <stefanha@gmail.com>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
<snip>
>>  void tcg_register_thread(void)
>>  {
>>      TCGContext *s = g_malloc(sizeof(*s));
>> @@ -814,6 +821,16 @@ void tcg_register_thread(void)
>>  
>>      tcg_ctx = s;
>>  }
>> +
>> +void tcg_unregister_thread(void)
>> +{
>> +    unsigned int n;
>> +
>> +    n = qatomic_fetch_dec(&tcg_cur_ctxs);
>> +    g_free(tcg_ctxs[n]);

Perhaps a bit of re-factoring and we could have a tcg_alloc_context and
tcg_free_context to keep everything together?

>
>
> Is the above off-by-one?
>
>
>> +    qatomic_set(&tcg_ctxs[n], NULL);
>> +}
>> +
>
> Thank you
>
> Miguel
>
> [1]: 
> https://lore.kernel.org/qemu-devel/20230926103654.34424-5-salil.mehta@huawei.com/
>
>
>>  #endif /* !CONFIG_USER_ONLY */
>>  
>>  /* pool based memory allocation */

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro