[libvirt] [PATCH 0/5] Fix deadlock in nwfilter code

Since we introduced fine grained locking into the QEMU driver so that VM start can run in parallel, we appear to have caused a race with the nwfilter code. In particular since we no longer hold the global QEMU driver lock for the duration of VM startup we have a lock ordering flaw. This results in deadlock when nwfilter operations happen in parallel with VM startup. This also affects the LXC driver. This patch series attempts to address the problem https://bugzilla.redhat.com/show_bug.cgi?id=929412 the removal of the windows thread impl isn't strictly required, I just didn't want to waste time creating a read/write lock impl for Windows threads. Daniel P. Berrange (5): Add a read/write lock implementation Remove windows thread implementation in favour of pthreads Push nwfilter update locking up to top level Add a mutex to serialize updates to firewall Turn nwfilter conf update mutex into a read/write lock configure.ac | 15 +- src/Makefile.am | 4 - src/conf/nwfilter_conf.c | 23 +- src/conf/nwfilter_conf.h | 3 +- src/libvirt_private.syms | 8 +- src/lxc/lxc_driver.c | 6 + src/nwfilter/nwfilter_driver.c | 12 +- src/nwfilter/nwfilter_gentech_driver.c | 21 +- src/nwfilter/nwfilter_gentech_driver.h | 2 +- src/qemu/qemu_driver.c | 6 + src/uml/uml_driver.c | 4 + src/util/virthread.c | 291 +++++++++++++++++++++++- src/util/virthread.h | 51 ++++- src/util/virthreadpthread.c | 278 ----------------------- src/util/virthreadpthread.h | 49 ---- src/util/virthreadwin32.c | 396 --------------------------------- src/util/virthreadwin32.h | 53 ----- 17 files changed, 395 insertions(+), 827 deletions(-) delete mode 100644 src/util/virthreadpthread.c delete mode 100644 src/util/virthreadpthread.h delete mode 100644 src/util/virthreadwin32.c delete mode 100644 src/util/virthreadwin32.h -- 1.8.4.2

Add virRWLock backed up by a POSIX rwlock primitive Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/libvirt_private.syms | 5 +++++ src/util/virthread.h | 10 ++++++++++ src/util/virthreadpthread.c | 31 +++++++++++++++++++++++++++++++ src/util/virthreadpthread.h | 4 ++++ 4 files changed, 50 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index d1a58f9..eb91693 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1816,6 +1816,11 @@ virMutexInitRecursive; virMutexLock; virMutexUnlock; virOnce; +virRWLockInit; +virRWLockDestroy; +virRWLockRead; +virRWLockWrite; +virRWLockUnlock; virThreadCancel; virThreadCreate; virThreadID; diff --git a/src/util/virthread.h b/src/util/virthread.h index 84d3bdc..7015d60 100644 --- a/src/util/virthread.h +++ b/src/util/virthread.h @@ -28,6 +28,9 @@ typedef struct virMutex virMutex; typedef virMutex *virMutexPtr; +typedef struct virRWLock virRWLock; +typedef virRWLock *virRWLockPtr; + typedef struct virCond virCond; typedef virCond *virCondPtr; @@ -89,6 +92,13 @@ void virMutexLock(virMutexPtr m); void virMutexUnlock(virMutexPtr m); +int virRWLockInit(virRWLockPtr m) ATTRIBUTE_RETURN_CHECK; +void virRWLockDestroy(virRWLockPtr m); + +void virRWLockRead(virRWLockPtr m); +void virRWLockWrite(virRWLockPtr m); +void virRWLockUnlock(virRWLockPtr m); + int virCondInit(virCondPtr c) ATTRIBUTE_RETURN_CHECK; int virCondDestroy(virCondPtr c); diff --git a/src/util/virthreadpthread.c b/src/util/virthreadpthread.c index ca841e4..2efb4c1 100644 --- a/src/util/virthreadpthread.c +++ b/src/util/virthreadpthread.c @@ -91,6 +91,37 @@ void virMutexUnlock(virMutexPtr m) } +int virRWLockInit(virRWLockPtr m) +{ + if (pthread_rwlock_init(&m->lock, NULL) != 0) { + errno = EINVAL; + return -1; + } + return 0; +} + +void virRWLockDestroy(virRWLockPtr m) +{ + pthread_rwlock_destroy(&m->lock); +} + + +void virRWLockRead(virRWLockPtr m) +{ + pthread_rwlock_rdlock(&m->lock); +} + +void virRWLockWrite(virRWLockPtr m) +{ + pthread_rwlock_wrlock(&m->lock); +} + + +void virRWLockUnlock(virRWLockPtr m) +{ + pthread_rwlock_unlock(&m->lock); +} + int virCondInit(virCondPtr c) { int ret; diff --git a/src/util/virthreadpthread.h b/src/util/virthreadpthread.h index b9f1319..cb607d0 100644 --- a/src/util/virthreadpthread.h +++ b/src/util/virthreadpthread.h @@ -27,6 +27,10 @@ struct virMutex { pthread_mutex_t lock; }; +struct virRWLock { + pthread_rwlock_t lock; +}; + struct virCond { pthread_cond_t cond; }; -- 1.8.4.2

On 01/23/2014 02:37 PM, Daniel P. Berrange wrote:
Add virRWLock backed up by a POSIX rwlock primitive
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/libvirt_private.syms | 5 +++++ src/util/virthread.h | 10 ++++++++++ src/util/virthreadpthread.c | 31 +++++++++++++++++++++++++++++++ src/util/virthreadpthread.h | 4 ++++ 4 files changed, 50 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index d1a58f9..eb91693 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1816,6 +1816,11 @@ virMutexInitRecursive; virMutexLock; virMutexUnlock; virOnce; +virRWLockInit; +virRWLockDestroy; +virRWLockRead; +virRWLockWrite; +virRWLockUnlock;
These are out of order.
virThreadCancel; virThreadCreate; virThreadID; diff --git a/src/util/virthread.h b/src/util/virthread.h index 84d3bdc..7015d60 100644 --- a/src/util/virthread.h +++ b/src/util/virthread.h @@ -28,6 +28,9 @@ typedef struct virMutex virMutex; typedef virMutex *virMutexPtr;
+typedef struct virRWLock virRWLock; +typedef virRWLock *virRWLockPtr; + typedef struct virCond virCond; typedef virCond *virCondPtr;
@@ -89,6 +92,13 @@ void virMutexLock(virMutexPtr m); void virMutexUnlock(virMutexPtr m);
+int virRWLockInit(virRWLockPtr m) ATTRIBUTE_RETURN_CHECK; +void virRWLockDestroy(virRWLockPtr m); + +void virRWLockRead(virRWLockPtr m); +void virRWLockWrite(virRWLockPtr m); +void virRWLockUnlock(virRWLockPtr m); +
I was about to predict a build failure on Windows since you haven't defined these functions in virthreadwin32.c, but then realized you aren't yet calling them from anywhere, so the build will complete just fine. It would reduce code motion a bit to swap this patch with patch 2, but functionally there's no difference. As far as the implementation of the functions - it can't get any more striaghtforward that this! ACK.
int virCondInit(virCondPtr c) ATTRIBUTE_RETURN_CHECK; int virCondDestroy(virCondPtr c); diff --git a/src/util/virthreadpthread.c b/src/util/virthreadpthread.c index ca841e4..2efb4c1 100644 --- a/src/util/virthreadpthread.c +++ b/src/util/virthreadpthread.c @@ -91,6 +91,37 @@ void virMutexUnlock(virMutexPtr m) }
+int virRWLockInit(virRWLockPtr m) +{ + if (pthread_rwlock_init(&m->lock, NULL) != 0) { + errno = EINVAL; + return -1; + } + return 0; +} + +void virRWLockDestroy(virRWLockPtr m) +{ + pthread_rwlock_destroy(&m->lock); +} + + +void virRWLockRead(virRWLockPtr m) +{ + pthread_rwlock_rdlock(&m->lock); +} + +void virRWLockWrite(virRWLockPtr m) +{ + pthread_rwlock_wrlock(&m->lock); +} + + +void virRWLockUnlock(virRWLockPtr m) +{ + pthread_rwlock_unlock(&m->lock); +} + int virCondInit(virCondPtr c) { int ret; diff --git a/src/util/virthreadpthread.h b/src/util/virthreadpthread.h index b9f1319..cb607d0 100644 --- a/src/util/virthreadpthread.h +++ b/src/util/virthreadpthread.h @@ -27,6 +27,10 @@ struct virMutex { pthread_mutex_t lock; };
+struct virRWLock { + pthread_rwlock_t lock; +}; + struct virCond { pthread_cond_t cond; };

On Thu, Jan 23, 2014 at 03:22:53PM +0200, Laine Stump wrote:
On 01/23/2014 02:37 PM, Daniel P. Berrange wrote:
Add virRWLock backed up by a POSIX rwlock primitive
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/libvirt_private.syms | 5 +++++ src/util/virthread.h | 10 ++++++++++ src/util/virthreadpthread.c | 31 +++++++++++++++++++++++++++++++ src/util/virthreadpthread.h | 4 ++++ 4 files changed, 50 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index d1a58f9..eb91693 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1816,6 +1816,11 @@ virMutexInitRecursive; virMutexLock; virMutexUnlock; virOnce; +virRWLockInit; +virRWLockDestroy; +virRWLockRead; +virRWLockWrite; +virRWLockUnlock;
These are out of order.
virThreadCancel; virThreadCreate; virThreadID; diff --git a/src/util/virthread.h b/src/util/virthread.h index 84d3bdc..7015d60 100644 --- a/src/util/virthread.h +++ b/src/util/virthread.h @@ -28,6 +28,9 @@ typedef struct virMutex virMutex; typedef virMutex *virMutexPtr;
+typedef struct virRWLock virRWLock; +typedef virRWLock *virRWLockPtr; + typedef struct virCond virCond; typedef virCond *virCondPtr;
@@ -89,6 +92,13 @@ void virMutexLock(virMutexPtr m); void virMutexUnlock(virMutexPtr m);
+int virRWLockInit(virRWLockPtr m) ATTRIBUTE_RETURN_CHECK; +void virRWLockDestroy(virRWLockPtr m); + +void virRWLockRead(virRWLockPtr m); +void virRWLockWrite(virRWLockPtr m); +void virRWLockUnlock(virRWLockPtr m); +
I was about to predict a build failure on Windows since you haven't defined these functions in virthreadwin32.c, but then realized you aren't yet calling them from anywhere, so the build will complete just fine.
It would reduce code motion a bit to swap this patch with patch 2, but functionally there's no difference.
FYI the reason I did it in this order, is to make it easier to backport to the stable branches. eg on the stable branches I'd want to skip patch 2, since that's going to require a gnulib update too. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

On 01/23/2014 03:46 PM, Daniel P. Berrange wrote:
On Thu, Jan 23, 2014 at 03:22:53PM +0200, Laine Stump wrote:
On 01/23/2014 02:37 PM, Daniel P. Berrange wrote:
Add virRWLock backed up by a POSIX rwlock primitive
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/libvirt_private.syms | 5 +++++ src/util/virthread.h | 10 ++++++++++ src/util/virthreadpthread.c | 31 +++++++++++++++++++++++++++++++ src/util/virthreadpthread.h | 4 ++++ 4 files changed, 50 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index d1a58f9..eb91693 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1816,6 +1816,11 @@ virMutexInitRecursive; virMutexLock; virMutexUnlock; virOnce; +virRWLockInit; +virRWLockDestroy; +virRWLockRead; +virRWLockWrite; +virRWLockUnlock; These are out of order.
virThreadCancel; virThreadCreate; virThreadID; diff --git a/src/util/virthread.h b/src/util/virthread.h index 84d3bdc..7015d60 100644 --- a/src/util/virthread.h +++ b/src/util/virthread.h @@ -28,6 +28,9 @@ typedef struct virMutex virMutex; typedef virMutex *virMutexPtr;
+typedef struct virRWLock virRWLock; +typedef virRWLock *virRWLockPtr; + typedef struct virCond virCond; typedef virCond *virCondPtr;
@@ -89,6 +92,13 @@ void virMutexLock(virMutexPtr m); void virMutexUnlock(virMutexPtr m);
+int virRWLockInit(virRWLockPtr m) ATTRIBUTE_RETURN_CHECK; +void virRWLockDestroy(virRWLockPtr m); + +void virRWLockRead(virRWLockPtr m); +void virRWLockWrite(virRWLockPtr m); +void virRWLockUnlock(virRWLockPtr m); + I was about to predict a build failure on Windows since you haven't defined these functions in virthreadwin32.c, but then realized you aren't yet calling them from anywhere, so the build will complete just fine.
It would reduce code motion a bit to swap this patch with patch 2, but functionally there's no difference. FYI the reason I did it in this order, is to make it easier to backport to the stable branches. eg on the stable branches I'd want to skip patch 2, since that's going to require a gnulib update too.
Aha. Totally worth it then!

On 01/23/2014 05:37 AM, Daniel P. Berrange wrote:
Add virRWLock backed up by a POSIX rwlock primitive
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/libvirt_private.syms | 5 +++++ src/util/virthread.h | 10 ++++++++++ src/util/virthreadpthread.c | 31 +++++++++++++++++++++++++++++++ src/util/virthreadpthread.h | 4 ++++ 4 files changed, 50 insertions(+)
+int virRWLockInit(virRWLockPtr m) +{ + if (pthread_rwlock_init(&m->lock, NULL) != 0) { + errno = EINVAL;
Wouldn't this be better as: int rc = pthread_rwlock_init(&m->lock, NULL); if (rc) { errno = rc; return -1; } return 0; so that we aren't clobbering non-EINVAL errors?
+void virRWLockDestroy(virRWLockPtr m) +{ + pthread_rwlock_destroy(&m->lock); +} + + +void virRWLockRead(virRWLockPtr m) +{ + pthread_rwlock_rdlock(&m->lock);
Worth a VIR_DEBUG on failure of these functions, to help diagnose incorrect usage? -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

There are a number of pthreads impls available on Win32 these days, in particular the mingw64 project has a good impl. Delete the native windows thread implementation and rely on using pthreads everywhere. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- configure.ac | 15 +- src/Makefile.am | 4 - src/util/virthread.c | 291 +++++++++++++++++++++++++++++++- src/util/virthread.h | 41 ++++- src/util/virthreadpthread.c | 309 ---------------------------------- src/util/virthreadpthread.h | 53 ------ src/util/virthreadwin32.c | 396 -------------------------------------------- src/util/virthreadwin32.h | 53 ------ 8 files changed, 329 insertions(+), 833 deletions(-) delete mode 100644 src/util/virthreadpthread.c delete mode 100644 src/util/virthreadpthread.h delete mode 100644 src/util/virthreadwin32.c delete mode 100644 src/util/virthreadwin32.h diff --git a/configure.ac b/configure.ac index 3a70375..168eb27 100644 --- a/configure.ac +++ b/configure.ac @@ -270,12 +270,21 @@ AC_CHECK_FUNCS_ONCE([cfmakeraw fallocate geteuid getgid getgrnam_r \ posix_memalign prlimit regexec sched_getaffinity setgroups setns \ setrlimit symlink sysctlbyname]) -dnl Availability of pthread functions (if missing, win32 threading is -dnl assumed). Because of $LIB_PTHREAD, we cannot use AC_CHECK_FUNCS_ONCE. -dnl LIB_PTHREAD and LIBMULTITHREAD were set during gl_INIT by gnulib. +dnl Availability of pthread functions. Because of $LIB_PTHREAD, we +dnl cannot use AC_CHECK_FUNCS_ONCE. LIB_PTHREAD and LIBMULTITHREAD +dnl were set during gl_INIT by gnulib. old_LIBS=$LIBS LIBS="$LIBS $LIB_PTHREAD $LIBMULTITHREAD" + +pthread_found=yes AC_CHECK_FUNCS([pthread_mutexattr_init]) +AC_CHECK_HEADER([pthread.h],,[pthread_found=no]) + +if test "$ac_cv_func_pthread_mutexattr_init:$pthread_found" != "yes:yes" +then + AC_MSG_ERROR([A pthreads impl is required for building libvirt]) +fi + dnl At least mingw64-winpthreads #defines pthread_sigmask to 0, dnl which in turn causes compilation to complain about unused variables. dnl Expose this broken implementation, so we can work around it. diff --git a/src/Makefile.am b/src/Makefile.am index 8f77658..2298fdb 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -141,8 +141,6 @@ UTIL_SOURCES = \ util/virsysinfo.c util/virsysinfo.h \ util/virsystemd.c util/virsystemd.h \ util/virthread.c util/virthread.h \ - util/virthreadpthread.h \ - util/virthreadwin32.h \ util/virthreadpool.c util/virthreadpool.h \ util/virtime.h util/virtime.c \ util/virtpm.h util/virtpm.c \ @@ -165,8 +163,6 @@ util/virkeymaps.h: $(srcdir)/util/keymaps.csv \ $(AM_V_GEN)$(PYTHON) $(srcdir)/util/virkeycode-mapgen.py \ <$(srcdir)/util/keymaps.csv >$(srcdir)/util/virkeymaps.h -EXTRA_DIST += util/virthreadpthread.c util/virthreadwin32.c - # Internal generic driver infrastructure NODE_INFO_SOURCES = nodeinfo.h nodeinfo.c DATATYPES_SOURCES = datatypes.h datatypes.c diff --git a/src/util/virthread.c b/src/util/virthread.c index dd1768e..b60fb4a 100644 --- a/src/util/virthread.c +++ b/src/util/virthread.c @@ -23,12 +23,289 @@ #include "virthread.h" -/* On mingw, we prefer native threading over the sometimes-broken - * pthreads-win32 library wrapper. */ -#ifdef WIN32 -# include "virthreadwin32.c" -#elif defined HAVE_PTHREAD_MUTEXATTR_INIT -# include "virthreadpthread.c" +#include <unistd.h> +#include <inttypes.h> +#if HAVE_SYS_SYSCALL_H +# include <sys/syscall.h> +#endif + +#include "viralloc.h" + + +/* Nothing special required for pthreads */ +int virThreadInitialize(void) +{ + return 0; +} + +void virThreadOnExit(void) +{ +} + +int virOnce(virOnceControlPtr once, virOnceFunc init) +{ + return pthread_once(&once->once, init); +} + + +int virMutexInit(virMutexPtr m) +{ + int ret; + pthread_mutexattr_t attr; + pthread_mutexattr_init(&attr); + pthread_mutexattr_settype(&attr, PTHREAD_MUTEX_NORMAL); + ret = pthread_mutex_init(&m->lock, &attr); + pthread_mutexattr_destroy(&attr); + if (ret != 0) { + errno = ret; + return -1; + } + return 0; +} + +int virMutexInitRecursive(virMutexPtr m) +{ + int ret; + pthread_mutexattr_t attr; + pthread_mutexattr_init(&attr); + pthread_mutexattr_settype(&attr, PTHREAD_MUTEX_RECURSIVE); + ret = pthread_mutex_init(&m->lock, &attr); + pthread_mutexattr_destroy(&attr); + if (ret != 0) { + errno = ret; + return -1; + } + return 0; +} + +void virMutexDestroy(virMutexPtr m) +{ + pthread_mutex_destroy(&m->lock); +} + +void virMutexLock(virMutexPtr m){ + pthread_mutex_lock(&m->lock); +} + +void virMutexUnlock(virMutexPtr m) +{ + pthread_mutex_unlock(&m->lock); +} + + +int virRWLockInit(virRWLockPtr m) +{ + if (pthread_rwlock_init(&m->lock, NULL) != 0) { + errno = EINVAL; + return -1; + } + return 0; +} + +void virRWLockDestroy(virRWLockPtr m) +{ + pthread_rwlock_destroy(&m->lock); +} + + +void virRWLockRead(virRWLockPtr m) +{ + pthread_rwlock_rdlock(&m->lock); +} + +void virRWLockWrite(virRWLockPtr m) +{ + pthread_rwlock_wrlock(&m->lock); +} + + +void virRWLockUnlock(virRWLockPtr m) +{ + pthread_rwlock_unlock(&m->lock); +} + +int virCondInit(virCondPtr c) +{ + int ret; + if ((ret = pthread_cond_init(&c->cond, NULL)) != 0) { + errno = ret; + return -1; + } + return 0; +} + +int virCondDestroy(virCondPtr c) +{ + int ret; + if ((ret = pthread_cond_destroy(&c->cond)) != 0) { + errno = ret; + return -1; + } + return 0; +} + +int virCondWait(virCondPtr c, virMutexPtr m) +{ + int ret; + if ((ret = pthread_cond_wait(&c->cond, &m->lock)) != 0) { + errno = ret; + return -1; + } + return 0; +} + +int virCondWaitUntil(virCondPtr c, virMutexPtr m, unsigned long long whenms) +{ + int ret; + struct timespec ts; + + ts.tv_sec = whenms / 1000; + ts.tv_nsec = (whenms % 1000) * 1000; + + if ((ret = pthread_cond_timedwait(&c->cond, &m->lock, &ts)) != 0) { + errno = ret; + return -1; + } + return 0; +} + +void virCondSignal(virCondPtr c) +{ + pthread_cond_signal(&c->cond); +} + +void virCondBroadcast(virCondPtr c) +{ + pthread_cond_broadcast(&c->cond); +} + +struct virThreadArgs { + virThreadFunc func; + void *opaque; +}; + +static void *virThreadHelper(void *data) +{ + struct virThreadArgs *args = data; + struct virThreadArgs local = *args; + + /* Free args early, rather than tying it up during the entire thread. */ + VIR_FREE(args); + local.func(local.opaque); + return NULL; +} + +int virThreadCreate(virThreadPtr thread, + bool joinable, + virThreadFunc func, + void *opaque) +{ + struct virThreadArgs *args; + pthread_attr_t attr; + int ret = -1; + int err; + + if ((err = pthread_attr_init(&attr)) != 0) + goto cleanup; + if (VIR_ALLOC_QUIET(args) < 0) { + err = ENOMEM; + goto cleanup; + } + + args->func = func; + args->opaque = opaque; + + if (!joinable) + pthread_attr_setdetachstate(&attr, 1); + + err = pthread_create(&thread->thread, &attr, virThreadHelper, args); + if (err != 0) { + VIR_FREE(args); + goto cleanup; + } + /* New thread owns 'args' in success case, so don't free */ + + ret = 0; +cleanup: + pthread_attr_destroy(&attr); + if (ret < 0) + errno = err; + return ret; +} + +void virThreadSelf(virThreadPtr thread) +{ + thread->thread = pthread_self(); +} + +bool virThreadIsSelf(virThreadPtr thread) +{ + return pthread_equal(pthread_self(), thread->thread) ? true : false; +} + +/* For debugging use only; this result is not guaranteed unique if + * pthread_t is larger than a 64-bit pointer, nor does it always match + * the pthread_self() id on Linux. */ +unsigned long long virThreadSelfID(void) +{ +#if defined(HAVE_SYS_SYSCALL_H) && defined(SYS_gettid) + pid_t tid = syscall(SYS_gettid); + return tid; #else -# error "Either pthreads or Win32 threads are required" + union { + unsigned long long l; + pthread_t t; + } u; + u.t = pthread_self(); + return u.l; #endif +} + +/* For debugging use only; this result is not guaranteed unique if + * pthread_t is larger than a 64-bit pointer, nor does it always match + * the thread id of virThreadSelfID on Linux. */ +unsigned long long virThreadID(virThreadPtr thread) +{ + union { + unsigned long long l; + pthread_t t; + } u; + u.t = thread->thread; + return u.l; +} + +void virThreadJoin(virThreadPtr thread) +{ + pthread_join(thread->thread, NULL); +} + +void virThreadCancel(virThreadPtr thread) +{ + pthread_cancel(thread->thread); +} + +int virThreadLocalInit(virThreadLocalPtr l, + virThreadLocalCleanup c) +{ + int ret; + if ((ret = pthread_key_create(&l->key, c)) != 0) { + errno = ret; + return -1; + } + return 0; +} + +void *virThreadLocalGet(virThreadLocalPtr l) +{ + return pthread_getspecific(l->key); +} + +int virThreadLocalSet(virThreadLocalPtr l, void *val) +{ + int err = pthread_setspecific(l->key, val); + if (err) { + errno = err; + return -1; + } + return 0; +} diff --git a/src/util/virthread.h b/src/util/virthread.h index 7015d60..94476ee 100644 --- a/src/util/virthread.h +++ b/src/util/virthread.h @@ -25,24 +25,57 @@ # include "internal.h" # include "virerror.h" +# include <pthread.h> + typedef struct virMutex virMutex; typedef virMutex *virMutexPtr; +struct virMutex { + pthread_mutex_t lock; +}; + typedef struct virRWLock virRWLock; typedef virRWLock *virRWLockPtr; +struct virRWLock { + pthread_rwlock_t lock; +}; + + typedef struct virCond virCond; typedef virCond *virCondPtr; +struct virCond { + pthread_cond_t cond; +}; + typedef struct virThreadLocal virThreadLocal; typedef virThreadLocal *virThreadLocalPtr; +struct virThreadLocal { + pthread_key_t key; +}; + typedef struct virThread virThread; typedef virThread *virThreadPtr; +struct virThread { + pthread_t thread; +}; + typedef struct virOnceControl virOnceControl; typedef virOnceControl *virOnceControlPtr; +struct virOnceControl { + pthread_once_t once; +}; + + +#define VIR_ONCE_CONTROL_INITIALIZER \ +{ \ + .once = PTHREAD_ONCE_INIT \ +} + typedef void (*virOnceFunc)(void); int virThreadInitialize(void) ATTRIBUTE_RETURN_CHECK; @@ -121,14 +154,6 @@ int virThreadLocalInit(virThreadLocalPtr l, void *virThreadLocalGet(virThreadLocalPtr l); int virThreadLocalSet(virThreadLocalPtr l, void*) ATTRIBUTE_RETURN_CHECK; -# ifdef WIN32 -# include "virthreadwin32.h" -# elif defined HAVE_PTHREAD_MUTEXATTR_INIT -# include "virthreadpthread.h" -# else -# error "Either pthreads or Win32 threads are required" -# endif - /** * VIR_ONCE_GLOBAL_INIT: diff --git a/src/util/virthreadpthread.c b/src/util/virthreadpthread.c deleted file mode 100644 index 2efb4c1..0000000 --- a/src/util/virthreadpthread.c +++ /dev/null @@ -1,309 +0,0 @@ -/* - * virthreadpthread.c: basic thread synchronization primitives - * - * Copyright (C) 2009-2011, 2013 Red Hat, Inc. - * - * This library is free software; you can redistribute it and/or - * modify it under the terms of the GNU Lesser General Public - * License as published by the Free Software Foundation; either - * version 2.1 of the License, or (at your option) any later version. - * - * This library is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU - * Lesser General Public License for more details. - * - * You should have received a copy of the GNU Lesser General Public - * License along with this library. If not, see - * <http://www.gnu.org/licenses/>. - * - */ - -#include <config.h> - -#include <unistd.h> -#include <inttypes.h> -#if HAVE_SYS_SYSCALL_H -# include <sys/syscall.h> -#endif - -#include "viralloc.h" - - -/* Nothing special required for pthreads */ -int virThreadInitialize(void) -{ - return 0; -} - -void virThreadOnExit(void) -{ -} - -int virOnce(virOnceControlPtr once, virOnceFunc init) -{ - return pthread_once(&once->once, init); -} - - -int virMutexInit(virMutexPtr m) -{ - int ret; - pthread_mutexattr_t attr; - pthread_mutexattr_init(&attr); - pthread_mutexattr_settype(&attr, PTHREAD_MUTEX_NORMAL); - ret = pthread_mutex_init(&m->lock, &attr); - pthread_mutexattr_destroy(&attr); - if (ret != 0) { - errno = ret; - return -1; - } - return 0; -} - -int virMutexInitRecursive(virMutexPtr m) -{ - int ret; - pthread_mutexattr_t attr; - pthread_mutexattr_init(&attr); - pthread_mutexattr_settype(&attr, PTHREAD_MUTEX_RECURSIVE); - ret = pthread_mutex_init(&m->lock, &attr); - pthread_mutexattr_destroy(&attr); - if (ret != 0) { - errno = ret; - return -1; - } - return 0; -} - -void virMutexDestroy(virMutexPtr m) -{ - pthread_mutex_destroy(&m->lock); -} - -void virMutexLock(virMutexPtr m){ - pthread_mutex_lock(&m->lock); -} - -void virMutexUnlock(virMutexPtr m) -{ - pthread_mutex_unlock(&m->lock); -} - - -int virRWLockInit(virRWLockPtr m) -{ - if (pthread_rwlock_init(&m->lock, NULL) != 0) { - errno = EINVAL; - return -1; - } - return 0; -} - -void virRWLockDestroy(virRWLockPtr m) -{ - pthread_rwlock_destroy(&m->lock); -} - - -void virRWLockRead(virRWLockPtr m) -{ - pthread_rwlock_rdlock(&m->lock); -} - -void virRWLockWrite(virRWLockPtr m) -{ - pthread_rwlock_wrlock(&m->lock); -} - - -void virRWLockUnlock(virRWLockPtr m) -{ - pthread_rwlock_unlock(&m->lock); -} - -int virCondInit(virCondPtr c) -{ - int ret; - if ((ret = pthread_cond_init(&c->cond, NULL)) != 0) { - errno = ret; - return -1; - } - return 0; -} - -int virCondDestroy(virCondPtr c) -{ - int ret; - if ((ret = pthread_cond_destroy(&c->cond)) != 0) { - errno = ret; - return -1; - } - return 0; -} - -int virCondWait(virCondPtr c, virMutexPtr m) -{ - int ret; - if ((ret = pthread_cond_wait(&c->cond, &m->lock)) != 0) { - errno = ret; - return -1; - } - return 0; -} - -int virCondWaitUntil(virCondPtr c, virMutexPtr m, unsigned long long whenms) -{ - int ret; - struct timespec ts; - - ts.tv_sec = whenms / 1000; - ts.tv_nsec = (whenms % 1000) * 1000; - - if ((ret = pthread_cond_timedwait(&c->cond, &m->lock, &ts)) != 0) { - errno = ret; - return -1; - } - return 0; -} - -void virCondSignal(virCondPtr c) -{ - pthread_cond_signal(&c->cond); -} - -void virCondBroadcast(virCondPtr c) -{ - pthread_cond_broadcast(&c->cond); -} - -struct virThreadArgs { - virThreadFunc func; - void *opaque; -}; - -static void *virThreadHelper(void *data) -{ - struct virThreadArgs *args = data; - struct virThreadArgs local = *args; - - /* Free args early, rather than tying it up during the entire thread. */ - VIR_FREE(args); - local.func(local.opaque); - return NULL; -} - -int virThreadCreate(virThreadPtr thread, - bool joinable, - virThreadFunc func, - void *opaque) -{ - struct virThreadArgs *args; - pthread_attr_t attr; - int ret = -1; - int err; - - if ((err = pthread_attr_init(&attr)) != 0) - goto cleanup; - if (VIR_ALLOC_QUIET(args) < 0) { - err = ENOMEM; - goto cleanup; - } - - args->func = func; - args->opaque = opaque; - - if (!joinable) - pthread_attr_setdetachstate(&attr, 1); - - err = pthread_create(&thread->thread, &attr, virThreadHelper, args); - if (err != 0) { - VIR_FREE(args); - goto cleanup; - } - /* New thread owns 'args' in success case, so don't free */ - - ret = 0; -cleanup: - pthread_attr_destroy(&attr); - if (ret < 0) - errno = err; - return ret; -} - -void virThreadSelf(virThreadPtr thread) -{ - thread->thread = pthread_self(); -} - -bool virThreadIsSelf(virThreadPtr thread) -{ - return pthread_equal(pthread_self(), thread->thread) ? true : false; -} - -/* For debugging use only; this result is not guaranteed unique if - * pthread_t is larger than a 64-bit pointer, nor does it always match - * the pthread_self() id on Linux. */ -unsigned long long virThreadSelfID(void) -{ -#if defined(HAVE_SYS_SYSCALL_H) && defined(SYS_gettid) - pid_t tid = syscall(SYS_gettid); - return tid; -#else - union { - unsigned long long l; - pthread_t t; - } u; - u.t = pthread_self(); - return u.l; -#endif -} - -/* For debugging use only; this result is not guaranteed unique if - * pthread_t is larger than a 64-bit pointer, nor does it always match - * the thread id of virThreadSelfID on Linux. */ -unsigned long long virThreadID(virThreadPtr thread) -{ - union { - unsigned long long l; - pthread_t t; - } u; - u.t = thread->thread; - return u.l; -} - -void virThreadJoin(virThreadPtr thread) -{ - pthread_join(thread->thread, NULL); -} - -void virThreadCancel(virThreadPtr thread) -{ - pthread_cancel(thread->thread); -} - -int virThreadLocalInit(virThreadLocalPtr l, - virThreadLocalCleanup c) -{ - int ret; - if ((ret = pthread_key_create(&l->key, c)) != 0) { - errno = ret; - return -1; - } - return 0; -} - -void *virThreadLocalGet(virThreadLocalPtr l) -{ - return pthread_getspecific(l->key); -} - -int virThreadLocalSet(virThreadLocalPtr l, void *val) -{ - int err = pthread_setspecific(l->key, val); - if (err) { - errno = err; - return -1; - } - return 0; -} diff --git a/src/util/virthreadpthread.h b/src/util/virthreadpthread.h deleted file mode 100644 index cb607d0..0000000 --- a/src/util/virthreadpthread.h +++ /dev/null @@ -1,53 +0,0 @@ -/* - * virthreadpthread.c: basic thread synchronization primitives - * - * Copyright (C) 2009, 2011 Red Hat, Inc. - * - * This library is free software; you can redistribute it and/or - * modify it under the terms of the GNU Lesser General Public - * License as published by the Free Software Foundation; either - * version 2.1 of the License, or (at your option) any later version. - * - * This library is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU - * Lesser General Public License for more details. - * - * You should have received a copy of the GNU Lesser General Public - * License along with this library. If not, see - * <http://www.gnu.org/licenses/>. - * - */ - -#include "internal.h" - -#include <pthread.h> - -struct virMutex { - pthread_mutex_t lock; -}; - -struct virRWLock { - pthread_rwlock_t lock; -}; - -struct virCond { - pthread_cond_t cond; -}; - -struct virThread { - pthread_t thread; -}; - -struct virThreadLocal { - pthread_key_t key; -}; - -struct virOnceControl { - pthread_once_t once; -}; - -#define VIR_ONCE_CONTROL_INITIALIZER \ -{ \ - .once = PTHREAD_ONCE_INIT \ -} diff --git a/src/util/virthreadwin32.c b/src/util/virthreadwin32.c deleted file mode 100644 index 5d6277f..0000000 --- a/src/util/virthreadwin32.c +++ /dev/null @@ -1,396 +0,0 @@ -/* - * virthreadwin32.c: basic thread synchronization primitives - * - * Copyright (C) 2009-2011, 2013 Red Hat, Inc. - * - * This library is free software; you can redistribute it and/or - * modify it under the terms of the GNU Lesser General Public - * License as published by the Free Software Foundation; either - * version 2.1 of the License, or (at your option) any later version. - * - * This library is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU - * Lesser General Public License for more details. - * - * You should have received a copy of the GNU Lesser General Public - * License along with this library. If not, see - * <http://www.gnu.org/licenses/>. - * - */ - -#include <config.h> - -#include <process.h> - -#include "viralloc.h" - -#define VIR_FROM_THIS VIR_FROM_NONE - -struct virThreadLocalData { - DWORD key; - virThreadLocalCleanup cleanup; -}; -typedef struct virThreadLocalData virThreadLocalData; -typedef virThreadLocalData *virThreadLocalDataPtr; - -virMutex virThreadLocalLock; -size_t virThreadLocalCount = 0; -virThreadLocalDataPtr virThreadLocalList = NULL; -DWORD selfkey; - -virThreadLocal virCondEvent; - -void virCondEventCleanup(void *data); - -int virThreadInitialize(void) -{ - if (virMutexInit(&virThreadLocalLock) < 0) - return -1; - if (virThreadLocalInit(&virCondEvent, virCondEventCleanup) < 0) - return -1; - if ((selfkey = TlsAlloc()) == TLS_OUT_OF_INDEXES) - return -1; - return 0; -} - -void virThreadOnExit(void) -{ - size_t i; - virMutexLock(&virThreadLocalLock); - for (i = 0; i < virThreadLocalCount; i++) { - if (virThreadLocalList[i].cleanup) { - void *data = TlsGetValue(virThreadLocalList[i].key); - if (data) { - TlsSetValue(virThreadLocalList[i].key, NULL); - - (virThreadLocalList[i].cleanup)(data); - } - } - } - virMutexUnlock(&virThreadLocalLock); -} - -int virOnce(virOnceControlPtr once, virOnceFunc func) -{ - if (!once->complete) { - if (InterlockedIncrement(&once->init) == 1) { - /* We're the first thread. */ - func(); - once->complete = 1; - } else { - /* We're a later thread. Decrement the init counter back - * to avoid overflow, then yield until the first thread - * marks that the function is complete. It is rare that - * multiple threads will be waiting here, and since each - * thread is yielding except the first, we should get out - * soon enough. */ - InterlockedDecrement(&once->init); - while (!once->complete) - Sleep(0); - } - } - return 0; -} - -int virMutexInit(virMutexPtr m) -{ - return virMutexInitRecursive(m); -} - -int virMutexInitRecursive(virMutexPtr m) -{ - if (!(m->lock = CreateMutex(NULL, FALSE, NULL))) { - errno = ESRCH; - return -1; - } - return 0; -} - -void virMutexDestroy(virMutexPtr m) -{ - CloseHandle(m->lock); -} - -void virMutexLock(virMutexPtr m) -{ - WaitForSingleObject(m->lock, INFINITE); -} - -void virMutexUnlock(virMutexPtr m) -{ - ReleaseMutex(m->lock); -} - - - -int virCondInit(virCondPtr c) -{ - c->waiters = NULL; - if (virMutexInit(&c->lock) < 0) - return -1; - return 0; -} - -int virCondDestroy(virCondPtr c) -{ - if (c->waiters) { - errno = EINVAL; - return -1; - } - virMutexDestroy(&c->lock); - return 0; -} - -void virCondEventCleanup(void *data) -{ - HANDLE event = data; - CloseHandle(event); -} - -int virCondWait(virCondPtr c, virMutexPtr m) -{ - HANDLE event = virThreadLocalGet(&virCondEvent); - - if (!event) { - event = CreateEvent(0, FALSE, FALSE, NULL); - if (!event) { - return -1; - } - if (virThreadLocalSet(&virCondEvent, event) < 0) { - CloseHandle(event); - return -1; - } - } - - virMutexLock(&c->lock); - - if (VIR_REALLOC_N(c->waiters, c->nwaiters + 1) < 0) { - virMutexUnlock(&c->lock); - return -1; - } - c->waiters[c->nwaiters] = event; - c->nwaiters++; - - virMutexUnlock(&c->lock); - - virMutexUnlock(m); - - if (WaitForSingleObject(event, INFINITE) == WAIT_FAILED) { - virMutexLock(m); - errno = EINVAL; - return -1; - } - - virMutexLock(m); - return 0; -} - -int virCondWaitUntil(virCondPtr c ATTRIBUTE_UNUSED, - virMutexPtr m ATTRIBUTE_UNUSED, - unsigned long long whenms ATTRIBUTE_UNUSED) -{ - /* FIXME: this function is currently only used by the QEMU driver that - * is not compiled on Windows, so it's okay for now to just - * miss an implementation */ - return -1; -} - -void virCondSignal(virCondPtr c) -{ - virMutexLock(&c->lock); - - if (c->nwaiters) { - HANDLE event = c->waiters[0]; - if (c->nwaiters > 1) - memmove(c->waiters, - c->waiters + 1, - sizeof(c->waiters[0]) * (c->nwaiters-1)); - if (VIR_REALLOC_N(c->waiters, c->nwaiters - 1) < 0) { - ; - } - c->nwaiters--; - SetEvent(event); - } - - virMutexUnlock(&c->lock); -} - -void virCondBroadcast(virCondPtr c) -{ - virMutexLock(&c->lock); - - if (c->nwaiters) { - size_t i; - for (i = 0; i < c->nwaiters; i++) { - HANDLE event = c->waiters[i]; - SetEvent(event); - } - VIR_FREE(c->waiters); - c->nwaiters = 0; - } - - virMutexUnlock(&c->lock); -} - - -struct virThreadArgs { - virThreadFunc func; - void *opaque; -}; - -static void virThreadHelperDaemon(void *data) -{ - struct virThreadArgs *args = data; - virThread self; - HANDLE handle = GetCurrentThread(); - HANDLE process = GetCurrentProcess(); - - self.joinable = false; - DuplicateHandle(process, handle, process, - &self.thread, 0, FALSE, - DUPLICATE_SAME_ACCESS); - TlsSetValue(selfkey, &self); - - args->func(args->opaque); - - TlsSetValue(selfkey, NULL); - CloseHandle(self.thread); - - VIR_FREE(args); -} - -static unsigned int __stdcall virThreadHelperJoinable(void *data) -{ - struct virThreadArgs *args = data; - virThread self; - HANDLE handle = GetCurrentThread(); - HANDLE process = GetCurrentProcess(); - - self.joinable = true; - DuplicateHandle(process, handle, process, - &self.thread, 0, FALSE, - DUPLICATE_SAME_ACCESS); - TlsSetValue(selfkey, &self); - - args->func(args->opaque); - - TlsSetValue(selfkey, NULL); - CloseHandle(self.thread); - - VIR_FREE(args); - return 0; -} - -int virThreadCreate(virThreadPtr thread, - bool joinable, - virThreadFunc func, - void *opaque) -{ - struct virThreadArgs *args; - uintptr_t ret; - - if (VIR_ALLOC(args) < 0) - return -1; - - args->func = func; - args->opaque = opaque; - - thread->joinable = joinable; - if (joinable) { - ret = _beginthreadex(NULL, 0, - virThreadHelperJoinable, - args, 0, NULL); - if (ret == 0) - return -1; - } else { - ret = _beginthread(virThreadHelperDaemon, - 0, args); - if (ret == -1L) - return -1; - } - - thread->thread = (HANDLE)ret; - - return 0; -} - -void virThreadSelf(virThreadPtr thread) -{ - virThreadPtr self = TlsGetValue(selfkey); - - if (self == NULL) { - /* called on a thread not created by virThreadCreate, e.g. the main thread */ - thread->thread = 0; - thread->joinable = false; - } else { - thread->thread = self->thread; - thread->joinable = self->joinable; - } -} - -bool virThreadIsSelf(virThreadPtr thread) -{ - virThread self; - virThreadSelf(&self); - return self.thread == thread->thread ? true : false; -} - -/* For debugging use only; see comments in virthreadpthread.c. */ -unsigned long long virThreadSelfID(void) -{ - return GetCurrentThreadId(); -} - -/* For debugging use only; see comments in virthreadpthread.c. */ -unsigned long long virThreadID(virThreadPtr thread) -{ - return (intptr_t)thread->thread; -} - - -void virThreadJoin(virThreadPtr thread) -{ - if (thread->joinable) { - WaitForSingleObject(thread->thread, INFINITE); - CloseHandle(thread->thread); - thread->thread = 0; - thread->joinable = false; - } -} - -void virThreadCancel(virThreadPtr thread ATTRIBUTE_UNUSED) -{} - -int virThreadLocalInit(virThreadLocalPtr l, - virThreadLocalCleanup c) -{ - if ((l->key = TlsAlloc()) == TLS_OUT_OF_INDEXES) { - errno = ESRCH; - return -1; - } - TlsSetValue(l->key, NULL); - - if (c) { - virMutexLock(&virThreadLocalLock); - if (VIR_REALLOC_N(virThreadLocalList, - virThreadLocalCount + 1) < 0) - return -1; - virThreadLocalList[virThreadLocalCount].key = l->key; - virThreadLocalList[virThreadLocalCount].cleanup = c; - virThreadLocalCount++; - virMutexUnlock(&virThreadLocalLock); - } - - return 0; -} - -void *virThreadLocalGet(virThreadLocalPtr l) -{ - return TlsGetValue(l->key); -} - -int virThreadLocalSet(virThreadLocalPtr l, void *val) -{ - return TlsSetValue(l->key, val) == 0 ? -1 : 0; -} diff --git a/src/util/virthreadwin32.h b/src/util/virthreadwin32.h deleted file mode 100644 index 645031b..0000000 --- a/src/util/virthreadwin32.h +++ /dev/null @@ -1,53 +0,0 @@ -/* - * virthreadwin32.h basic thread synchronization primitives - * - * Copyright (C) 2009, 2011-2012 Red Hat, Inc. - * - * This library is free software; you can redistribute it and/or - * modify it under the terms of the GNU Lesser General Public - * License as published by the Free Software Foundation; either - * version 2.1 of the License, or (at your option) any later version. - * - * This library is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU - * Lesser General Public License for more details. - * - * You should have received a copy of the GNU Lesser General Public - * License along with this library. If not, see - * <http://www.gnu.org/licenses/>. - * - */ - -#include "internal.h" - -#ifdef HAVE_WINSOCK2_H -# include <winsock2.h> -#endif -#include <windows.h> - -struct virMutex { - HANDLE lock; -}; - -struct virCond { - virMutex lock; - size_t nwaiters; - HANDLE *waiters; -}; - -struct virThread { - HANDLE thread; - bool joinable; -}; - -struct virThreadLocal { - DWORD key; -}; - -struct virOnceControl { - volatile long init; /* 0 at startup, > 0 if init has started */ - volatile long complete; /* 0 until first thread completes callback */ -}; - -#define VIR_ONCE_CONTROL_INITIALIZER { 0, 0 } -- 1.8.4.2

On 01/23/2014 02:37 PM, Daniel P. Berrange wrote:
There are a number of pthreads impls available on Win32 these days, in particular the mingw64 project has a good impl. Delete the native windows thread implementation and rely on using pthreads everywhere.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- configure.ac | 15 +- src/Makefile.am | 4 - src/util/virthread.c | 291 +++++++++++++++++++++++++++++++- src/util/virthread.h | 41 ++++- src/util/virthreadpthread.c | 309 ---------------------------------- src/util/virthreadpthread.h | 53 ------ src/util/virthreadwin32.c | 396 -------------------------------------------- src/util/virthreadwin32.h | 53 ------ 8 files changed, 329 insertions(+), 833 deletions(-) delete mode 100644 src/util/virthreadpthread.c delete mode 100644 src/util/virthreadpthread.h delete mode 100644 src/util/virthreadwin32.c delete mode 100644 src/util/virthreadwin32.h
diff --git a/configure.ac b/configure.ac index 3a70375..168eb27 100644 --- a/configure.ac +++ b/configure.ac @@ -270,12 +270,21 @@ AC_CHECK_FUNCS_ONCE([cfmakeraw fallocate geteuid getgid getgrnam_r \ posix_memalign prlimit regexec sched_getaffinity setgroups setns \ setrlimit symlink sysctlbyname])
-dnl Availability of pthread functions (if missing, win32 threading is -dnl assumed). Because of $LIB_PTHREAD, we cannot use AC_CHECK_FUNCS_ONCE. -dnl LIB_PTHREAD and LIBMULTITHREAD were set during gl_INIT by gnulib. +dnl Availability of pthread functions. Because of $LIB_PTHREAD, we +dnl cannot use AC_CHECK_FUNCS_ONCE. LIB_PTHREAD and LIBMULTITHREAD +dnl were set during gl_INIT by gnulib. old_LIBS=$LIBS LIBS="$LIBS $LIB_PTHREAD $LIBMULTITHREAD" + +pthread_found=yes AC_CHECK_FUNCS([pthread_mutexattr_init]) +AC_CHECK_HEADER([pthread.h],,[pthread_found=no]) + +if test "$ac_cv_func_pthread_mutexattr_init:$pthread_found" != "yes:yes" +then + AC_MSG_ERROR([A pthreads impl is required for building libvirt]) +fi + dnl At least mingw64-winpthreads #defines pthread_sigmask to 0, dnl which in turn causes compilation to complain about unused variables. dnl Expose this broken implementation, so we can work around it. diff --git a/src/Makefile.am b/src/Makefile.am index 8f77658..2298fdb 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -141,8 +141,6 @@ UTIL_SOURCES = \ util/virsysinfo.c util/virsysinfo.h \ util/virsystemd.c util/virsystemd.h \ util/virthread.c util/virthread.h \ - util/virthreadpthread.h \ - util/virthreadwin32.h \ util/virthreadpool.c util/virthreadpool.h \ util/virtime.h util/virtime.c \ util/virtpm.h util/virtpm.c \ @@ -165,8 +163,6 @@ util/virkeymaps.h: $(srcdir)/util/keymaps.csv \ $(AM_V_GEN)$(PYTHON) $(srcdir)/util/virkeycode-mapgen.py \ <$(srcdir)/util/keymaps.csv >$(srcdir)/util/virkeymaps.h
-EXTRA_DIST += util/virthreadpthread.c util/virthreadwin32.c - # Internal generic driver infrastructure NODE_INFO_SOURCES = nodeinfo.h nodeinfo.c DATATYPES_SOURCES = datatypes.h datatypes.c diff --git a/src/util/virthread.c b/src/util/virthread.c index dd1768e..b60fb4a 100644 --- a/src/util/virthread.c +++ b/src/util/virthread.c @@ -23,12 +23,289 @@
#include "virthread.h"
-/* On mingw, we prefer native threading over the sometimes-broken - * pthreads-win32 library wrapper. */ -#ifdef WIN32 -# include "virthreadwin32.c" -#elif defined HAVE_PTHREAD_MUTEXATTR_INIT -# include "virthreadpthread.c" +#include <unistd.h> +#include <inttypes.h> +#if HAVE_SYS_SYSCALL_H +# include <sys/syscall.h> +#endif + +#include "viralloc.h" + + +/* Nothing special required for pthreads */ +int virThreadInitialize(void) +{ + return 0; +} + +void virThreadOnExit(void) +{ +} + +int virOnce(virOnceControlPtr once, virOnceFunc init) +{ + return pthread_once(&once->once, init); +} + + +int virMutexInit(virMutexPtr m) +{ + int ret; + pthread_mutexattr_t attr; + pthread_mutexattr_init(&attr); + pthread_mutexattr_settype(&attr, PTHREAD_MUTEX_NORMAL); + ret = pthread_mutex_init(&m->lock, &attr); + pthread_mutexattr_destroy(&attr); + if (ret != 0) { + errno = ret; + return -1; + } + return 0; +}
You just moved the contents of virthreadpthread.c into virthread.c, right? Is there maybe some way to convince git to make a shorter diff? (I did something similar in netcf - moved 90% of one file into another, and renamed the original, and it properly figured out to rename the original into the new 90%, and create the one I thought I was renaming from scratch). Not terribly important though, since we all know what really happened here :-)
diff --git a/src/util/virthread.h b/src/util/virthread.h index 7015d60..94476ee 100644 --- a/src/util/virthread.h +++ b/src/util/virthread.h @@ -25,24 +25,57 @@ # include "internal.h" # include "virerror.h"
+# include <pthread.h> + [...] + typedef struct virOnceControl virOnceControl; typedef virOnceControl *virOnceControlPtr;
+struct virOnceControl { + pthread_once_t once; +}; + + +#define VIR_ONCE_CONTROL_INITIALIZER \
This is now improperly indented. Oh, and also I just noticed the copyright date hasn't been updated. I guess as long as it builds on Windows/ming64 I give it a functional ACK. I don't know enough about problems in "certain" Windows pthreads implementations to comfortably ACK the removal of the Windows native threads code, though.

On 01/23/2014 05:37 AM, Daniel P. Berrange wrote:
There are a number of pthreads impls available on Win32 these days, in particular the mingw64 project has a good impl. Delete the native windows thread implementation and rely on using pthreads everywhere.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- configure.ac | 15 +- src/Makefile.am | 4 - src/util/virthread.c | 291 +++++++++++++++++++++++++++++++- src/util/virthread.h | 41 ++++- src/util/virthreadpthread.c | 309 ---------------------------------- src/util/virthreadpthread.h | 53 ------ src/util/virthreadwin32.c | 396 -------------------------------------------- src/util/virthreadwin32.h | 53 ------ 8 files changed, 329 insertions(+), 833 deletions(-) delete mode 100644 src/util/virthreadpthread.c delete mode 100644 src/util/virthreadpthread.h delete mode 100644 src/util/virthreadwin32.c delete mode 100644 src/util/virthreadwin32.h
This works for Fedora 20, but is it going to work on F19? What about RHEL (or does RHEL not care about mingw builds?) While I'm in favor of reducing the duplication, I'm wondering if we should still wait a bit longer before assuming winpthreads is reliably available for mingw. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On Mon, Jan 27, 2014 at 01:03:23PM -0700, Eric Blake wrote:
On 01/23/2014 05:37 AM, Daniel P. Berrange wrote:
There are a number of pthreads impls available on Win32 these days, in particular the mingw64 project has a good impl. Delete the native windows thread implementation and rely on using pthreads everywhere.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- configure.ac | 15 +- src/Makefile.am | 4 - src/util/virthread.c | 291 +++++++++++++++++++++++++++++++- src/util/virthread.h | 41 ++++- src/util/virthreadpthread.c | 309 ---------------------------------- src/util/virthreadpthread.h | 53 ------ src/util/virthreadwin32.c | 396 -------------------------------------------- src/util/virthreadwin32.h | 53 ------ 8 files changed, 329 insertions(+), 833 deletions(-) delete mode 100644 src/util/virthreadpthread.c delete mode 100644 src/util/virthreadpthread.h delete mode 100644 src/util/virthreadwin32.c delete mode 100644 src/util/virthreadwin32.h
This works for Fedora 20, but is it going to work on F19? What about RHEL (or does RHEL not care about mingw builds?) While I'm in favor of reducing the duplication, I'm wondering if we should still wait a bit longer before assuming winpthreads is reliably available for mingw.
Previous to winpthreads there was an alternative pthreads library in mingw on Fedora. We didn't use that since its header files were pretty evil and polluted the namespaces, but functionally it ought to work with libvirt. So I think we're still ok. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

The NWFilter code has as a deadlock race condition between the virNWFilter{Define,Undefine} APIs and starting of guest VMs due to mis-matched lock ordering. In the virNWFilter{Define,Undefine} codepaths the lock ordering is 1. nwfilter driver lock 2. virt driver lock 3. nwfilter update lock 4. domain object lock In the VM guest startup paths the lock ordering is 1. virt driver lock 2. domain object lock 3. nwfilter update lock As can be seen the domain object and nwfilter update locks are not acquired in a consistent order. The fix used is to push the nwfilter update lock upto the top level resulting in a lock ordering for virNWFilter{Define,Undefine} of 1. nwfilter driver lock 2. nwfilter update lock 3. virt driver lock 4. domain object lock and VM start using 1. nwfilter update lock 2. virt driver lock 3. domain object lock This has the effect of serializing VM startup once again, even if no nwfilters are applied to the guest. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/conf/nwfilter_conf.c | 6 ------ src/lxc/lxc_driver.c | 6 ++++++ src/nwfilter/nwfilter_driver.c | 8 ++++---- src/nwfilter/nwfilter_gentech_driver.c | 6 ------ src/qemu/qemu_driver.c | 6 ++++++ src/uml/uml_driver.c | 4 ++++ 6 files changed, 20 insertions(+), 16 deletions(-) diff --git a/src/conf/nwfilter_conf.c b/src/conf/nwfilter_conf.c index 6db8ea9..e712ca5 100644 --- a/src/conf/nwfilter_conf.c +++ b/src/conf/nwfilter_conf.c @@ -2990,14 +2990,12 @@ virNWFilterObjAssignDef(virNWFilterObjListPtr nwfilters, return NULL; } - virNWFilterLockFilterUpdates(); if ((nwfilter = virNWFilterObjFindByName(nwfilters, def->name))) { if (virNWFilterDefEqual(def, nwfilter->def, false)) { virNWFilterDefFree(nwfilter->def); nwfilter->def = def; - virNWFilterUnlockFilterUpdates(); return nwfilter; } @@ -3005,7 +3003,6 @@ virNWFilterObjAssignDef(virNWFilterObjListPtr nwfilters, /* trigger the update on VMs referencing the filter */ if (virNWFilterTriggerVMFilterRebuild()) { nwfilter->newDef = NULL; - virNWFilterUnlockFilterUpdates(); virNWFilterObjUnlock(nwfilter); return NULL; } @@ -3013,12 +3010,9 @@ virNWFilterObjAssignDef(virNWFilterObjListPtr nwfilters, virNWFilterDefFree(nwfilter->def); nwfilter->def = def; nwfilter->newDef = NULL; - virNWFilterUnlockFilterUpdates(); return nwfilter; } - virNWFilterUnlockFilterUpdates(); - if (VIR_ALLOC(nwfilter) < 0) return NULL; diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index e319234..b1f8a89 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -1015,6 +1015,8 @@ static int lxcDomainCreateWithFiles(virDomainPtr dom, virCheckFlags(VIR_DOMAIN_START_AUTODESTROY, -1); + virNWFilterLockFilterUpdates(); + if (!(vm = lxcDomObjFromDomain(dom))) goto cleanup; @@ -1053,6 +1055,7 @@ cleanup: if (event) virObjectEventStateQueue(driver->domainEventState, event); virObjectUnref(cfg); + virNWFilterUnlockFilterUpdates(); return ret; } @@ -1109,6 +1112,8 @@ lxcDomainCreateXMLWithFiles(virConnectPtr conn, virCheckFlags(VIR_DOMAIN_START_AUTODESTROY, NULL); + virNWFilterLockFilterUpdates(); + if (!(caps = virLXCDriverGetCapabilities(driver, false))) goto cleanup; @@ -1164,6 +1169,7 @@ cleanup: virObjectEventStateQueue(driver->domainEventState, event); virObjectUnref(caps); virObjectUnref(cfg); + virNWFilterUnlockFilterUpdates(); return dom; } diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c index d21dd82..2972731 100644 --- a/src/nwfilter/nwfilter_driver.c +++ b/src/nwfilter/nwfilter_driver.c @@ -554,6 +554,7 @@ nwfilterDefineXML(virConnectPtr conn, virNWFilterPtr ret = NULL; nwfilterDriverLock(driver); + virNWFilterLockFilterUpdates(); virNWFilterCallbackDriversLock(); if (!(def = virNWFilterDefParseString(xml))) @@ -580,6 +581,7 @@ cleanup: virNWFilterObjUnlock(nwfilter); virNWFilterCallbackDriversUnlock(); + virNWFilterUnlockFilterUpdates(); nwfilterDriverUnlock(driver); return ret; } @@ -592,9 +594,8 @@ nwfilterUndefine(virNWFilterPtr obj) { int ret = -1; nwfilterDriverLock(driver); - virNWFilterCallbackDriversLock(); - virNWFilterLockFilterUpdates(); + virNWFilterCallbackDriversLock(); nwfilter = virNWFilterObjFindByUUID(&driver->nwfilters, obj->uuid); if (!nwfilter) { @@ -626,9 +627,8 @@ cleanup: if (nwfilter) virNWFilterObjUnlock(nwfilter); - virNWFilterUnlockFilterUpdates(); - virNWFilterCallbackDriversUnlock(); + virNWFilterUnlockFilterUpdates(); nwfilterDriverUnlock(driver); return ret; } diff --git a/src/nwfilter/nwfilter_gentech_driver.c b/src/nwfilter/nwfilter_gentech_driver.c index 89913cf8..f0e78ed 100644 --- a/src/nwfilter/nwfilter_gentech_driver.c +++ b/src/nwfilter/nwfilter_gentech_driver.c @@ -935,7 +935,6 @@ _virNWFilterInstantiateFilter(virNWFilterDriverStatePtr driver, int ifindex; int rc; - virNWFilterLockFilterUpdates(); /* after grabbing the filter update lock check for the interface; if it's not there anymore its filters will be or are being removed @@ -964,7 +963,6 @@ _virNWFilterInstantiateFilter(virNWFilterDriverStatePtr driver, foundNewFilter); cleanup: - virNWFilterUnlockFilterUpdates(); return rc; } @@ -984,8 +982,6 @@ virNWFilterInstantiateFilterLate(virNWFilterDriverStatePtr driver, int rc; bool foundNewFilter = false; - virNWFilterLockFilterUpdates(); - rc = __virNWFilterInstantiateFilter(driver, vmuuid, true, @@ -1009,8 +1005,6 @@ virNWFilterInstantiateFilterLate(virNWFilterDriverStatePtr driver, } } - virNWFilterUnlockFilterUpdates(); - return rc; } diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index bc29714..2e55cfd 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1576,6 +1576,8 @@ static virDomainPtr qemuDomainCreateXML(virConnectPtr conn, if (flags & VIR_DOMAIN_START_AUTODESTROY) start_flags |= VIR_QEMU_PROCESS_START_AUTODESTROY; + virNWFilterLockFilterUpdates(); + if (!(caps = virQEMUDriverGetCapabilities(driver, false))) goto cleanup; @@ -1656,6 +1658,7 @@ cleanup: } virObjectUnref(caps); virObjectUnref(qemuCaps); + virNWFilterUnlockFilterUpdates(); return dom; } @@ -6095,6 +6098,8 @@ qemuDomainCreateWithFlags(virDomainPtr dom, unsigned int flags) VIR_DOMAIN_START_BYPASS_CACHE | VIR_DOMAIN_START_FORCE_BOOT, -1); + virNWFilterLockFilterUpdates(); + if (!(vm = qemuDomObjFromDomain(dom))) return -1; @@ -6122,6 +6127,7 @@ endjob: cleanup: if (vm) virObjectUnlock(vm); + virNWFilterUnlockFilterUpdates(); return ret; } diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c index f286f41..19b5f62 100644 --- a/src/uml/uml_driver.c +++ b/src/uml/uml_driver.c @@ -1574,6 +1574,7 @@ static virDomainPtr umlDomainCreateXML(virConnectPtr conn, const char *xml, virCheckFlags(VIR_DOMAIN_START_AUTODESTROY, NULL); + virNWFilterLockFilterUpdates(); umlDriverLock(driver); if (!(def = virDomainDefParseString(xml, driver->caps, driver->xmlopt, 1 << VIR_DOMAIN_VIRT_UML, @@ -1613,6 +1614,7 @@ cleanup: if (event) umlDomainEventQueue(driver, event); umlDriverUnlock(driver); + virNWFilterUnlockFilterUpdates(); return dom; } @@ -1997,6 +1999,7 @@ static int umlDomainCreateWithFlags(virDomainPtr dom, unsigned int flags) { virCheckFlags(VIR_DOMAIN_START_AUTODESTROY, -1); + virNWFilterLockFilterUpdates(); umlDriverLock(driver); vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); @@ -2023,6 +2026,7 @@ cleanup: if (event) umlDomainEventQueue(driver, event); umlDriverUnlock(driver); + virNWFilterUnlockFilterUpdates(); return ret; } -- 1.8.4.2

On 01/23/2014 02:37 PM, Daniel P. Berrange wrote:
The NWFilter code has as a deadlock race condition between the virNWFilter{Define,Undefine} APIs and starting of guest VMs due to mis-matched lock ordering.
In the virNWFilter{Define,Undefine} codepaths the lock ordering is
1. nwfilter driver lock 2. virt driver lock 3. nwfilter update lock 4. domain object lock
In the VM guest startup paths the lock ordering is
1. virt driver lock 2. domain object lock 3. nwfilter update lock
and previously, virNWFilterLockUpdates was called not directly from the hypervisor driver, but inside a function in the nwfilter driver that was called from the hypervisor driver's code indirectly via some sort of callback scheme.
As can be seen the domain object and nwfilter update locks are not acquired in a consistent order.
The fix used is to push the nwfilter update lock upto the top level resulting in a lock ordering for virNWFilter{Define,Undefine} of
1. nwfilter driver lock 2. nwfilter update lock 3. virt driver lock 4. domain object lock
and VM start using
1. nwfilter update lock 2. virt driver lock 3. domain object lock
This has the effect of serializing VM startup once again, even if no nwfilters are applied to the guest.
Maybe this comment should be made more prominent, so that nobody makes the mistake of applying this patch to fix the crash, but then *not* applying the two following patches that parallelize guest startups again. (Possibly even mention the followup patches by name as an extra clue). Aside from this, agreeing with your analysis, and nothing that make syntax-check and make passed for me on F20, I don't feel qualified to do a deeper review including potential unseen consequences of these changes, so hopefully Stefan will be able to do that. (I should also say in advance that I plan to review/build 4/5 and 5/5, but only to see if I can pick out some problems - for now I'll leave the ACKing up to (hopefully) Stefan.)

On 01/23/2014 07:37 AM, Daniel P. Berrange wrote:
The NWFilter code has as a deadlock race condition between the virNWFilter{Define,Undefine} APIs and starting of guest VMs due to mis-matched lock ordering.
In the virNWFilter{Define,Undefine} codepaths the lock ordering is
1. nwfilter driver lock 2. virt driver lock 3. nwfilter update lock 4. domain object lock
In the VM guest startup paths the lock ordering is
1. virt driver lock 2. domain object lock 3. nwfilter update lock
As can be seen the domain object and nwfilter update locks are not acquired in a consistent order.
The fix used is to push the nwfilter update lock upto the top level resulting in a lock ordering for virNWFilter{Define,Undefine} of
1. nwfilter driver lock 2. nwfilter update lock 3. virt driver lock 4. domain object lock
and VM start using
1. nwfilter update lock 2. virt driver lock 3. domain object lock
Makes sense... there are other paths as well: - SIGHUP or restart of libvirt that recreates all filters - late instantiation of filters following detection of VM's IP address
This has the effect of serializing VM startup once again, even if no nwfilters are applied to the guest.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/conf/nwfilter_conf.c | 6 ------ src/lxc/lxc_driver.c | 6 ++++++ src/nwfilter/nwfilter_driver.c | 8 ++++---- src/nwfilter/nwfilter_gentech_driver.c | 6 ------ src/qemu/qemu_driver.c | 6 ++++++ src/uml/uml_driver.c | 4 ++++ 6 files changed, 20 insertions(+), 16 deletions(-)
diff --git a/src/conf/nwfilter_conf.c b/src/conf/nwfilter_conf.c index 6db8ea9..e712ca5 100644 --- a/src/conf/nwfilter_conf.c +++ b/src/conf/nwfilter_conf.c @@ -2990,14 +2990,12 @@ virNWFilterObjAssignDef(virNWFilterObjListPtr nwfilters, return NULL; }
- virNWFilterLockFilterUpdates();
if ((nwfilter = virNWFilterObjFindByName(nwfilters, def->name))) {
if (virNWFilterDefEqual(def, nwfilter->def, false)) { virNWFilterDefFree(nwfilter->def); nwfilter->def = def; - virNWFilterUnlockFilterUpdates(); return nwfilter; }
I think you should add a comment to this function that it must be called with this lock held. With the removal of the locking from this function here you have to do the locking in the callers. Callers of virNWFilterObjAssignDef are: src/conf/nwfilter_conf.c::virNWFilterObjLoad : I don't see this function grabbing the lock. I think this is missing. src/conf/nwfilter_driver.c::nwfilterDefineXML : ok, found it below
diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index e319234..b1f8a89 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -1015,6 +1015,8 @@ static int lxcDomainCreateWithFiles(virDomainPtr dom,
virCheckFlags(VIR_DOMAIN_START_AUTODESTROY, -1);
+ virNWFilterLockFilterUpdates(); + if (!(vm = lxcDomObjFromDomain(dom))) goto cleanup;
@@ -1053,6 +1055,7 @@ cleanup: if (event) virObjectEventStateQueue(driver->domainEventState, event); virObjectUnref(cfg); + virNWFilterUnlockFilterUpdates(); return ret; }
@@ -1109,6 +1112,8 @@ lxcDomainCreateXMLWithFiles(virConnectPtr conn,
virCheckFlags(VIR_DOMAIN_START_AUTODESTROY, NULL);
+ virNWFilterLockFilterUpdates(); + if (!(caps = virLXCDriverGetCapabilities(driver, false))) goto cleanup;
@@ -1164,6 +1169,7 @@ cleanup: virObjectEventStateQueue(driver->domainEventState, event); virObjectUnref(caps); virObjectUnref(cfg); + virNWFilterUnlockFilterUpdates(); return dom; }
Probably these calls are correctly placed since all other creation functions funnel into them.Same for UML and QEMU drivers.
diff --git a/src/nwfilter/nwfilter_gentech_driver.c b/src/nwfilter/nwfilter_gentech_driver.c index 89913cf8..f0e78ed 100644 --- a/src/nwfilter/nwfilter_gentech_driver.c +++ b/src/nwfilter/nwfilter_gentech_driver.c @@ -935,7 +935,6 @@ _virNWFilterInstantiateFilter(virNWFilterDriverStatePtr driver, int ifindex; int rc;
- virNWFilterLockFilterUpdates();
/* after grabbing the filter update lock check for the interface; if it's not there anymore its filters will be or are being removed @@ -964,7 +963,6 @@ _virNWFilterInstantiateFilter(virNWFilterDriverStatePtr driver, foundNewFilter);
cleanup: - virNWFilterUnlockFilterUpdates();
return rc; }
This function is called by virNWFilterInstantiateFilter and virNWFilterUpdateInstantiateFilter. So, in the former case this is called when a VM is started, in the latter case if VMs' filters are updated while the VM is running. I think you are covering the VM creation case with the above calls for lxc and further below with the changes to the qemu driver and the uml driver. There is at least one other part that needs to be covered: src/conf/nwfilter_conf.c::virNWFilterInstFiltersOnAllVMs :: kicked off by nwfilterStateReload upon SIGHUP. We need a lock there now. src/conf/nwfilter_conf.c::virNWFilterTriggerVMFilterRebuild:: - called by virNWFilterTestUnassignDef: - called by src/nwfilter/nwfilter/nwfilterUndefine:: You seem to just reorder some locks there; now we need a (writer) lock there - called by virNWFilterObjAssignDef: which must be called with lock called following above reasoning
@@ -984,8 +982,6 @@ virNWFilterInstantiateFilterLate(virNWFilterDriverStatePtr driver, int rc; bool foundNewFilter = false;
- virNWFilterLockFilterUpdates(); - rc = __virNWFilterInstantiateFilter(driver, vmuuid, true, @@ -1009,8 +1005,6 @@ virNWFilterInstantiateFilterLate(virNWFilterDriverStatePtr driver, } }
- virNWFilterUnlockFilterUpdates(); - return rc; }
This function here is called by src/nwfilter/nwfilter_learnip.c::learnIPAddressThread src/nwfilter/nwfilter_dhcpsnoop.c::virNWFilterSnoopIPLeaseInstallRule They instantiate the filters once a VM's IP address has been detected. So this is where the *Late() comes from. If you remove the locking from here, you have to lock it there. Considering what you do layer, I would keep the lock here and convert this into a reader lock layer on. The farther away we do this from the calls where the actual action is happening, the more tricky this becomes. I would almost recommend to plant an assert into those far way functions that tests whether the lock has been grabbed. Regards, Stefan

On Thu, Jan 23, 2014 at 04:13:57PM -0500, Stefan Berger wrote:
On 01/23/2014 07:37 AM, Daniel P. Berrange wrote:
Makes sense... there are other paths as well:
- SIGHUP or restart of libvirt that recreates all filters - late instantiation of filters following detection of VM's IP address
Ok, yes, I see those now. I was struggling to understand just what codepaths could result in __virNWFilterInstantiateFilter being invoked so I generated a callgraph for that function http://berrange.fedorapeople.org/nwfilter.ps tracing the calls, confirms there are the 6 entry points - filter define - filter undefine - state reload on sighup or dbus notify - vm startup / hotplug - vm shutdown / hotunplug - dhcp / ip snooping the first 3 will require write locks, while the last three will only require read locks. I'm squashing the conversion to read/write lock into this patch, since otherwise the intermediate state would be deadlock prone.
src/conf/nwfilter_conf.c::virNWFilterObjLoad : I don't see this function grabbing the lock. I think this is missing.
virNWFilterObjLoad is called by StateInitialize or StateReload and only the reload function requires the lock. So I'm adding the lock to that function
diff --git a/src/nwfilter/nwfilter_gentech_driver.c b/src/nwfilter/nwfilter_gentech_driver.c index 89913cf8..f0e78ed 100644 --- a/src/nwfilter/nwfilter_gentech_driver.c +++ b/src/nwfilter/nwfilter_gentech_driver.c @@ -935,7 +935,6 @@ _virNWFilterInstantiateFilter(virNWFilterDriverStatePtr driver, int ifindex; int rc;
- virNWFilterLockFilterUpdates();
/* after grabbing the filter update lock check for the interface; if it's not there anymore its filters will be or are being removed @@ -964,7 +963,6 @@ _virNWFilterInstantiateFilter(virNWFilterDriverStatePtr driver, foundNewFilter);
cleanup: - virNWFilterUnlockFilterUpdates();
return rc; }
This function is called by virNWFilterInstantiateFilter and virNWFilterUpdateInstantiateFilter. So, in the former case this is called when a VM is started, in the latter case if VMs' filters are updated while the VM is running. I think you are covering the VM creation case with the above calls for lxc and further below with the changes to the qemu driver and the uml driver.
There is at least one other part that needs to be covered:
src/conf/nwfilter_conf.c::virNWFilterInstFiltersOnAllVMs :: kicked off by nwfilterStateReload upon SIGHUP. We need a lock there now.
src/conf/nwfilter_conf.c::virNWFilterTriggerVMFilterRebuild:: - called by virNWFilterTestUnassignDef: - called by src/nwfilter/nwfilter/nwfilterUndefine:: You seem to just reorder some locks there; now we need a (writer) lock there - called by virNWFilterObjAssignDef: which must be called with lock called following above reasoning
Yep, I concur and have added lock calls to the StateReload function
@@ -984,8 +982,6 @@ virNWFilterInstantiateFilterLate(virNWFilterDriverStatePtr driver, int rc; bool foundNewFilter = false;
- virNWFilterLockFilterUpdates(); - rc = __virNWFilterInstantiateFilter(driver, vmuuid, true, @@ -1009,8 +1005,6 @@ virNWFilterInstantiateFilterLate(virNWFilterDriverStatePtr driver, } }
- virNWFilterUnlockFilterUpdates(); - return rc; }
This function here is called by src/nwfilter/nwfilter_learnip.c::learnIPAddressThread src/nwfilter/nwfilter_dhcpsnoop.c::virNWFilterSnoopIPLeaseInstallRule
They instantiate the filters once a VM's IP address has been detected. So this is where the *Late() comes from.
If you remove the locking from here, you have to lock it there. Considering what you do layer, I would keep the lock here and convert this into a reader lock layer on.
Yes, now I'm squashing the read/write lock conversion in, I'll keep the locking in this location. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

On 01/27/2014 12:15 PM, Daniel P. Berrange wrote:
On Thu, Jan 23, 2014 at 04:13:57PM -0500, Stefan Berger wrote:
This function here is called by src/nwfilter/nwfilter_learnip.c::learnIPAddressThread src/nwfilter/nwfilter_dhcpsnoop.c::virNWFilterSnoopIPLeaseInstallRule
They instantiate the filters once a VM's IP address has been detected. So this is where the *Late() comes from.
If you remove the locking from here, you have to lock it there. Considering what you do layer, I would keep the lock here and convert this into a reader lock layer on. Yes, now I'm squashing the read/write lock conversion in, I'll keep the locking in this location.
Is there interest in introducing an assert() in those functions expecting a lock to be held? I know it's not as simple as just putting an assert() into the code. Actually we would have to record in an array or linked list the threads holding a lock and provide a function to check whether pthread_self() is holding the lock. The latter function would be called by an assert(). I would give it a shot by introducing a boolean as parameter to the lock init function that activates the recording of which threads are currently holding a lock -- if this is thought to be useful. Stefan

On Mon, Jan 27, 2014 at 01:05:51PM -0500, Stefan Berger wrote:
On 01/27/2014 12:15 PM, Daniel P. Berrange wrote:
On Thu, Jan 23, 2014 at 04:13:57PM -0500, Stefan Berger wrote:
This function here is called by src/nwfilter/nwfilter_learnip.c::learnIPAddressThread src/nwfilter/nwfilter_dhcpsnoop.c::virNWFilterSnoopIPLeaseInstallRule
They instantiate the filters once a VM's IP address has been detected. So this is where the *Late() comes from.
If you remove the locking from here, you have to lock it there. Considering what you do layer, I would keep the lock here and convert this into a reader lock layer on. Yes, now I'm squashing the read/write lock conversion in, I'll keep the locking in this location.
Is there interest in introducing an assert() in those functions expecting a lock to be held? I know it's not as simple as just putting an assert() into the code. Actually we would have to record in an array or linked list the threads holding a lock and provide a function to check whether pthread_self() is holding the lock. The latter function would be called by an assert(). I would give it a shot by introducing a boolean as parameter to the lock init function that activates the recording of which threads are currently holding a lock -- if this is thought to be useful.
Usage of assert() is forbiddenn in any libvirt code, and the complexity of tracking threads this really not something I'd want to persue. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

The nwfilter conf update mutex previously serialized updates to the internal data structures for firewall rules, and updates to the firewall itself. Since the former is going to be turned into a read/write lock instead of a mutex, a new lock is required to serialize access to the firewall itself. With this new lock, the lock ordering rules will be for virNWFilter{Define,Undefine} 1. nwfilter driver lock 2. nwfilter update lock 3. virt driver lock 4. domain object lock 5. gentech driver lock and VM start 1. nwfilter update lock 2. virt driver lock 3. domain object lock 4. gentech driver lock Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/nwfilter/nwfilter_driver.c | 4 +++- src/nwfilter/nwfilter_gentech_driver.c | 15 ++++++++++++--- src/nwfilter/nwfilter_gentech_driver.h | 2 +- 3 files changed, 16 insertions(+), 5 deletions(-) diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c index 2972731..8518e90 100644 --- a/src/nwfilter/nwfilter_driver.c +++ b/src/nwfilter/nwfilter_driver.c @@ -200,7 +200,8 @@ nwfilterStateInitialize(bool privileged, if (virNWFilterDHCPSnoopInit() < 0) goto err_exit_learnshutdown; - virNWFilterTechDriversInit(privileged); + if (virNWFilterTechDriversInit(privileged) < 0) + goto err_dhcpsnoop_shutdown; if (virNWFilterConfLayerInit(virNWFilterDomainFWUpdateCB, driverState) < 0) @@ -251,6 +252,7 @@ error: err_techdrivers_shutdown: virNWFilterTechDriversShutdown(); +err_dhcpsnoop_shutdown: virNWFilterDHCPSnoopShutdown(); err_exit_learnshutdown: virNWFilterLearnShutdown(); diff --git a/src/nwfilter/nwfilter_gentech_driver.c b/src/nwfilter/nwfilter_gentech_driver.c index f0e78ed..af7505e 100644 --- a/src/nwfilter/nwfilter_gentech_driver.c +++ b/src/nwfilter/nwfilter_gentech_driver.c @@ -55,15 +55,21 @@ static virNWFilterTechDriverPtr filter_tech_drivers[] = { NULL }; +/* Serializes execution of iptables/ip6tables/ebtables calls */ +static virMutex updateMutex; -void virNWFilterTechDriversInit(bool privileged) { +int virNWFilterTechDriversInit(bool privileged) { size_t i = 0; VIR_DEBUG("Initializing NWFilter technology drivers"); + if (virMutexInitRecursive(&updateMutex) < 0) + return -1; + while (filter_tech_drivers[i]) { if (!(filter_tech_drivers[i]->flags & TECHDRV_FLAG_INITIALIZED)) filter_tech_drivers[i]->init(privileged); i++; } + return 0; } @@ -74,6 +80,7 @@ void virNWFilterTechDriversShutdown(void) { filter_tech_drivers[i]->shutdown(); i++; } + virMutexDestroy(&updateMutex); } @@ -935,7 +942,7 @@ _virNWFilterInstantiateFilter(virNWFilterDriverStatePtr driver, int ifindex; int rc; - + virMutexLock(&updateMutex); /* after grabbing the filter update lock check for the interface; if it's not there anymore its filters will be or are being removed (while holding the lock) and we don't want to build new ones */ @@ -963,7 +970,7 @@ _virNWFilterInstantiateFilter(virNWFilterDriverStatePtr driver, foundNewFilter); cleanup: - + virMutexUnlock(&updateMutex); return rc; } @@ -982,6 +989,7 @@ virNWFilterInstantiateFilterLate(virNWFilterDriverStatePtr driver, int rc; bool foundNewFilter = false; + virMutexLock(&updateMutex); rc = __virNWFilterInstantiateFilter(driver, vmuuid, true, @@ -1005,6 +1013,7 @@ virNWFilterInstantiateFilterLate(virNWFilterDriverStatePtr driver, } } + virMutexUnlock(&updateMutex); return rc; } diff --git a/src/nwfilter/nwfilter_gentech_driver.h b/src/nwfilter/nwfilter_gentech_driver.h index f4789e1..d72e040 100644 --- a/src/nwfilter/nwfilter_gentech_driver.h +++ b/src/nwfilter/nwfilter_gentech_driver.h @@ -31,7 +31,7 @@ virNWFilterTechDriverPtr virNWFilterTechDriverForName(const char *name); int virNWFilterRuleInstAddData(virNWFilterRuleInstPtr res, void *data); -void virNWFilterTechDriversInit(bool privileged); +int virNWFilterTechDriversInit(bool privileged); void virNWFilterTechDriversShutdown(void); enum instCase { -- 1.8.4.2

On 01/23/2014 02:37 PM, Daniel P. Berrange wrote:
The nwfilter conf update mutex previously serialized updates to the internal data structures for firewall rules, and updates to the firewall itself. Since the former is going to be turned into a read/write lock instead of a mutex, a new lock is required to serialize access to the firewall itself.
Again, I agree with the analysis and theory of the change, but don't think I'm qualified to ACK. I did do a make and make syntax-check successfully (after fixing the few problems in the prior patches in the series, of course) Same comment goes for 5/5.

The nwfilter conf update mutex is held whenever a virt driver is starting a VM, or when an nwfilter define/undefine operation is taking place. As such this serializes startup of VMs which is highly undesirable. The VM startup process doesn't make changes to the nwfilter configuration, so it can be relaxed to only hold a read lock. Only the define/undefine operations need write locks. Thus using a read/write lock removes contention when starting guests, unless concurrent nwfilter updates are taking place. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/conf/nwfilter_conf.c | 17 +++++++++++------ src/conf/nwfilter_conf.h | 3 ++- src/libvirt_private.syms | 3 ++- src/lxc/lxc_driver.c | 4 ++-- src/nwfilter/nwfilter_driver.c | 4 ++-- src/qemu/qemu_driver.c | 4 ++-- src/uml/uml_driver.c | 4 ++-- 7 files changed, 23 insertions(+), 16 deletions(-) diff --git a/src/conf/nwfilter_conf.c b/src/conf/nwfilter_conf.c index e712ca5..52e1c06 100644 --- a/src/conf/nwfilter_conf.c +++ b/src/conf/nwfilter_conf.c @@ -143,17 +143,22 @@ static const struct int_map chain_priorities[] = { /* * only one filter update allowed */ -static virMutex updateMutex; +static virRWLock updateLock; static bool initialized = false; void -virNWFilterLockFilterUpdates(void) { - virMutexLock(&updateMutex); +virNWFilterReadLockFilterUpdates(void) { + virRWLockRead(&updateLock); +} + +void +virNWFilterWriteLockFilterUpdates(void) { + virRWLockWrite(&updateLock); } void virNWFilterUnlockFilterUpdates(void) { - virMutexUnlock(&updateMutex); + virRWLockUnlock(&updateLock); } @@ -3477,7 +3482,7 @@ int virNWFilterConfLayerInit(virDomainObjListIterator domUpdateCB, initialized = true; - if (virMutexInitRecursive(&updateMutex) < 0) + if (virRWLockInit(&updateLock) < 0) return -1; return 0; @@ -3489,7 +3494,7 @@ void virNWFilterConfLayerShutdown(void) if (!initialized) return; - virMutexDestroy(&updateMutex); + virRWLockDestroy(&updateLock); initialized = false; virNWFilterDomainFWUpdateOpaque = NULL; diff --git a/src/conf/nwfilter_conf.h b/src/conf/nwfilter_conf.h index 6b8b515..0d09b6a 100644 --- a/src/conf/nwfilter_conf.h +++ b/src/conf/nwfilter_conf.h @@ -716,7 +716,8 @@ virNWFilterDefPtr virNWFilterDefParseFile(const char *filename); void virNWFilterObjLock(virNWFilterObjPtr obj); void virNWFilterObjUnlock(virNWFilterObjPtr obj); -void virNWFilterLockFilterUpdates(void); +void virNWFilterWriteLockFilterUpdates(void); +void virNWFilterReadLockFilterUpdates(void); void virNWFilterUnlockFilterUpdates(void); int virNWFilterConfLayerInit(virDomainObjListIterator domUpdateCB, void *opaque); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index eb91693..a4b1c14 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -575,7 +575,6 @@ virNWFilterDefParseString; virNWFilterInstFiltersOnAllVMs; virNWFilterJumpTargetTypeToString; virNWFilterLoadAllConfigs; -virNWFilterLockFilterUpdates; virNWFilterObjAssignDef; virNWFilterObjDeleteDef; virNWFilterObjFindByName; @@ -587,6 +586,7 @@ virNWFilterObjSaveDef; virNWFilterObjUnlock; virNWFilterPrintStateMatchFlags; virNWFilterPrintTCPFlags; +virNWFilterReadLockFilterUpdates; virNWFilterRegisterCallbackDriver; virNWFilterRuleActionTypeToString; virNWFilterRuleDirectionTypeToString; @@ -594,6 +594,7 @@ virNWFilterRuleProtocolTypeToString; virNWFilterTestUnassignDef; virNWFilterUnlockFilterUpdates; virNWFilterUnRegisterCallbackDriver; +virNWFilterWriteLockFilterUpdates; # conf/nwfilter_ipaddrmap.h diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index b1f8a89..aeaa2da 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -1015,7 +1015,7 @@ static int lxcDomainCreateWithFiles(virDomainPtr dom, virCheckFlags(VIR_DOMAIN_START_AUTODESTROY, -1); - virNWFilterLockFilterUpdates(); + virNWFilterReadLockFilterUpdates(); if (!(vm = lxcDomObjFromDomain(dom))) goto cleanup; @@ -1112,7 +1112,7 @@ lxcDomainCreateXMLWithFiles(virConnectPtr conn, virCheckFlags(VIR_DOMAIN_START_AUTODESTROY, NULL); - virNWFilterLockFilterUpdates(); + virNWFilterReadLockFilterUpdates(); if (!(caps = virLXCDriverGetCapabilities(driver, false))) goto cleanup; diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c index 8518e90..f8952c9 100644 --- a/src/nwfilter/nwfilter_driver.c +++ b/src/nwfilter/nwfilter_driver.c @@ -556,7 +556,7 @@ nwfilterDefineXML(virConnectPtr conn, virNWFilterPtr ret = NULL; nwfilterDriverLock(driver); - virNWFilterLockFilterUpdates(); + virNWFilterWriteLockFilterUpdates(); virNWFilterCallbackDriversLock(); if (!(def = virNWFilterDefParseString(xml))) @@ -596,7 +596,7 @@ nwfilterUndefine(virNWFilterPtr obj) { int ret = -1; nwfilterDriverLock(driver); - virNWFilterLockFilterUpdates(); + virNWFilterWriteLockFilterUpdates(); virNWFilterCallbackDriversLock(); nwfilter = virNWFilterObjFindByUUID(&driver->nwfilters, obj->uuid); diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 2e55cfd..e246e6f 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1576,7 +1576,7 @@ static virDomainPtr qemuDomainCreateXML(virConnectPtr conn, if (flags & VIR_DOMAIN_START_AUTODESTROY) start_flags |= VIR_QEMU_PROCESS_START_AUTODESTROY; - virNWFilterLockFilterUpdates(); + virNWFilterReadLockFilterUpdates(); if (!(caps = virQEMUDriverGetCapabilities(driver, false))) goto cleanup; @@ -6098,7 +6098,7 @@ qemuDomainCreateWithFlags(virDomainPtr dom, unsigned int flags) VIR_DOMAIN_START_BYPASS_CACHE | VIR_DOMAIN_START_FORCE_BOOT, -1); - virNWFilterLockFilterUpdates(); + virNWFilterReadLockFilterUpdates(); if (!(vm = qemuDomObjFromDomain(dom))) return -1; diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c index 19b5f62..ae34a0e 100644 --- a/src/uml/uml_driver.c +++ b/src/uml/uml_driver.c @@ -1574,7 +1574,7 @@ static virDomainPtr umlDomainCreateXML(virConnectPtr conn, const char *xml, virCheckFlags(VIR_DOMAIN_START_AUTODESTROY, NULL); - virNWFilterLockFilterUpdates(); + virNWFilterReadLockFilterUpdates(); umlDriverLock(driver); if (!(def = virDomainDefParseString(xml, driver->caps, driver->xmlopt, 1 << VIR_DOMAIN_VIRT_UML, @@ -1999,7 +1999,7 @@ static int umlDomainCreateWithFlags(virDomainPtr dom, unsigned int flags) { virCheckFlags(VIR_DOMAIN_START_AUTODESTROY, -1); - virNWFilterLockFilterUpdates(); + virNWFilterReadLockFilterUpdates(); umlDriverLock(driver); vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); -- 1.8.4.2

On 01/23/2014 07:37 AM, Daniel P. Berrange wrote:
@@ -3477,7 +3482,7 @@ int virNWFilterConfLayerInit(virDomainObjListIterator domUpdateCB,
initialized = true;
- if (virMutexInitRecursive(&updateMutex) < 0) + if (virRWLockInit(&updateLock) < 0) return -1;
Obviously it was a recursive lock before. Now pthread_rwlock_wrlock() must not be called twice by the same thread. I am fine with the conversion below per se. The functions below are quite low-level now (close to virsh) that they are not called recursively. So from this perspective for this patch: ACK

On Thu, Jan 23, 2014 at 02:46:42PM -0500, Stefan Berger wrote:
On 01/23/2014 07:37 AM, Daniel P. Berrange wrote:
@@ -3477,7 +3482,7 @@ int virNWFilterConfLayerInit(virDomainObjListIterator domUpdateCB,
initialized = true;
- if (virMutexInitRecursive(&updateMutex) < 0) + if (virRWLockInit(&updateLock) < 0) return -1;
Obviously it was a recursive lock before. Now pthread_rwlock_wrlock() must not be called twice by the same thread. I am fine with the conversion below per se. The functions below are quite low-level now (close to virsh) that they are not called recursively. So from this perspective for this patch:
The write locks aren't ever acquired recursively, since they're only held in top level functions. The read locks are still allowed to be recursive so we're safe in that respect. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
participants (4)
-
Daniel P. Berrange
-
Eric Blake
-
Laine Stump
-
Stefan Berger