[glib: 2/3] gthread: Rework to avoid holding a mutex half the time
- From: Philip Withnall <pwithnall src gnome org>
- To: commits-list gnome org
- Cc:
- Subject: [glib: 2/3] gthread: Rework to avoid holding a mutex half the time
- Date: Fri, 1 Feb 2019 00:25:55 +0000 (UTC)
commit 630fa82ed0d8bde0014201df84aeed5fcd489667
Author: Colin Walters <walters verbum org>
Date: Thu Nov 17 17:17:30 2016 -0500
gthread: Rework to avoid holding a mutex half the time
This code was a persistent source of `-fsanitize=thread` errors
when I was trying to use it on OSTree.
The problem is that while I think this code is functionally correct,
we hold a mutex during the writes, but not the reads, and TSAN (IMO
correctly) flags that.
Reading this, I don't see a reason we need a mutex at all. At the
cost of some small code duplication between posix/win32, we can just
pass the data we need down into each implementation. This ends up
being notably cleaner I think than the awkward "lock/unlock to
serialize" dance.
(Minor review changes made by Philip Withnall <withnall endlessm com>.)
https://gitlab.gnome.org/GNOME/glib/issues/1224
glib/gthread-posix.c | 15 +++++++++++++--
glib/gthread-win32.c | 15 +++++++++++++--
glib/gthread.c | 28 ++--------------------------
glib/gthreadprivate.h | 5 ++++-
4 files changed, 32 insertions(+), 31 deletions(-)
---
diff --git a/glib/gthread-posix.c b/glib/gthread-posix.c
index 5fff51477..9c0b032e6 100644
--- a/glib/gthread-posix.c
+++ b/glib/gthread-posix.c
@@ -1148,15 +1148,26 @@ g_system_thread_free (GRealThread *thread)
}
GRealThread *
-g_system_thread_new (GThreadFunc thread_func,
+g_system_thread_new (GThreadFunc proxy,
gulong stack_size,
+ const char *name,
+ GThreadFunc func,
+ gpointer data,
GError **error)
{
GThreadPosix *thread;
+ GRealThread *base_thread;
pthread_attr_t attr;
gint ret;
thread = g_slice_new0 (GThreadPosix);
+ base_thread = (GRealThread*)thread;
+ base_thread->ref_count = 2;
+ base_thread->ours = TRUE;
+ base_thread->thread.joinable = TRUE;
+ base_thread->thread.func = func;
+ base_thread->thread.data = data;
+ base_thread->name = g_strdup (name);
posix_check_cmd (pthread_attr_init (&attr));
@@ -1174,7 +1185,7 @@ g_system_thread_new (GThreadFunc thread_func,
}
#endif /* HAVE_PTHREAD_ATTR_SETSTACKSIZE */
- ret = pthread_create (&thread->system_thread, &attr, (void* (*)(void*))thread_func, thread);
+ ret = pthread_create (&thread->system_thread, &attr, (void* (*)(void*))proxy, thread);
posix_check_cmd (pthread_attr_destroy (&attr));
diff --git a/glib/gthread-win32.c b/glib/gthread-win32.c
index 1ad5ece80..89df0daa0 100644
--- a/glib/gthread-win32.c
+++ b/glib/gthread-win32.c
@@ -429,15 +429,26 @@ g_thread_win32_proxy (gpointer data)
}
GRealThread *
-g_system_thread_new (GThreadFunc func,
+g_system_thread_new (GThreadFunc proxy,
gulong stack_size,
+ const char *name,
+ GThreadFunc func,
+ gpointer data,
GError **error)
{
GThreadWin32 *thread;
+ GRealThread *base_thread;
guint ignore;
thread = g_slice_new0 (GThreadWin32);
- thread->proxy = func;
+ thread->proxy = proxy;
+ base_thread = (GRealThread*)thread;
+ base_thread->ref_count = 2;
+ base_thread->ours = TRUE;
+ base_thread->thread.joinable = TRUE;
+ base_thread->thread.func = func;
+ base_thread->thread.data = data;
+ base_thread->name = g_strdup (name);
thread->handle = (HANDLE) _beginthreadex (NULL, stack_size, g_thread_win32_proxy, thread, 0, &ignore);
diff --git a/glib/gthread.c b/glib/gthread.c
index 2b7be91dd..66e4a1f66 100644
--- a/glib/gthread.c
+++ b/glib/gthread.c
@@ -515,8 +515,6 @@ static GSList *g_once_init_list = NULL;
static void g_thread_cleanup (gpointer data);
static GPrivate g_thread_specific_private = G_PRIVATE_INIT (g_thread_cleanup);
-G_LOCK_DEFINE_STATIC (g_thread_new);
-
/*
* g_private_set_alloc0:
* @key: a #GPrivate
@@ -792,16 +790,8 @@ g_thread_proxy (gpointer data)
GRealThread* thread = data;
g_assert (data);
-
- /* This has to happen before G_LOCK, as that might call g_thread_self */
g_private_set (&g_thread_specific_private, data);
- /* The lock makes sure that g_thread_new_internal() has a chance to
- * setup 'func' and 'data' before we make the call.
- */
- G_LOCK (g_thread_new);
- G_UNLOCK (g_thread_new);
-
TRACE (GLIB_THREAD_SPAWNED (thread->thread.func, thread->thread.data,
thread->name));
@@ -897,24 +887,10 @@ g_thread_new_internal (const gchar *name,
gsize stack_size,
GError **error)
{
- GRealThread *thread;
-
g_return_val_if_fail (func != NULL, NULL);
- G_LOCK (g_thread_new);
- thread = g_system_thread_new (proxy, stack_size, error);
- if (thread)
- {
- thread->ref_count = 2;
- thread->ours = TRUE;
- thread->thread.joinable = TRUE;
- thread->thread.func = func;
- thread->thread.data = data;
- thread->name = g_strdup (name);
- }
- G_UNLOCK (g_thread_new);
-
- return (GThread*) thread;
+ return (GThread*) g_system_thread_new (proxy, stack_size, name,
+ func, data, error);
}
/**
diff --git a/glib/gthreadprivate.h b/glib/gthreadprivate.h
index 75bb3cbf2..b0294af4c 100644
--- a/glib/gthreadprivate.h
+++ b/glib/gthreadprivate.h
@@ -37,8 +37,11 @@ struct _GRealThread
/* system thread implementation (gthread-posix.c, gthread-win32.c) */
void g_system_thread_wait (GRealThread *thread);
-GRealThread * g_system_thread_new (GThreadFunc func,
+GRealThread * g_system_thread_new (GThreadFunc proxy,
gulong stack_size,
+ const char *name,
+ GThreadFunc func,
+ gpointer data,
GError **error);
void g_system_thread_free (GRealThread *thread);
[
Date Prev][
Date Next] [
Thread Prev][
Thread Next]
[
Thread Index]
[
Date Index]
[
Author Index]