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

[Daniel P. Berrangé]

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