Re: [Ping][PATCH v0] vl: flush all task from rcu queue before exiting
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(>chr, false);
+ object_unref(OBJECT(>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
Re: [Ping][PATCH v0] vl: flush all task from rcu queue before exiting
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(>chr, false);
+ object_unref(OBJECT(>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?
Denis
Paolo
Re: [Ping][PATCH v0] vl: flush all task from rcu queue before exiting
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();
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().
Paolo
