1. Patchew
  2. Libvirt
  3. [libvirt PATCH 0/2] fix regression in SSH tunnelling performance
  4. View patch

[libvirt PATCH 2/2] util: avoid glib event loop workaround where possible

Daniel P. Berrangé posted 2 patches 5 years, 2 months ago
[libvirt PATCH 2/2] util: avoid glib event loop workaround where possible
Posted by Daniel P. Berrangé 5 years, 2 months ago 
I previously did a workaround for a glib event loop race
that causes crashes:

  commit 0db4743645b7a0611a3c0687f834205c9956f7fc
  Author: Daniel P. Berrangé <berrange@redhat.com>
  Date:   Tue Jul 28 16:52:47 2020 +0100

    util: avoid crash due to race in glib event loop code

it turns out that the workaround has a significant performance
penalty on I/O intensive workloads. We thus need to avoid the
workaround if we know we have a new enough glib to avoid the
race condition.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 src/util/vireventglib.c | 29 +++++++++++++++++++++--------
 1 file changed, 21 insertions(+), 8 deletions(-)

diff --git a/src/util/vireventglib.c b/src/util/vireventglib.c
index 6842c6e806..8c5495bfab 100644
--- a/src/util/vireventglib.c
+++ b/src/util/vireventglib.c
@@ -189,9 +189,21 @@ virEventGLibHandleFind(int watch)
  * 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
+ *
+ *    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)
 {
@@ -201,6 +213,7 @@ virEventGLibSourceUnrefIdle(gpointer data)
 
     return FALSE;
 }
+#endif
 
 
 static void
@@ -231,7 +244,7 @@ virEventGLibHandleUpdate(int watch,
         if (data->source != NULL) {
             VIR_DEBUG("Removed old handle source=%p", data->source);
             g_source_destroy(data->source);
-            g_idle_add(virEventGLibSourceUnrefIdle, data->source);
+            g_vir_source_unref_safe(data->source);
         }
 
         data->source = virEventGLibAddSocketWatch(
@@ -245,7 +258,7 @@ virEventGLibHandleUpdate(int watch,
 
         VIR_DEBUG("Removed old handle source=%p", data->source);
         g_source_destroy(data->source);
-        g_idle_add(virEventGLibSourceUnrefIdle, data->source);
+        g_vir_source_unref_safe(data->source);
         data->source = NULL;
         data->events = 0;
     }
@@ -294,7 +307,7 @@ virEventGLibHandleRemove(int watch)
 
     if (data->source != NULL) {
         g_source_destroy(data->source);
-        g_idle_add(virEventGLibSourceUnrefIdle, data->source);
+        g_vir_source_unref_safe(data->source);
         data->source = NULL;
         data->events = 0;
     }
@@ -427,7 +440,7 @@ virEventGLibTimeoutUpdate(int timer,
     if (interval >= 0) {
         if (data->source != NULL) {
             g_source_destroy(data->source);
-            g_idle_add(virEventGLibSourceUnrefIdle, data->source);
+            g_vir_source_unref_safe(data->source);
         }
 
         data->interval = interval;
@@ -437,7 +450,7 @@ virEventGLibTimeoutUpdate(int timer,
             goto cleanup;
 
         g_source_destroy(data->source);
-        g_idle_add(virEventGLibSourceUnrefIdle, data->source);
+        g_vir_source_unref_safe(data->source);
         data->source = NULL;
     }
 
@@ -486,7 +499,7 @@ virEventGLibTimeoutRemove(int timer)
 
     if (data->source != NULL) {
         g_source_destroy(data->source);
-        g_idle_add(virEventGLibSourceUnrefIdle, data->source);
+        g_vir_source_unref_safe(data->source);
         data->source = NULL;
     }
 
-- 
2.25.4
Re: [libvirt PATCH 2/2] util: avoid glib event loop workaround where possible
Posted by Michal Prívozník 5 years, 2 months ago 
On 11/25/20 7:04 PM, Daniel P. Berrangé wrote:
> I previously did a workaround for a glib event loop race
> that causes crashes:
> 
>    commit 0db4743645b7a0611a3c0687f834205c9956f7fc
>    Author: Daniel P. Berrangé <berrange@redhat.com>
>    Date:   Tue Jul 28 16:52:47 2020 +0100
> 
>      util: avoid crash due to race in glib event loop code
> 
> it turns out that the workaround has a significant performance
> penalty on I/O intensive workloads. We thus need to avoid the
> workaround if we know we have a new enough glib to avoid the
> race condition.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>   src/util/vireventglib.c | 29 +++++++++++++++++++++--------
>   1 file changed, 21 insertions(+), 8 deletions(-)
> 
> diff --git a/src/util/vireventglib.c b/src/util/vireventglib.c
> index 6842c6e806..8c5495bfab 100644
> --- a/src/util/vireventglib.c
> +++ b/src/util/vireventglib.c
> @@ -189,9 +189,21 @@ virEventGLibHandleFind(int watch)
>    * 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
> + *
> + *    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);

s/;//

> +

Would something like the following be totally disgusting or only a bit?

#if !GLIB_CHECK_VERSION(2, 64, 0)
# define g_source_unref(source) g_idle_add(virEventGLibSourceUnrefIdle, 
source)
#endif

- g_idle_add(...);
+ g_source_unref(...);


This way we could just drop the redefine once we upgrade min glib version.

Michal
Re: [libvirt PATCH 2/2] util: avoid glib event loop workaround where possible
Posted by Daniel P. Berrangé 5 years, 2 months ago 
On Wed, Nov 25, 2020 at 08:37:02PM +0100, Michal Prívozník wrote:
> On 11/25/20 7:04 PM, Daniel P. Berrangé wrote:
> > I previously did a workaround for a glib event loop race
> > that causes crashes:
> > 
> >    commit 0db4743645b7a0611a3c0687f834205c9956f7fc
> >    Author: Daniel P. Berrangé <berrange@redhat.com>
> >    Date:   Tue Jul 28 16:52:47 2020 +0100
> > 
> >      util: avoid crash due to race in glib event loop code
> > 
> > it turns out that the workaround has a significant performance
> > penalty on I/O intensive workloads. We thus need to avoid the
> > workaround if we know we have a new enough glib to avoid the
> > race condition.
> > 
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> >   src/util/vireventglib.c | 29 +++++++++++++++++++++--------
> >   1 file changed, 21 insertions(+), 8 deletions(-)
> > 
> > diff --git a/src/util/vireventglib.c b/src/util/vireventglib.c
> > index 6842c6e806..8c5495bfab 100644
> > --- a/src/util/vireventglib.c
> > +++ b/src/util/vireventglib.c
> > @@ -189,9 +189,21 @@ virEventGLibHandleFind(int watch)
> >    * 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
> > + *
> > + *    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);
> 
> s/;//
> 
> > +
> 
> Would something like the following be totally disgusting or only a bit?
> 
> #if !GLIB_CHECK_VERSION(2, 64, 0)
> # define g_source_unref(source) g_idle_add(virEventGLibSourceUnrefIdle,
> source)
> #endif
> 
> - g_idle_add(...);
> + g_source_unref(...);
> 
> 
> This way we could just drop the redefine once we upgrade min glib version.

it is possible, but the hack with g_idle_add is semantically different
enough from g_source_unref that I want people to know that something
unusal is going on when reading the code.

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 :|

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.