Re: [Ping][PATCH v0] vl: flush all task from rcu queue before exiting

[Paolo Bonzini <pbonzini@redhat.com>]

On 11/10/21 14:29, Denis Plotnikov wrote:
> 
> On 09.11.2021 20:46, Paolo Bonzini wrote:
>> On 11/9/21 08:23, Denis Plotnikov wrote:
>>> Ping ping!
>>
>> Looks good, but can you explain why it's okay to call it before 
>> qemu_chr_cleanup() and user_creatable_cleanup()?
>>
>> I think a better solution to the ordering problem would be:
>>
>>   qemu_chr_cleanup();
>>   user_creatable_cleanup();
>>   flush_rcu();
>>   monitor_cleanup();
> I agree, this looks better
>>
>> with something like this:
>>
>> diff --git a/chardev/char-fe.c b/chardev/char-fe.c
>> index 7789f7be9c..f0c3ea5447 100644
>> --- a/chardev/char-fe.c
>> +++ b/chardev/char-fe.c
>> @@ -195,6 +195,7 @@ bool qemu_chr_fe_init(CharBackend *b,
>>      int tag = 0;
>>
>>      if (s) {
>> +        object_ref(OBJECT(s));
>>          if (CHARDEV_IS_MUX(s)) {
>>              MuxChardev *d = MUX_CHARDEV(s);
>>
>> @@ -241,6 +242,7 @@ void qemu_chr_fe_deinit(CharBackend *b, bool del)
>>              } else {
>>                  object_unref(obj);
>>              }
>> +            object_unref(obj);
>>          }
>>          b->chr = NULL;
>>      }
>>
>> to keep the chardev live between qemu_chr_cleanup() and 
>> monitor_cleanup().
> 
> but frankly speaking I don't understand why we have to do ref/unref in 
> char-fe interface functions, instead of just ref/uref-ing monitor's char 
> device directly like this:
> 
> diff --git a/monitor/monitor.c b/monitor/monitor.c
> index 21c7a68758f5..3692a8e15268 100644
> --- a/monitor/monitor.c
> +++ b/monitor/monitor.c
> @@ -611,6 +611,7 @@ void monitor_data_destroy(Monitor *mon)
>   {
>       g_free(mon->mon_cpu_path);
>       qemu_chr_fe_deinit(&mon->chr, false);
> +    object_unref(OBJECT(&mon->chr));
>       if (monitor_is_qmp(mon)) {
>           monitor_data_destroy_qmp(container_of(mon, MonitorQMP, common));
>       } else {
> @@ -737,6 +738,7 @@ int monitor_init(MonitorOptions *opts, bool 
> allow_hmp, Error **errp)
>           error_propagate(errp, local_err);
>           return -1;
>       }
> +    object_ref(OBJECT(chr));
>       return 0;
>   }
> 
> May be this shows the intentions better?

Sure, that works too.  But in the end the char-fe _is_ the place where 
the extra reference is taken/dropped (in the ->chr field of 
CharBackend), so I was thinking of a more generic solution too.  Feel 
free to submit yours though, it's certainly safer for 6.2 freeze.

Paolo