[libvirt] [PATCH v2 0/4] 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. See also this callgraph http://berrange.fedorapeople.org/nwfilter.ps In v2: - Re-ordered patches and squashed two together - Add missing locking in state reload function - Don't remove locking from virNWFilterInstantiateFilterLate Daniel P. Berrange (4): Add a read/write lock implementation Remove windows thread implementation in favour of pthreads Add a mutex to serialize updates to firewall Push nwfilter update locking up to top level 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 | 15 +- src/nwfilter/nwfilter_gentech_driver.c | 23 +- 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, 404 insertions(+), 823 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 ++++ src/util/virthreadwin32.c | 19 +++++++++++++++++++ src/util/virthreadwin32.h | 4 ++++ 6 files changed, 73 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index d1a58f9..10d45c3 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1816,6 +1816,11 @@ virMutexInitRecursive; virMutexLock; virMutexUnlock; virOnce; +virRWLockDestroy; +virRWLockInit; +virRWLockRead; +virRWLockUnlock; +virRWLockWrite; 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; }; diff --git a/src/util/virthreadwin32.c b/src/util/virthreadwin32.c index 5d6277f..a9e2353 100644 --- a/src/util/virthreadwin32.c +++ b/src/util/virthreadwin32.c @@ -123,6 +123,25 @@ void virMutexUnlock(virMutexPtr m) } +int virRWLockInit(virRWLockPtr m ATTRIBUTE_UNUSED) +{ + errno = ENOSYS; + return -1; +} + +void virRWLockDestroy(virRWLockPtr m ATTRIBUTE_UNUSED) +{} + + +void virRWLockRead(virRWLockPtr m ATTRIBUTE_UNUSED) +{} + +void virRWLockWrite(virRWLockPtr m ATTRIBUTE_UNUSED) +{} + + +void virRWLockUnlock(virRWLockPtr m ATTRIBUTE_UNUSED) +{} int virCondInit(virCondPtr c) { diff --git a/src/util/virthreadwin32.h b/src/util/virthreadwin32.h index 645031b..31d9444 100644 --- a/src/util/virthreadwin32.h +++ b/src/util/virthreadwin32.h @@ -30,6 +30,10 @@ struct virMutex { HANDLE lock; }; +struct virRWLock { + bool ignored; +}; + struct virCond { virMutex lock; size_t nwaiters; -- 1.8.4.2

On 01/27/2014 10:18 AM, Daniel P. Berrange wrote:
Add virRWLock backed up by a POSIX rwlock primitive
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> ---
+int virRWLockInit(virRWLockPtr m) +{ + if (pthread_rwlock_init(&m->lock, NULL) != 0) { + errno = EINVAL; + return -1;
My concern from v1 still stands - this blindly overwrites non-EINVAL errors, and better would be: int rc = pthread_rwlock_init(&m->lock, NULL); if (rc) { errno = rc; return -1; }
+void virRWLockDestroy(virRWLockPtr m) +{ + pthread_rwlock_destroy(&m->lock);
Likewise, it might be nice to add VIR_DEBUG messages when discarding a non-zero result, as a way to diagnose the programmer errors that caused that situation. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On Mon, Jan 27, 2014 at 03:45:13PM -0700, Eric Blake wrote:
On 01/27/2014 10:18 AM, Daniel P. Berrange wrote:
Add virRWLock backed up by a POSIX rwlock primitive
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> ---
+int virRWLockInit(virRWLockPtr m) +{ + if (pthread_rwlock_init(&m->lock, NULL) != 0) { + errno = EINVAL; + return -1;
My concern from v1 still stands - this blindly overwrites non-EINVAL errors, and better would be:
int rc = pthread_rwlock_init(&m->lock, NULL); if (rc) { errno = rc; return -1; }
Ah, yes, it shuld match MutexInit in this way.
+void virRWLockDestroy(virRWLockPtr m) +{ + pthread_rwlock_destroy(&m->lock);
Likewise, it might be nice to add VIR_DEBUG messages when discarding a non-zero result, as a way to diagnose the programmer errors that caused that situation.
Well we don't do that logging for any of the other Destroy calls in this threads code. If you think its useful then we should do it everywhere perhaps ? 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 :|

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 | 415 -------------------------------------------- src/util/virthreadwin32.h | 57 ------ 8 files changed, 329 insertions(+), 856 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 997899e..d541883 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..eba8dc3 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 a9e2353..0000000 --- a/src/util/virthreadwin32.c +++ /dev/null @@ -1,415 +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 virRWLockInit(virRWLockPtr m ATTRIBUTE_UNUSED) -{ - errno = ENOSYS; - return -1; -} - -void virRWLockDestroy(virRWLockPtr m ATTRIBUTE_UNUSED) -{} - - -void virRWLockRead(virRWLockPtr m ATTRIBUTE_UNUSED) -{} - -void virRWLockWrite(virRWLockPtr m ATTRIBUTE_UNUSED) -{} - - -void virRWLockUnlock(virRWLockPtr m ATTRIBUTE_UNUSED) -{} - -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 31d9444..0000000 --- a/src/util/virthreadwin32.h +++ /dev/null @@ -1,57 +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 virRWLock { - bool ignored; -}; - -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/27/2014 10:18 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> ---
I'm still a bit worried about alienating F19 or RHEL.
+if test "$ac_cv_func_pthread_mutexattr_init:$pthread_found" != "yes:yes" +then + AC_MSG_ERROR([A pthreads impl is required for building libvirt])
s/impl/implementation/ -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On Mon, Jan 27, 2014 at 03:47:19PM -0700, Eric Blake wrote:
On 01/27/2014 10:18 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> ---
I'm still a bit worried about alienating F19 or RHEL.
I think it should still work with old Win32 pthreads impl, but I'll check. 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 Mon, Jan 27, 2014 at 03:47:19PM -0700, Eric Blake wrote:
On 01/27/2014 10:18 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> ---
I'm still a bit worried about alienating F19 or RHEL.
So I tested on F19 with this series applied and hit a small problem CC util/libvirt_util_la-viraudit.lo In file included from util/virstoragefile.h:28:0, from util/virfile.h:32, from util/viraudit.c:33: util/virutil.h:118:19: error: conflicting types for 'pthread_sigmask' static inline int pthread_sigmask(int how, ^ In file included from util/viraudit.c:27:0: ../gnulib/lib/signal.h:451:1: note: previous declaration of 'pthread_sigmask' was here _GL_FUNCDECL_SYS (pthread_sigmask, int, ^ The problem seems to be our recently added configure.ac check from commit c91d13bd0ffde54905e7c5b0aedce2bb9c2b347e Author: Eric Blake <eblake@redhat.com> Date: Fri Jan 10 14:01:10 2014 -0700 build: fix build on mingw with winpthreads The F19 pthreads.h does not define any pthread_sigmask function, so our configure check fails and activates our dummy wrapper in virutil.h. Unfortunately gnulib has also detected the missing pthread_sigmask and provided its own wrapper in signal.h and thus we clash. We need to somehow make our configure.ac check detect distinguish missing pthread_sigmask from broken pthread_sigmask 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/28/2014 08:10 AM, Daniel P. Berrange wrote:
On Mon, Jan 27, 2014 at 03:47:19PM -0700, Eric Blake wrote:
On 01/27/2014 10:18 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> ---
I'm still a bit worried about alienating F19 or RHEL.
So I tested on F19 with this series applied and hit a small problem
CC util/libvirt_util_la-viraudit.lo In file included from util/virstoragefile.h:28:0, from util/virfile.h:32, from util/viraudit.c:33: util/virutil.h:118:19: error: conflicting types for 'pthread_sigmask' static inline int pthread_sigmask(int how, ^ In file included from util/viraudit.c:27:0: ../gnulib/lib/signal.h:451:1: note: previous declaration of 'pthread_sigmask' was here _GL_FUNCDECL_SYS (pthread_sigmask, int, ^
Okay, I'll fire up my F19 vm and resolve the issue (not sure yet if the solution will require another gnulib submodule update...) -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On Tue, Jan 28, 2014 at 08:12:35AM -0700, Eric Blake wrote:
On 01/28/2014 08:10 AM, Daniel P. Berrange wrote:
On Mon, Jan 27, 2014 at 03:47:19PM -0700, Eric Blake wrote:
On 01/27/2014 10:18 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> ---
I'm still a bit worried about alienating F19 or RHEL.
So I tested on F19 with this series applied and hit a small problem
CC util/libvirt_util_la-viraudit.lo In file included from util/virstoragefile.h:28:0, from util/virfile.h:32, from util/viraudit.c:33: util/virutil.h:118:19: error: conflicting types for 'pthread_sigmask' static inline int pthread_sigmask(int how, ^ In file included from util/viraudit.c:27:0: ../gnulib/lib/signal.h:451:1: note: previous declaration of 'pthread_sigmask' was here _GL_FUNCDECL_SYS (pthread_sigmask, int, ^
Okay, I'll fire up my F19 vm and resolve the issue (not sure yet if the solution will require another gnulib submodule update...)
Actually I think it is easy to fix - we just look at ac_cv_func_pthread_sigmask which gnulib will have already set ie @@ -288,18 +288,21 @@ 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. -AC_CACHE_CHECK([whether pthread_sigmask does anything], - [lv_cv_pthread_sigmask_works], - [AC_COMPILE_IFELSE([AC_LANG_PROGRAM([[ - #include <sys/types.h> - #include <signal.h> - ]], [[ - int (*foo)(int, const sigset_t *, sigset_t *) = &pthread_sigmask; - return !foo; - ]])], [lv_cv_pthread_sigmask_works=yes], [lv_cv_pthread_sigmask_works=no])]) -if test "x$lv_cv_pthread_sigmask_works" != xyes; then +if test $ac_cv_func_pthread_sigmask = yes +then + AC_CACHE_CHECK([whether pthread_sigmask does anything], + [lv_cv_pthread_sigmask_works], + [AC_COMPILE_IFELSE([AC_LANG_PROGRAM([[ + #include <sys/types.h> + #include <signal.h> + ]], [[ + int (*foo)(int, const sigset_t *, sigset_t *) = &pthread_sigmask; + return !foo; + ]])], [lv_cv_pthread_sigmask_works=yes], [lv_cv_pthread_sigmask_works=no])]) + if test "x$lv_cv_pthread_sigmask_works" != xyes; then AC_DEFINE([FUNC_PTHREAD_SIGMASK_BROKEN], [1], [Define to 1 if pthread_sigmask is not a real function]) + fi fi LIBS=$old_libs 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 | 19 +++++++++++++++++-- src/nwfilter/nwfilter_gentech_driver.h | 2 +- 3 files changed, 21 insertions(+), 4 deletions(-) diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c index d21dd82..112e8cb 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 89913cf8..d500963 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); } @@ -936,6 +943,7 @@ _virNWFilterInstantiateFilter(virNWFilterDriverStatePtr driver, int rc; virNWFilterLockFilterUpdates(); + 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 @@ -965,6 +973,7 @@ _virNWFilterInstantiateFilter(virNWFilterDriverStatePtr driver, cleanup: virNWFilterUnlockFilterUpdates(); + virMutexUnlock(&updateMutex); return rc; } @@ -985,6 +994,7 @@ virNWFilterInstantiateFilterLate(virNWFilterDriverStatePtr driver, bool foundNewFilter = false; virNWFilterLockFilterUpdates(); + virMutexLock(&updateMutex); rc = __virNWFilterInstantiateFilter(driver, vmuuid, @@ -1010,6 +1020,7 @@ virNWFilterInstantiateFilterLate(virNWFilterDriverStatePtr driver, } virNWFilterUnlockFilterUpdates(); + virMutexUnlock(&updateMutex); return rc; } @@ -1133,7 +1144,11 @@ _virNWFilterTeardownFilter(const char *ifname) int virNWFilterTeardownFilter(const virDomainNetDef *net) { - return _virNWFilterTeardownFilter(net->ifname); + int ret; + virMutexLock(&updateMutex); + ret = _virNWFilterTeardownFilter(net->ifname); + virMutexUnlock(&updateMutex); + return ret; } 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/27/2014 12:18 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
Hm, wasn't aware (anymore) of this double-purpose. I also hadn't looked at this patch in the first round...
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
Insert: 3. nwfilter callback drivers lock This is then used in this order also by nwfilterStateReload
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 | 19 +++++++++++++++++-- src/nwfilter/nwfilter_gentech_driver.h | 2 +- 3 files changed, 21 insertions(+), 4 deletions(-)
diff --git a/src/nwfilter/nwfilter_gentech_driver.c b/src/nwfilter/nwfilter_gentech_driver.c index 89913cf8..d500963 100644 --- a/src/nwfilter/nwfilter_gentech_driver.c +++ b/src/nwfilter/nwfilter_gentech_driver.c @@ -936,6 +943,7 @@ _virNWFilterInstantiateFilter(virNWFilterDriverStatePtr driver, int rc;
virNWFilterLockFilterUpdates(); + virMutexLock(&updateMutex);
Since the filter updates lock had the two purposes before, you are now introducing a separate lock to assign a purpose to each lock. Further below you are preventing concurrent teardowns to this here. I am wondering how much further down this lock here could actually be pushed. This and the other function (virNWFilterInstantiateFilterLate) where you place this lock are calling __virNWFilterInstantiateFilter and nothing else calls that function [and the filter read protection above the lock call will remain]. So I think this lock could be placed inside __virNWFilterInstantiateFilter(). Also looking at that function I am not sure whether there is anything worth protecting using this newly introduced lock then. It ends up calling virNWFilterInstantiate(). Here I would be a bit careful with the threads being started to learn the IP addresses. So maybe this function could be the place where to serialize access. What's your take? Stefan

On Mon, Jan 27, 2014 at 04:50:56PM -0500, Stefan Berger wrote:
On 01/27/2014 12:18 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
Hm, wasn't aware (anymore) of this double-purpose.
It wasn't entirely clear to me either, but I am right in believing that it isn't safe to have multiple threads all creating iptables rules for different VMs at the same time, aren't I ?
I also hadn't looked at this patch in the first round...
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
Insert: 3. nwfilter callback drivers lock
This is then used in this order also by nwfilterStateReload
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 | 19 +++++++++++++++++-- src/nwfilter/nwfilter_gentech_driver.h | 2 +- 3 files changed, 21 insertions(+), 4 deletions(-)
diff --git a/src/nwfilter/nwfilter_gentech_driver.c b/src/nwfilter/nwfilter_gentech_driver.c index 89913cf8..d500963 100644 --- a/src/nwfilter/nwfilter_gentech_driver.c +++ b/src/nwfilter/nwfilter_gentech_driver.c @@ -936,6 +943,7 @@ _virNWFilterInstantiateFilter(virNWFilterDriverStatePtr driver, int rc;
virNWFilterLockFilterUpdates(); + virMutexLock(&updateMutex);
Since the filter updates lock had the two purposes before, you are now introducing a separate lock to assign a purpose to each lock. Further below you are preventing concurrent teardowns to this here.
I am wondering how much further down this lock here could actually be pushed. This and the other function (virNWFilterInstantiateFilterLate) where you place this lock are calling __virNWFilterInstantiateFilter and nothing else calls that function [and the filter read protection above the lock call will remain]. So I think this lock could be placed inside __virNWFilterInstantiateFilter(). Also looking at that function I am not sure whether there is anything worth protecting using this newly introduced lock then. It ends up calling virNWFilterInstantiate(). Here I would be a bit careful with the threads being started to learn the IP addresses. So maybe this function could be the place where to serialize access. What's your take?
The callgraph showed the three entry points into this area of code look like: virNWFilterInstantiateFilterLate virNWFilterInstantiateFilter virNWFilterTeardownFilter | | | +-------------------------|-----+ | | +------------+ | | | | | | V V V V _virNWFilterInstantiateFilter _virNWFilterTeardownFilter Looking at the code I don't see how it is safe to allow teardown to happen in parallel with instantiate. The teardown code could confuse and conflict with the instantiate code causing it to fail I believe. In particular a VM could exit causing QEMU to request teardown, while the DHCP snoop thread is in the middle of re-creating the iptables ruleset for a VM, so we surely require serialization of modifications to iptables. I could, push the locking down one level, but it wouldn't change the level of serialization, and the lock calls are clearer at the level of the code I have them I believe. 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/28/2014 06:15 AM, Daniel P. Berrange wrote:
On Mon, Jan 27, 2014 at 04:50:56PM -0500, Stefan Berger wrote:
On 01/27/2014 12:18 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 Hm, wasn't aware (anymore) of this double-purpose. It wasn't entirely clear to me either, but I am right in believing that it isn't safe to have multiple threads all creating iptables rules for different VMs at the same time, aren't I ?
At least one thread shouldn't try to instantiate filters for one (VM) interface while another one tears them down. So that would be serialization per interface. I think instantiation of filters could be done concurrently for different interfaces, but not the execution of the iptables commands themselves, though. The latter is locking that needs to be done by the ebtables/iptables driver and that driver does serialize the execution of all scripts using the execCLIMutex. The ebtables and iptables rules are created on a per-interface basis, all hooking into some form of 'root' chains. The critical part here is that the 'root' chains can be accessed concurrently by different interfaces -- from what I can tell is that all the scripts that are run by the eb/iptables driver only need to be serialized and that is done with that execCLIMutex. So we should be fine from that perspective. At least locking on a per-interface basis is already happening in the 'gentech' driver: nwfilter_gentech_driver.c::virNWFilterInstantiate [...] if (virNWFilterLockIface(ifname) < 0) goto err_exit; rc = techdriver->applyNewRules(ifname, nptrs, ptrs); if (teardownOld && rc == 0) techdriver->tearOldRules(ifname); if (rc == 0 && (virNetDevValidateConfig(ifname, NULL, ifindex) <= 0)) { virResetLastError(); /* interface changed/disppeared */ techdriver->allTeardown(ifname); rc = -1; } virNWFilterUnlockIface(ifname); [...] nwfilter_gentech_driver.c::_virNWFilterTeardownFilter [...] if (virNWFilterLockIface(ifname) < 0) return -1; techdriver->allTeardown(ifname); virNWFilterIPAddrMapDelIPAddr(ifname, NULL); virNWFilterUnlockIface(ifname); [...] (Besides the above calls to the 'techdriver', there are others that call into the techdriver during the test phases of a filter updated. They hold the writer lock to the filter updates and with this block every concurrent thread then.) I may be missing something subtle, but I think there is already enough serialization happening per interface.
I also hadn't looked at this patch in the first round...
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
Insert: 3. nwfilter callback drivers lock
This is then used in this order also by nwfilterStateReload
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 | 19 +++++++++++++++++-- src/nwfilter/nwfilter_gentech_driver.h | 2 +- 3 files changed, 21 insertions(+), 4 deletions(-)
diff --git a/src/nwfilter/nwfilter_gentech_driver.c b/src/nwfilter/nwfilter_gentech_driver.c index 89913cf8..d500963 100644 --- a/src/nwfilter/nwfilter_gentech_driver.c +++ b/src/nwfilter/nwfilter_gentech_driver.c @@ -936,6 +943,7 @@ _virNWFilterInstantiateFilter(virNWFilterDriverStatePtr driver, int rc;
virNWFilterLockFilterUpdates(); + virMutexLock(&updateMutex);
Since the filter updates lock had the two purposes before, you are now introducing a separate lock to assign a purpose to each lock. Further below you are preventing concurrent teardowns to this here.
I am wondering how much further down this lock here could actually be pushed. This and the other function (virNWFilterInstantiateFilterLate) where you place this lock are calling __virNWFilterInstantiateFilter and nothing else calls that function [and the filter read protection above the lock call will remain]. So I think this lock could be placed inside __virNWFilterInstantiateFilter(). Also looking at that function I am not sure whether there is anything worth protecting using this newly introduced lock then. It ends up calling virNWFilterInstantiate(). Here I would be a bit careful with the threads being started to learn the IP addresses. So maybe this function could be the place where to serialize access. What's your take? The callgraph showed the three entry points into this area of code look like:
virNWFilterInstantiateFilterLate virNWFilterInstantiateFilter virNWFilterTeardownFilter | | | +-------------------------|-----+ | | +------------+ | | | | | | V V V V _virNWFilterInstantiateFilter _virNWFilterTeardownFilter
Looking at the code I don't see how it is safe to allow teardown to happen in parallel with instantiate. The teardown code could confuse and conflict with the instantiate code causing it to fail I believe.
I agree. These two need to per serialized. But do they need to be serialized on a per-interface basis only or as a whole? In the latter case I think a more global lock should now go into the ebtables/iptables driver rather than this one here, but I think it wouldn't be necessary.
In particular a VM could exit causing QEMU to request teardown, while the DHCP snoop thread is in the middle of re-creating the iptables ruleset for a VM, so we surely require serialization of modifications to iptables.
I could, push the locking down one level, but it wouldn't change the level of serialization, and the lock calls are clearer at the level of the code I have them I believe.
Let me look at the locking for a bit and I'll try to get back to you as soon as I can. Stefan

On Tue, Jan 28, 2014 at 07:31:04AM -0500, Stefan Berger wrote:
On 01/28/2014 06:15 AM, Daniel P. Berrange wrote:
On Mon, Jan 27, 2014 at 04:50:56PM -0500, Stefan Berger wrote:
On 01/27/2014 12:18 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 Hm, wasn't aware (anymore) of this double-purpose. It wasn't entirely clear to me either, but I am right in believing that it isn't safe to have multiple threads all creating iptables rules for different VMs at the same time, aren't I ?
At least one thread shouldn't try to instantiate filters for one (VM) interface while another one tears them down. So that would be serialization per interface. I think instantiation of filters could be done concurrently for different interfaces, but not the execution of the iptables commands themselves, though. The latter is locking that needs to be done by the ebtables/iptables driver and that driver does serialize the execution of all scripts using the execCLIMutex. The ebtables and iptables rules are created on a per-interface basis, all hooking into some form of 'root' chains. The critical part here is that the 'root' chains can be accessed concurrently by different interfaces -- from what I can tell is that all the scripts that are run by the eb/iptables driver only need to be serialized and that is done with that execCLIMutex. So we should be fine from that perspective.
At least locking on a per-interface basis is already happening in the 'gentech' driver:
nwfilter_gentech_driver.c::virNWFilterInstantiate
[...] if (virNWFilterLockIface(ifname) < 0) goto err_exit;
rc = techdriver->applyNewRules(ifname, nptrs, ptrs);
if (teardownOld && rc == 0) techdriver->tearOldRules(ifname);
if (rc == 0 && (virNetDevValidateConfig(ifname, NULL, ifindex) <= 0)) { virResetLastError(); /* interface changed/disppeared */ techdriver->allTeardown(ifname); rc = -1; }
virNWFilterUnlockIface(ifname); [...]
nwfilter_gentech_driver.c::_virNWFilterTeardownFilter
[...] if (virNWFilterLockIface(ifname) < 0) return -1;
techdriver->allTeardown(ifname);
virNWFilterIPAddrMapDelIPAddr(ifname, NULL);
virNWFilterUnlockIface(ifname); [...]
(Besides the above calls to the 'techdriver', there are others that call into the techdriver during the test phases of a filter updated. They hold the writer lock to the filter updates and with this block every concurrent thread then.)
I may be missing something subtle, but I think there is already enough serialization happening per interface.
Ok, I think you might be right then, and we can just skip this patch entirely and rely in the ifname locks. 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/28/2014 07:38 AM, Daniel P. Berrange wrote:
On Mon, Jan 27, 2014 at 04:50:56PM -0500, Stefan Berger wrote:
On 01/27/2014 12:18 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 Hm, wasn't aware (anymore) of this double-purpose. It wasn't entirely clear to me either, but I am right in believing that it isn't safe to have multiple threads all creating iptables rules for different VMs at the same time, aren't I ? At least one thread shouldn't try to instantiate filters for one (VM) interface while another one tears them down. So that would be serialization per interface. I think instantiation of filters could be done concurrently for different interfaces, but not the execution of the iptables commands themselves, though. The latter is locking
On 01/28/2014 06:15 AM, Daniel P. Berrange wrote: that needs to be done by the ebtables/iptables driver and that driver does serialize the execution of all scripts using the execCLIMutex. The ebtables and iptables rules are created on a per-interface basis, all hooking into some form of 'root' chains. The critical part here is that the 'root' chains can be accessed concurrently by different interfaces -- from what I can tell is that all the scripts that are run by the eb/iptables driver only need to be serialized and that is done with that execCLIMutex. So we should be fine from that perspective.
At least locking on a per-interface basis is already happening in the 'gentech' driver:
nwfilter_gentech_driver.c::virNWFilterInstantiate
[...] if (virNWFilterLockIface(ifname) < 0) goto err_exit;
rc = techdriver->applyNewRules(ifname, nptrs, ptrs);
if (teardownOld && rc == 0) techdriver->tearOldRules(ifname);
if (rc == 0 && (virNetDevValidateConfig(ifname, NULL, ifindex) <= 0)) { virResetLastError(); /* interface changed/disppeared */ techdriver->allTeardown(ifname); rc = -1; }
virNWFilterUnlockIface(ifname); [...]
nwfilter_gentech_driver.c::_virNWFilterTeardownFilter
[...] if (virNWFilterLockIface(ifname) < 0) return -1;
techdriver->allTeardown(ifname);
virNWFilterIPAddrMapDelIPAddr(ifname, NULL);
virNWFilterUnlockIface(ifname); [...]
(Besides the above calls to the 'techdriver', there are others that call into the techdriver during the test phases of a filter updated. They hold the writer lock to the filter updates and with this block every concurrent thread then.)
I may be missing something subtle, but I think there is already enough serialization happening per interface. Ok, I think you might be right then, and we can just skip this
On Tue, Jan 28, 2014 at 07:31:04AM -0500, Stefan Berger wrote: patch entirely and rely in the ifname locks.
I definitely hope so. Regards, Stefan
Daniel

On 01/27/2014 10:18 AM, 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.
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> ---
Is it worth adding a file describing the lock rules? Comparable to qemu/THREADS.txt? -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

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. There is also the possibility of deadlock due to a call graph loop via virNWFilterInstantiate and virNWFilterInstantiateFilterLate. These two problems mean the lock must be turned into a read/write lock instead of a plain mutex at the same time. The lock is used to serialize changes to the "driver->nwfilters" hash, so the write lock only needs to be held by the define/undefine methods. All other methods can rely on a read lock which allows good concurrency. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/conf/nwfilter_conf.c | 23 +++++++++++------------ src/conf/nwfilter_conf.h | 3 ++- src/libvirt_private.syms | 3 ++- src/lxc/lxc_driver.c | 6 ++++++ src/nwfilter/nwfilter_driver.c | 11 +++++++---- src/nwfilter/nwfilter_gentech_driver.c | 4 +--- src/qemu/qemu_driver.c | 6 ++++++ src/uml/uml_driver.c | 4 ++++ 8 files changed, 39 insertions(+), 21 deletions(-) diff --git a/src/conf/nwfilter_conf.c b/src/conf/nwfilter_conf.c index 6db8ea9..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); } @@ -2990,14 +2995,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 +3008,6 @@ virNWFilterObjAssignDef(virNWFilterObjListPtr nwfilters, /* trigger the update on VMs referencing the filter */ if (virNWFilterTriggerVMFilterRebuild()) { nwfilter->newDef = NULL; - virNWFilterUnlockFilterUpdates(); virNWFilterObjUnlock(nwfilter); return NULL; } @@ -3013,12 +3015,9 @@ virNWFilterObjAssignDef(virNWFilterObjListPtr nwfilters, virNWFilterDefFree(nwfilter->def); nwfilter->def = def; nwfilter->newDef = NULL; - virNWFilterUnlockFilterUpdates(); return nwfilter; } - virNWFilterUnlockFilterUpdates(); - if (VIR_ALLOC(nwfilter) < 0) return NULL; @@ -3483,7 +3482,7 @@ int virNWFilterConfLayerInit(virDomainObjListIterator domUpdateCB, initialized = true; - if (virMutexInitRecursive(&updateMutex) < 0) + if (virRWLockInit(&updateLock) < 0) return -1; return 0; @@ -3495,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 10d45c3..2e293d1 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 e319234..aeaa2da 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); + virNWFilterReadLockFilterUpdates(); + 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); + virNWFilterReadLockFilterUpdates(); + 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 112e8cb..80030c8 100644 --- a/src/nwfilter/nwfilter_driver.c +++ b/src/nwfilter/nwfilter_driver.c @@ -285,12 +285,15 @@ nwfilterStateReload(void) virNWFilterLearnThreadsTerminate(true); nwfilterDriverLock(driverState); + virNWFilterWriteLockFilterUpdates(); virNWFilterCallbackDriversLock(); + virNWFilterLoadAllConfigs(&driverState->nwfilters, driverState->configDir); virNWFilterCallbackDriversUnlock(); + virNWFilterUnlockFilterUpdates(); nwfilterDriverUnlock(driverState); virNWFilterInstFiltersOnAllVMs(); @@ -556,6 +559,7 @@ nwfilterDefineXML(virConnectPtr conn, virNWFilterPtr ret = NULL; nwfilterDriverLock(driver); + virNWFilterWriteLockFilterUpdates(); virNWFilterCallbackDriversLock(); if (!(def = virNWFilterDefParseString(xml))) @@ -582,6 +586,7 @@ cleanup: virNWFilterObjUnlock(nwfilter); virNWFilterCallbackDriversUnlock(); + virNWFilterUnlockFilterUpdates(); nwfilterDriverUnlock(driver); return ret; } @@ -594,10 +599,9 @@ nwfilterUndefine(virNWFilterPtr obj) { int ret = -1; nwfilterDriverLock(driver); + virNWFilterWriteLockFilterUpdates(); virNWFilterCallbackDriversLock(); - virNWFilterLockFilterUpdates(); - nwfilter = virNWFilterObjFindByUUID(&driver->nwfilters, obj->uuid); if (!nwfilter) { virReportError(VIR_ERR_NO_NWFILTER, @@ -628,9 +632,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 d500963..b133e21 100644 --- a/src/nwfilter/nwfilter_gentech_driver.c +++ b/src/nwfilter/nwfilter_gentech_driver.c @@ -942,7 +942,6 @@ _virNWFilterInstantiateFilter(virNWFilterDriverStatePtr driver, int ifindex; int rc; - virNWFilterLockFilterUpdates(); virMutexLock(&updateMutex); /* after grabbing the filter update lock check for the interface; if @@ -972,7 +971,6 @@ _virNWFilterInstantiateFilter(virNWFilterDriverStatePtr driver, foundNewFilter); cleanup: - virNWFilterUnlockFilterUpdates(); virMutexUnlock(&updateMutex); return rc; @@ -993,7 +991,7 @@ virNWFilterInstantiateFilterLate(virNWFilterDriverStatePtr driver, int rc; bool foundNewFilter = false; - virNWFilterLockFilterUpdates(); + virNWFilterReadLockFilterUpdates(); virMutexLock(&updateMutex); rc = __virNWFilterInstantiateFilter(driver, diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index bc29714..e246e6f 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; + virNWFilterReadLockFilterUpdates(); + 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); + virNWFilterReadLockFilterUpdates(); + 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..ae34a0e 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); + virNWFilterReadLockFilterUpdates(); 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); + virNWFilterReadLockFilterUpdates(); 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/27/2014 12:18 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
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
Insert: 3. nwfilter callback drivers lock This is then used in this order also by nwfilterStateReload
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. There is also the possibility of deadlock due to a call graph loop via virNWFilterInstantiate and virNWFilterInstantiateFilterLate.
These two problems mean the lock must be turned into a read/write lock instead of a plain mutex at the same time. The lock is used to serialize changes to the "driver->nwfilters" hash, so the write lock only needs to be held by the define/undefine methods. All other methods can rely on a read lock which allows good concurrency.
Signed-off-by: Daniel P. Berrange<berrange@redhat.com> --- src/conf/nwfilter_conf.c | 23 +++++++++++------------ src/conf/nwfilter_conf.h | 3 ++- src/libvirt_private.syms | 3 ++- src/lxc/lxc_driver.c | 6 ++++++ src/nwfilter/nwfilter_driver.c | 11 +++++++---- src/nwfilter/nwfilter_gentech_driver.c | 4 +--- src/qemu/qemu_driver.c | 6 ++++++ src/uml/uml_driver.c | 4 ++++ 8 files changed, 39 insertions(+), 21 deletions(-)
diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c index 112e8cb..80030c8 100644 --- a/src/nwfilter/nwfilter_driver.c +++ b/src/nwfilter/nwfilter_driver.c @@ -285,12 +285,15 @@ nwfilterStateReload(void) virNWFilterLearnThreadsTerminate(true);
nwfilterDriverLock(driverState); + virNWFilterWriteLockFilterUpdates(); virNWFilterCallbackDriversLock();
+
stray newline here
virNWFilterLoadAllConfigs(&driverState->nwfilters, driverState->configDir);
virNWFilterCallbackDriversUnlock(); + virNWFilterUnlockFilterUpdates(); nwfilterDriverUnlock(driverState);
virNWFilterInstFiltersOnAllVMs();
Modulo the above stray line and then comment in the introductory text, I think this patch is good. I am just a bit concerned about 3/4. Stefan

On Mon, Jan 27, 2014 at 04:53:53PM -0500, Stefan Berger wrote:
On 01/27/2014 12:18 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
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
Insert: 3. nwfilter callback drivers lock
When I say "virt driver lock" here I'm refering to the fact that the nwfilter callback drivers lock hook is basically just calling the virt driver lock
This is then used in this order also by nwfilterStateReload
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. There is also the possibility of deadlock due to a call graph loop via virNWFilterInstantiate and virNWFilterInstantiateFilterLate.
These two problems mean the lock must be turned into a read/write lock instead of a plain mutex at the same time. The lock is used to serialize changes to the "driver->nwfilters" hash, so the write lock only needs to be held by the define/undefine methods. All other methods can rely on a read lock which allows good concurrency.
Signed-off-by: Daniel P. Berrange<berrange@redhat.com> --- src/conf/nwfilter_conf.c | 23 +++++++++++------------ src/conf/nwfilter_conf.h | 3 ++- src/libvirt_private.syms | 3 ++- src/lxc/lxc_driver.c | 6 ++++++ src/nwfilter/nwfilter_driver.c | 11 +++++++---- src/nwfilter/nwfilter_gentech_driver.c | 4 +--- src/qemu/qemu_driver.c | 6 ++++++ src/uml/uml_driver.c | 4 ++++ 8 files changed, 39 insertions(+), 21 deletions(-)
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:18 PM, Daniel P. Berrange wrote:
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.
See also this callgraph
What tool do you use to render this? Ghostview only shows me part of the content. Stefan

On Mon, Jan 27, 2014 at 03:23:29PM -0500, Stefan Berger wrote:
On 01/27/2014 12:18 PM, Daniel P. Berrange wrote:
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.
See also this callgraph
What tool do you use to render this? Ghostview only shows me part of the content.
evince renders the whole thing nicely. 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 (3)
-
Daniel P. Berrange
-
Eric Blake
-
Stefan Berger