3:48 a.m.
Similarly to commit 0db4743645b7 we need to fix the other event loop as well.
To be completely honest I do not completely understand all the details in the
original workaround, so I just hope this is the best way to fix the issue.
Hopefully-resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1931331
Martin Kletzander (2):
util: Move glib event loop workaround to glibcompat
glib: Use safe glib event workaround in other event loops
src/qemu/qemu_agent.c | 2 +-
src/qemu/qemu_monitor.c | 2 +-
src/util/glibcompat.c | 35 +++++++++++++++++++++++++++++++++++
src/util/glibcompat.h | 11 +++++++++++
src/util/vireventglib.c | 30 ------------------------------
src/util/vireventthread.c | 2 +-
6 files changed, 49 insertions(+), 33 deletions(-)
--
2.30.1
3:48 a.m.
New subject: [libvirt PATCH 1/2] util: Move glib event loop workaround to glibcompat
This way it can be used from other places as well.
Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com>
---
src/util/glibcompat.c | 35 +++++++++++++++++++++++++++++++++++
src/util/glibcompat.h | 11 +++++++++++
src/util/vireventglib.c | 30 ------------------------------
3 files changed, 46 insertions(+), 30 deletions(-)
diff --git a/src/util/glibcompat.c b/src/util/glibcompat.c
index 9f0f7f015cad..76fda18a62d8 100644
--- a/src/util/glibcompat.c
+++ b/src/util/glibcompat.c
@@ -211,3 +211,38 @@ vir_g_strdup_vprintf(const char *msg, va_list args)
abort();
return ret;
}
+
+
+/*
+ * If the last reference to a GSource is released in a non-main
+ * thread we're exposed to a race condition that causes a
+ * crash:
+ *
+ * https://gitlab.gnome.org/GNOME/glib/-/merge_requests/1358
+ *
+ * Thus we're using an idle func to release our ref...
+ *
+ * ...but this imposes a significant performance penalty on
+ * I/O intensive workloads which are sensitive to the iterations
+ * of the event loop, so avoid the workaround if we know we have
+ * new enough glib.
+ *
+ * The function below is used from a header file definition.
+ *
+ * Drop when min glib >= 2.64.0
+ */
+#if GLIB_CHECK_VERSION(2, 64, 0)
+# define g_vir_source_unref_safe(source) g_source_unref(source)
+#else
+# define g_vir_source_unref_safe(source) g_idle_add(virEventGLibSourceUnrefIdle, source)
+
+gboolean
+virEventGLibSourceUnrefIdle(gpointer data)
+{
+ GSource *src = data;
+
+ g_source_unref(src);
+
+ return FALSE;
+}
+#endif
diff --git a/src/util/glibcompat.h b/src/util/glibcompat.h
index 6463c4179a7e..9c528432742b 100644
--- a/src/util/glibcompat.h
+++ b/src/util/glibcompat.h
@@ -84,3 +84,14 @@ char *vir_g_strdup_vprintf(const char *msg, va_list args)
#define g_canonicalize_filename vir_g_canonicalize_filename
#undef g_fsync
#define g_fsync vir_g_fsync
+
+/* Drop when min glib >= 2.64.0 */
+#if GLIB_CHECK_VERSION(2, 64, 0)
+# define g_vir_source_unref_safe(source) g_source_unref(source)
+#else
+# define g_vir_source_unref_safe(source) g_idle_add(virEventGLibSourceUnrefIdle, source)
+
+gboolean
+virEventGLibSourceUnrefIdle(gpointer data);
+
+#endif
diff --git a/src/util/vireventglib.c b/src/util/vireventglib.c
index 193b3bfde240..88e3ec6d5dbb 100644
--- a/src/util/vireventglib.c
+++ b/src/util/vireventglib.c
@@ -185,36 +185,6 @@ virEventGLibHandleFind(int watch)
return NULL;
}
-/*
- * If the last reference to a GSource is released in a non-main
- * thread we're exposed to a race condition that causes a
- * crash:
- *
- * https://gitlab.gnome.org/GNOME/glib/-/merge_requests/1358
- *
- * Thus we're using an idle func to release our ref...
- *
- * ...but this imposes a significant performance penalty on
- * I/O intensive workloads which are sensitive to the iterations
- * of the event loop, so avoid the workaround if we know we have
- * new enough glib.
- */
-#if GLIB_CHECK_VERSION(2, 64, 0)
-# define g_vir_source_unref_safe(source) g_source_unref(source)
-#else
-# define g_vir_source_unref_safe(source) g_idle_add(virEventGLibSourceUnrefIdle, source)
-
-static gboolean
-virEventGLibSourceUnrefIdle(gpointer data)
-{
- GSource *src = data;
-
- g_source_unref(src);
-
- return FALSE;
-}
-#endif
-
static void
virEventGLibHandleUpdate(int watch,
--
2.30.1
4:20 a.m.
New subject: [libvirt PATCH 1/2] util: Move glib event loop workaround to glibcompat
On Thu, Mar 04, 2021 at 10:48:10AM +0100, Martin Kletzander wrote:
This way it can be used from other places as well.
Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com>
---
src/util/glibcompat.c | 35 +++++++++++++++++++++++++++++++++++
src/util/glibcompat.h | 11 +++++++++++
src/util/vireventglib.c | 30 ------------------------------
3 files changed, 46 insertions(+), 30 deletions(-)
9rev
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
5:11 a.m.
New subject: [libvirt PATCH 1/2] util: Move glib event loop workaround to glibcompat
On Thu, Mar 04, 2021 at 10:20:14AM +0000, Daniel P. Berrangé wrote:
On Thu, Mar 04, 2021 at 10:48:10AM +0100, Martin Kletzander wrote:
> This way it can be used from other places as well.
>
> Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com>
> ---
> src/util/glibcompat.c | 35 +++++++++++++++++++++++++++++++++++
> src/util/glibcompat.h | 11 +++++++++++
> src/util/vireventglib.c | 30 ------------------------------
> 3 files changed, 46 insertions(+), 30 deletions(-)
9rev
which should be
Reviewed-by: Daniel P. Berrangé <berrange(a)redhat.com>
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
3:48 a.m.
New subject: [libvirt PATCH 2/2] glib: Use safe glib event workaround in other event loops
Similarly to the crash workaround:
commit 0db4743645b7a0611a3c0687f834205c9956f7fc
Author: Daniel P. Berrangé <berrange(a)redhat.com>
Date: Tue Jul 28 16:52:47 2020 +0100
util: avoid crash due to race in glib event loop code
we need to do this in the other event loop as crash in that one was also
reported:
https://bugzilla.redhat.com/show_bug.cgi?id=1931331
Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com>
---
src/qemu/qemu_agent.c | 2 +-
src/qemu/qemu_monitor.c | 2 +-
src/util/vireventthread.c | 2 +-
3 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c
index 9a74b802b89d..8b880d0d157c 100644
--- a/src/qemu/qemu_agent.c
+++ b/src/qemu/qemu_agent.c
@@ -533,7 +533,7 @@ qemuAgentUnregister(qemuAgentPtr agent)
{
if (agent->watch) {
g_source_destroy(agent->watch);
- g_source_unref(agent->watch);
+ g_vir_source_unref_safe(agent->watch);
agent->watch = NULL;
}
}
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index 73f337a6be53..b4f2641504f8 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -862,7 +862,7 @@ qemuMonitorUnregister(qemuMonitorPtr mon)
{
if (mon->watch) {
g_source_destroy(mon->watch);
- g_source_unref(mon->watch);
+ g_vir_source_unref_safe(mon->watch);
mon->watch = NULL;
}
}
diff --git a/src/util/vireventthread.c b/src/util/vireventthread.c
index 8342f420f666..e9d18d3acf2f 100644
--- a/src/util/vireventthread.c
+++ b/src/util/vireventthread.c
@@ -123,7 +123,7 @@ virEventThreadWorker(void *opaque)
g_main_loop_run(data->loop);
- g_source_unref(running);
+ g_vir_source_unref_safe(running);
virEventThreadDataFree(data);
return NULL;
--
2.30.1
4:19 a.m.
New subject: [libvirt PATCH 2/2] glib: Use safe glib event workaround in other event loops
On Thu, Mar 04, 2021 at 10:48:11AM +0100, Martin Kletzander wrote:
Similarly to the crash workaround:
commit 0db4743645b7a0611a3c0687f834205c9956f7fc
Author: Daniel P. Berrangé <berrange(a)redhat.com>
Date: Tue Jul 28 16:52:47 2020 +0100
util: avoid crash due to race in glib event loop code
we need to do this in the other event loop as crash in that one was also
reported:
https://bugzilla.redhat.com/show_bug.cgi?id=1931331
Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com>
---
src/qemu/qemu_agent.c | 2 +-
src/qemu/qemu_monitor.c | 2 +-
src/util/vireventthread.c | 2 +-
3 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c
index 9a74b802b89d..8b880d0d157c 100644
--- a/src/qemu/qemu_agent.c
+++ b/src/qemu/qemu_agent.c
@@ -533,7 +533,7 @@ qemuAgentUnregister(qemuAgentPtr agent)
{
if (agent->watch) {
g_source_destroy(agent->watch);
- g_source_unref(agent->watch);
+ g_vir_source_unref_safe(agent->watch);
agent->watch = NULL;
}
}
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index 73f337a6be53..b4f2641504f8 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -862,7 +862,7 @@ qemuMonitorUnregister(qemuMonitorPtr mon)
{
if (mon->watch) {
g_source_destroy(mon->watch);
- g_source_unref(mon->watch);
+ g_vir_source_unref_safe(mon->watch);
mon->watch = NULL;
}
}
Upto here:
Reviewed-by: Daniel P. Berrangé <berrange(a)redhat.com>
diff --git a/src/util/vireventthread.c b/src/util/vireventthread.c
index 8342f420f666..e9d18d3acf2f 100644
--- a/src/util/vireventthread.c
+++ b/src/util/vireventthread.c
@@ -123,7 +123,7 @@ virEventThreadWorker(void *opaque)
g_main_loop_run(data->loop);
- g_source_unref(running);
+ g_vir_source_unref_safe(running);
This one isn't desirable as it'll cause a memory leak. This method is the
very thread that was running the event loop, so there's no possibility
of a race condition. Also the loop has stopped running at this point,
so the idle source will never run. Just drop this hunk
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
1359
days inactive
1359
days old
5 comments
2 participants
participants (2)
-
Daniel P. Berrangé -
Martin Kletzander