[Qemu-devel] [PATCH for-3.1 0/2] Fix qemu_thread_atexit* for OSX

[Qemu-devel] [PATCH for-3.1 0/2] Fix qemu_thread_atexit* for OSX

[Peter Maydell]

Our current implementation of qemu_thread_atexit* is broken on OSX.
This is because it works by cerating a piece of thread-specific
data with pthread_key_create() and using the destructor function
for that data to run the notifier function passed to it by
the caller of qemu_thread_atexit_add(). The expected use case
is that the caller uses a __thread variable as the notifier,
and uses the callback to clean up information that it is
keeping per-thread in __thread variables.

Unfortunately, on OSX this does not work, because on OSX
a __thread variable may be destroyed (freed) before the
pthread_key_create() destructor runs. (POSIX imposes no
ordering constraint here; the OSX implementation happens
to implement __thread variables in terms of pthread_key_create((),
whereas Linux uses different mechanisms that mean the __thread
variables will still be present when the pthread_key_create()
destructor is run.)

Fix this by switching to a scheme similar to the one qemu-thread-win32
uses for qemu_thread_atexit: keep the thread's notifiers on a
__thread variable, and run the notifiers on calls to
qemu_thread_exit() and on return from the start routine passed
to qemu_thread_start(). We do this with the pthread_cleanup_push()
API. (This approach was suggested by Paolo.)

There is a slightly awkward corner here for the main thread,
which isn't started via qemu_thread_start() and so can exit
without passing through the cleanup handler. qemu-thread-win32.c
tries to handle the main thread using an atexit() handler to
run its notifiers. I don't think this will work because the atexit
handler may not run on the main thread, in which case notify
callback functions which refer to __thread variables will get
the wrong ones. So instead I have documented that the API
leaves it unspecified whether notifiers are run for thread
exits that happen when the entire process is exiting. This
is OK for our current users (who only use the callbacks to
clean up memory or win32 Fibers which are deleted on process
exit anyway), and means we don't need to worry about running
callbacks on main thread exit (which always causes the whole
process to exit).

In particular, this fixes the ASAN errors and lockups we
currently see on OSX hosts when running test-bdrv-drain.

thanks
-- PMM

Peter Maydell (2):
  include/qemu/thread.h: Document qemu_thread_atexit* API
  util/qemu-thread-posix: Fix qemu_thread_atexit* for OSX

 include/qemu/thread.h    | 22 ++++++++++++++++++++
 util/qemu-thread-posix.c | 44 ++++++++++++++++++----------------------
 2 files changed, 42 insertions(+), 24 deletions(-)

-- 
2.19.1

[Qemu-devel] [PATCH for-3.1 1/2] include/qemu/thread.h: Document qemu_thread_atexit* API

[Peter Maydell]

Add documentation for the qemu_thread_atexit_add() and
qemu_thread_atexit_remove() functions.

We include a (previously undocumented) constraint that notifiers
may not be called if a thread is exiting because the entire
process is exiting. This is fine for our current use because
the callers use it only for cleaning up resources which go away
on process exit (memory, Win32 fibers), and we will need the
flexibility for the new posix implementation.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/qemu/thread.h | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/include/qemu/thread.h b/include/qemu/thread.h
index b2661b66720..55d83a907cd 100644
--- a/include/qemu/thread.h
+++ b/include/qemu/thread.h
@@ -162,7 +162,29 @@ void qemu_thread_exit(void *retval);
 void qemu_thread_naming(bool enable);
 
 struct Notifier;
+/**
+ * qemu_thread_atexit_add:
+ * @notifier: Notifier to add
+ *
+ * Add the specified notifier to a list which will be run via
+ * notifier_list_notify() when this thread exits (either by calling
+ * qemu_thread_exit() or by returning from its start_routine).
+ * The usual usage is that the caller passes a Notifier which is
+ * a per-thread variable; it can then use the callback to free
+ * other per-thread data.
+ *
+ * If the thread exits as part of the entire process exiting,
+ * it is unspecified whether notifiers are called or not.
+ */
 void qemu_thread_atexit_add(struct Notifier *notifier);
+/**
+ * qemu_thread_atexit_remove:
+ * @notifier: Notifier to remove
+ *
+ * Remove the specified notifier from the thread-exit notification
+ * list. It is not valid to try to remove a notifier which is not
+ * on the list.
+ */
 void qemu_thread_atexit_remove(struct Notifier *notifier);
 
 struct QemuSpin {
-- 
2.19.1

[Qemu-devel] [PATCH for-3.1 2/2] util/qemu-thread-posix: Fix qemu_thread_atexit* for OSX

[Peter Maydell]

Our current implementation of qemu_thread_atexit* is broken on OSX.
This is because it works by cerating a piece of thread-specific
data with pthread_key_create() and using the destructor function
for that data to run the notifier function passed to it by
the caller of qemu_thread_atexit_add(). The expected use case
is that the caller uses a __thread variable as the notifier,
and uses the callback to clean up information that it is
keeping per-thread in __thread variables.

Unfortunately, on OSX this does not work, because on OSX
a __thread variable may be destroyed (freed) before the
pthread_key_create() destructor runs. (POSIX imposes no
ordering constraint here; the OSX implementation happens
to implement __thread variables in terms of pthread_key_create((),
whereas Linux uses different mechanisms that mean the __thread
variables will still be present when the pthread_key_create()
destructor is run.)

Fix this by switching to a scheme similar to the one qemu-thread-win32
uses for qemu_thread_atexit: keep the thread's notifiers on a
__thread variable, and run the notifiers on calls to
qemu_thread_exit() and on return from the start routine passed
to qemu_thread_start(). We do this with the pthread_cleanup_push()
API.

We take advantage of the qemu_thread_atexit_add() API
permission not to run thread notifiers on process exit to
avoid having to special case the main thread.

Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
qemu-thread-win32.c tries to handle the "main thread" case
by using an atexit() handler to run its notifiers. I don't
think this will work because the atexit handler may not run
on the main thread, in which case notify callback functions
which refer to __thread variables will get the wrong ones.
---
 util/qemu-thread-posix.c | 44 ++++++++++++++++++----------------------
 1 file changed, 20 insertions(+), 24 deletions(-)

diff --git a/util/qemu-thread-posix.c b/util/qemu-thread-posix.c
index dfa66ff2fbf..865e476df5b 100644
--- a/util/qemu-thread-posix.c
+++ b/util/qemu-thread-posix.c
@@ -443,42 +443,34 @@ void qemu_event_wait(QemuEvent *ev)
     }
 }
 
-static pthread_key_t exit_key;
-
-union NotifierThreadData {
-    void *ptr;
-    NotifierList list;
-};
-QEMU_BUILD_BUG_ON(sizeof(union NotifierThreadData) != sizeof(void *));
+static __thread NotifierList thread_exit;
 
+/*
+ * Note that in this implementation you can register a thread-exit
+ * notifier for the main thread, but it will never be called.
+ * This is OK because main thread exit can only happen when the
+ * entire process is exiting, and the API allows notifiers to not
+ * be called on process exit.
+ */
 void qemu_thread_atexit_add(Notifier *notifier)
 {
-    union NotifierThreadData ntd;
-    ntd.ptr = pthread_getspecific(exit_key);
-    notifier_list_add(&ntd.list, notifier);
-    pthread_setspecific(exit_key, ntd.ptr);
+    notifier_list_add(&thread_exit, notifier);
 }
 
 void qemu_thread_atexit_remove(Notifier *notifier)
 {
-    union NotifierThreadData ntd;
-    ntd.ptr = pthread_getspecific(exit_key);
     notifier_remove(notifier);
-    pthread_setspecific(exit_key, ntd.ptr);
 }
 
-static void qemu_thread_atexit_run(void *arg)
+static void qemu_thread_atexit_notify(void *arg)
 {
-    union NotifierThreadData ntd = { .ptr = arg };
-    notifier_list_notify(&ntd.list, NULL);
+    /*
+     * Called when non-main thread exits (via qemu_thread_exit()
+     * or by returning from its start routine.)
+     */
+    notifier_list_notify(&thread_exit, NULL);
 }
 
-static void __attribute__((constructor)) qemu_thread_atexit_init(void)
-{
-    pthread_key_create(&exit_key, qemu_thread_atexit_run);
-}
-
-
 typedef struct {
     void *(*start_routine)(void *);
     void *arg;
@@ -490,6 +482,7 @@ static void *qemu_thread_start(void *args)
     QemuThreadArgs *qemu_thread_args = args;
     void *(*start_routine)(void *) = qemu_thread_args->start_routine;
     void *arg = qemu_thread_args->arg;
+    void *r;
 
 #ifdef CONFIG_PTHREAD_SETNAME_NP
     /* Attempt to set the threads name; note that this is for debug, so
@@ -501,7 +494,10 @@ static void *qemu_thread_start(void *args)
 #endif
     g_free(qemu_thread_args->name);
     g_free(qemu_thread_args);
-    return start_routine(arg);
+    pthread_cleanup_push(qemu_thread_atexit_notify, NULL);
+    r = start_routine(arg);
+    pthread_cleanup_pop(1);
+    return r;
 }
 
 void qemu_thread_create(QemuThread *thread, const char *name,
-- 
2.19.1

Re: [Qemu-devel] [PATCH for-3.1 1/2] include/qemu/thread.h: Document qemu_thread_atexit* API

[Eric Blake]

On 11/5/18 7:55 AM, Peter Maydell wrote:
> Add documentation for the qemu_thread_atexit_add() and
> qemu_thread_atexit_remove() functions.
> 
> We include a (previously undocumented) constraint that notifiers
> may not be called if a thread is exiting because the entire
> process is exiting. This is fine for our current use because
> the callers use it only for cleaning up resources which go away
> on process exit (memory, Win32 fibers), and we will need the
> flexibility for the new posix implementation.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   include/qemu/thread.h | 22 ++++++++++++++++++++++
>   1 file changed, 22 insertions(+)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH for-3.1 2/2] util/qemu-thread-posix: Fix qemu_thread_atexit* for OSX

[Eric Blake]

On 11/5/18 7:55 AM, Peter Maydell wrote:
> Our current implementation of qemu_thread_atexit* is broken on OSX.
> This is because it works by cerating a piece of thread-specific

s/cerating/creating/

> data with pthread_key_create() and using the destructor function
> for that data to run the notifier function passed to it by
> the caller of qemu_thread_atexit_add(). The expected use case
> is that the caller uses a __thread variable as the notifier,
> and uses the callback to clean up information that it is
> keeping per-thread in __thread variables.
> 
> Unfortunately, on OSX this does not work, because on OSX
> a __thread variable may be destroyed (freed) before the
> pthread_key_create() destructor runs. (POSIX imposes no
> ordering constraint here; the OSX implementation happens
> to implement __thread variables in terms of pthread_key_create((),
> whereas Linux uses different mechanisms that mean the __thread
> variables will still be present when the pthread_key_create()
> destructor is run.)
> 
> Fix this by switching to a scheme similar to the one qemu-thread-win32
> uses for qemu_thread_atexit: keep the thread's notifiers on a
> __thread variable, and run the notifiers on calls to
> qemu_thread_exit() and on return from the start routine passed
> to qemu_thread_start(). We do this with the pthread_cleanup_push()
> API.
> 
> We take advantage of the qemu_thread_atexit_add() API
> permission not to run thread notifiers on process exit to
> avoid having to special case the main thread.
> 
> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> qemu-thread-win32.c tries to handle the "main thread" case
> by using an atexit() handler to run its notifiers. I don't
> think this will work because the atexit handler may not run
> on the main thread, in which case notify callback functions
> which refer to __thread variables will get the wrong ones.
> ---
>   util/qemu-thread-posix.c | 44 ++++++++++++++++++----------------------
>   1 file changed, 20 insertions(+), 24 deletions(-)
> 

> @@ -501,7 +494,10 @@ static void *qemu_thread_start(void *args)
>   #endif
>       g_free(qemu_thread_args->name);
>       g_free(qemu_thread_args);
> -    return start_routine(arg);
> +    pthread_cleanup_push(qemu_thread_atexit_notify, NULL);
> +    r = start_routine(arg);
> +    pthread_cleanup_pop(1);
> +    return r;
>   }
>   
>   void qemu_thread_create(QemuThread *thread, const char *name,
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH for-3.1 0/2] Fix qemu_thread_atexit* for OSX

[Paolo Bonzini]

On 05/11/2018 14:55, Peter Maydell wrote:
> Our current implementation of qemu_thread_atexit* is broken on OSX.
> This is because it works by cerating a piece of thread-specific
> data with pthread_key_create() and using the destructor function
> for that data to run the notifier function passed to it by
> the caller of qemu_thread_atexit_add(). The expected use case
> is that the caller uses a __thread variable as the notifier,
> and uses the callback to clean up information that it is
> keeping per-thread in __thread variables.
> 
> Unfortunately, on OSX this does not work, because on OSX
> a __thread variable may be destroyed (freed) before the
> pthread_key_create() destructor runs. (POSIX imposes no
> ordering constraint here; the OSX implementation happens
> to implement __thread variables in terms of pthread_key_create((),
> whereas Linux uses different mechanisms that mean the __thread
> variables will still be present when the pthread_key_create()
> destructor is run.)
> 
> Fix this by switching to a scheme similar to the one qemu-thread-win32
> uses for qemu_thread_atexit: keep the thread's notifiers on a
> __thread variable, and run the notifiers on calls to
> qemu_thread_exit() and on return from the start routine passed
> to qemu_thread_start(). We do this with the pthread_cleanup_push()
> API. (This approach was suggested by Paolo.)
> 
> There is a slightly awkward corner here for the main thread,
> which isn't started via qemu_thread_start() and so can exit
> without passing through the cleanup handler. qemu-thread-win32.c
> tries to handle the main thread using an atexit() handler to
> run its notifiers. I don't think this will work because the atexit
> handler may not run on the main thread, in which case notify
> callback functions which refer to __thread variables will get
> the wrong ones. So instead I have documented that the API
> leaves it unspecified whether notifiers are run for thread
> exits that happen when the entire process is exiting. This
> is OK for our current users (who only use the callbacks to
> clean up memory or win32 Fibers which are deleted on process
> exit anyway), and means we don't need to worry about running
> callbacks on main thread exit (which always causes the whole
> process to exit).
> 
> In particular, this fixes the ASAN errors and lockups we
> currently see on OSX hosts when running test-bdrv-drain.

Queued, thanks.  As an extra cleanup we can remove
qemu_thread_atexit_remove (it is unused, and a simple notifier_remove
now works), and also remove the atexit from Win32 so we can document
that the notifier does _not_ run for the main thread.

Paolo