[PATCH v1 0/2] vl: flush all task from rcu queue before exiting

[PATCH v1 0/2] vl: flush all task from rcu queue before exiting

[Denis Plotnikov]

v1 -> v0:
 * move monitor cleanup to the very end of qemu cleanup [Paolo]

The goal is to notify management layer about device destruction on qemu shutdown.
Without this series DEVICE_DELETED event may not be sent because of stuck tasks
in the rcu thread. The rcu tasks may stuck on qemu shutdown because the rcu
not always have enough time to run them. 


Denis Plotnikov (2):
  monitor: move monitor destruction to the very end of qemu cleanup
  vl: flush all task from rcu queue before exiting

 include/qemu/rcu.h |  1 +
 monitor/monitor.c  |  6 ++++++
 softmmu/runstate.c |  4 +++-
 util/rcu.c         | 12 ++++++++++++
 4 files changed, 22 insertions(+), 1 deletion(-)

-- 
2.25.1

[PATCH v1 1/2] monitor: move monitor destruction to the very end of qemu cleanup

[Denis Plotnikov]

This is needed to keep sending DEVICE_DELETED events on qemu cleanup.
The event may happen in the rcu thread and we're going to flush the rcu queue
explicitly before qemu exiting in the next patch. So move the monitor
destruction to the very end of qemu cleanup to be able to send all the events.

Signed-off-by: Denis Plotnikov <den-plotnikov@yandex-team.ru>
---
 monitor/monitor.c  | 6 ++++++
 softmmu/runstate.c | 2 +-
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/monitor/monitor.c b/monitor/monitor.c
index 21c7a68758f5..b04ae4850db2 100644
--- a/monitor/monitor.c
+++ b/monitor/monitor.c
@@ -605,11 +605,17 @@ void monitor_data_init(Monitor *mon, bool is_qmp, bool skip_flush,
     mon->outbuf = g_string_new(NULL);
     mon->skip_flush = skip_flush;
     mon->use_io_thread = use_io_thread;
+    /*
+     * take an extra ref to prevent monitor's chardev
+     * from destroying in qemu_chr_cleanup()
+     */
+    object_ref(OBJECT(mon->chr.chr));
 }
 
 void monitor_data_destroy(Monitor *mon)
 {
     g_free(mon->mon_cpu_path);
+    object_unref(OBJECT(mon->chr.chr));
     qemu_chr_fe_deinit(&mon->chr, false);
     if (monitor_is_qmp(mon)) {
         monitor_data_destroy_qmp(container_of(mon, MonitorQMP, common));
diff --git a/softmmu/runstate.c b/softmmu/runstate.c
index 10d9b7365aa7..8d29dd2c00e2 100644
--- a/softmmu/runstate.c
+++ b/softmmu/runstate.c
@@ -819,8 +819,8 @@ void qemu_cleanup(void)
     tpm_cleanup();
     net_cleanup();
     audio_cleanup();
-    monitor_cleanup();
     qemu_chr_cleanup();
     user_creatable_cleanup();
+    monitor_cleanup();
     /* TODO: unref root container, check all devices are ok */
 }
-- 
2.25.1

[PATCH v1 2/2] vl: flush all task from rcu queue before exiting

[Denis Plotnikov]

The device destruction may superimpose over qemu shutdown.
In this case some management layer, requested a device unplug and
waiting for DEVICE_DELETED event, may never get this event.

This happens because device_finalize() may never be called on qemu shutdown
for some devices using address_space_destroy(). The later is called from
the rcu thread.
On qemu shutdown, not all rcu callbacks may be called because the rcu thread
may not have enough time to converge before qemu main thread exit.

To resolve this issue this patch makes rcu thread to finish all its callbacks
explicitly by calling a new rcu intreface function right before
qemu main thread exit.

Signed-off-by: Denis Plotnikov <den-plotnikov@yandex-team.ru>
---
 include/qemu/rcu.h |  1 +
 softmmu/runstate.c |  2 ++
 util/rcu.c         | 12 ++++++++++++
 3 files changed, 15 insertions(+)

diff --git a/include/qemu/rcu.h b/include/qemu/rcu.h
index 515d327cf11c..f7fbdc3781e5 100644
--- a/include/qemu/rcu.h
+++ b/include/qemu/rcu.h
@@ -134,6 +134,7 @@ struct rcu_head {
 
 extern void call_rcu1(struct rcu_head *head, RCUCBFunc *func);
 extern void drain_call_rcu(void);
+extern void flush_rcu(void);
 
 /* The operands of the minus operator must have the same type,
  * which must be the one that we specify in the cast.
diff --git a/softmmu/runstate.c b/softmmu/runstate.c
index 8d29dd2c00e2..3f833678f6eb 100644
--- a/softmmu/runstate.c
+++ b/softmmu/runstate.c
@@ -821,6 +821,8 @@ void qemu_cleanup(void)
     audio_cleanup();
     qemu_chr_cleanup();
     user_creatable_cleanup();
+    /* finish all the tasks from rcu queue before exiting */
+    flush_rcu();
     monitor_cleanup();
     /* TODO: unref root container, check all devices are ok */
 }
diff --git a/util/rcu.c b/util/rcu.c
index 13ac0f75cb2a..f047f8ee8d16 100644
--- a/util/rcu.c
+++ b/util/rcu.c
@@ -348,6 +348,18 @@ void drain_call_rcu(void)
 
 }
 
+/*
+ * This function drains rcu queue until there are no tasks to do left
+ * and aims to the cases when one needs to ensure that no work hang
+ * in rcu thread before proceeding, e.g. on qemu shutdown.
+ */
+void flush_rcu(void)
+{
+    while (qatomic_read(&rcu_call_count) > 0) {
+        drain_call_rcu();
+    }
+}
+
 void rcu_register_thread(void)
 {
     assert(rcu_reader.ctr == 0);
-- 
2.25.1

[Ping] [PATCH v1 0/2] vl: flush all task from rcu queue before exiting

[Denis Plotnikov]

[-- Attachment #1: Type: text/plain, Size: 767 bytes --]

Ping!

On 15.11.2021 12:41, Denis Plotnikov wrote:
> v1 -> v0:
>   * move monitor cleanup to the very end of qemu cleanup [Paolo]
>
> The goal is to notify management layer about device destruction on qemu shutdown.
> Without this series DEVICE_DELETED event may not be sent because of stuck tasks
> in the rcu thread. The rcu tasks may stuck on qemu shutdown because the rcu
> not always have enough time to run them.
>
>
> Denis Plotnikov (2):
>    monitor: move monitor destruction to the very end of qemu cleanup
>    vl: flush all task from rcu queue before exiting
>
>   include/qemu/rcu.h |  1 +
>   monitor/monitor.c  |  6 ++++++
>   softmmu/runstate.c |  4 +++-
>   util/rcu.c         | 12 ++++++++++++
>   4 files changed, 22 insertions(+), 1 deletion(-)
>

[-- Attachment #2: Type: text/html, Size: 1146 bytes --]

[PING][Ping] [PATCH v1 0/2] vl: flush all task from rcu queue before exiting

[Denis Plotnikov]

[-- Attachment #1: Type: text/plain, Size: 848 bytes --]

ping ping

On 19.11.2021 12:42, Denis Plotnikov wrote:
>
> Ping!
>
> On 15.11.2021 12:41, Denis Plotnikov wrote:
>> v1 -> v0:
>>   * move monitor cleanup to the very end of qemu cleanup [Paolo]
>>
>> The goal is to notify management layer about device destruction on qemu shutdown.
>> Without this series DEVICE_DELETED event may not be sent because of stuck tasks
>> in the rcu thread. The rcu tasks may stuck on qemu shutdown because the rcu
>> not always have enough time to run them.
>>
>>
>> Denis Plotnikov (2):
>>    monitor: move monitor destruction to the very end of qemu cleanup
>>    vl: flush all task from rcu queue before exiting
>>
>>   include/qemu/rcu.h |  1 +
>>   monitor/monitor.c  |  6 ++++++
>>   softmmu/runstate.c |  4 +++-
>>   util/rcu.c         | 12 ++++++++++++
>>   4 files changed, 22 insertions(+), 1 deletion(-)
>>

[-- Attachment #2: Type: text/html, Size: 1535 bytes --]

Re: [PATCH v1 1/2] monitor: move monitor destruction to the very end of qemu cleanup

[Markus Armbruster]

Denis Plotnikov <den-plotnikov@yandex-team.ru> writes:

> This is needed to keep sending DEVICE_DELETED events on qemu cleanup.
> The event may happen in the rcu thread and we're going to flush the rcu queue
> explicitly before qemu exiting in the next patch. So move the monitor
> destruction to the very end of qemu cleanup to be able to send all the events.
>
> Signed-off-by: Denis Plotnikov <den-plotnikov@yandex-team.ru>
> ---
>  monitor/monitor.c  | 6 ++++++
>  softmmu/runstate.c | 2 +-
>  2 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/monitor/monitor.c b/monitor/monitor.c
> index 21c7a68758f5..b04ae4850db2 100644
> --- a/monitor/monitor.c
> +++ b/monitor/monitor.c
> @@ -605,11 +605,17 @@ void monitor_data_init(Monitor *mon, bool is_qmp, bool skip_flush,
>      mon->outbuf = g_string_new(NULL);
>      mon->skip_flush = skip_flush;
>      mon->use_io_thread = use_io_thread;
> +    /*
> +     * take an extra ref to prevent monitor's chardev
> +     * from destroying in qemu_chr_cleanup()
> +     */
> +    object_ref(OBJECT(mon->chr.chr));

I'm not sure we need the comment in the long run.

Taking a reference changes mon->chr.chr from soft reference to hard
reference.  Feels right to me.

Note that mon->chr.chr isn't set here, but earlier.  Unlike the other
parts of @mon.  Because of this it starts as a soft reference, and
hardens only here.

It's set in three places:

1. monitor_init_hmp():

    if (!qemu_chr_fe_init(&mon->common.chr, chr, errp)) {
        g_free(mon);
        return;
    }

    monitor_data_init(&mon->common, false, false, false);

2. monitor_init_qmp():

    if (!qemu_chr_fe_init(&mon->common.chr, chr, errp)) {
        g_free(mon);
        return;
    }
    qemu_chr_fe_set_echo(&mon->common.chr, true);

    /* Note: we run QMP monitor in I/O thread when @chr supports that */
    monitor_data_init(&mon->common, true, false,
                      qemu_chr_has_feature(chr, QEMU_CHAR_FEATURE_GCONTEXT));

3. qmp_human_monitor_command()

    MonitorHMP hmp = {};

    monitor_data_init(&hmp.common, false, true, false);

   hmp.common.chr.chr remains null here.  Works, because
   object_ref(OBJECT(NULL)) and object_unref(OBJECT(NULL)) do nothing.

Slightly cleaner, I think: pass the character device as an argument to
monitor_data_init(), take a reference and store it in mon->chr.chr
there.

>  }
>  
>  void monitor_data_destroy(Monitor *mon)
>  {
>      g_free(mon->mon_cpu_path);
> +    object_unref(OBJECT(mon->chr.chr));
>      qemu_chr_fe_deinit(&mon->chr, false);
>      if (monitor_is_qmp(mon)) {
>          monitor_data_destroy_qmp(container_of(mon, MonitorQMP, common));
> diff --git a/softmmu/runstate.c b/softmmu/runstate.c
> index 10d9b7365aa7..8d29dd2c00e2 100644
> --- a/softmmu/runstate.c
> +++ b/softmmu/runstate.c
> @@ -819,8 +819,8 @@ void qemu_cleanup(void)
>      tpm_cleanup();
>      net_cleanup();
>      audio_cleanup();
> -    monitor_cleanup();
>      qemu_chr_cleanup();
>      user_creatable_cleanup();
> +    monitor_cleanup();
>      /* TODO: unref root container, check all devices are ok */
>  }

Monitor cleanup now runs with character devices that have been
"unparented" from the QOM tree.  Paolo, is that okay?