From c2f0d52b7fb937f3f66ef8b2b74b0fdf239b0e9b Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Sun, 22 Sep 2013 21:30:46 -0400 Subject: [PATCH 01/32] Split threading-related code out of compat.c Also, re-enable the #if'd out condition-variable code. Work queues are going to make us hack on all of this stuff a bit more closely, so it might not be a terrible idea to make it easier to hack. --- configure.ac | 4 + src/common/compat.c | 394 --------------------------------- src/common/compat.h | 57 +---- src/common/compat_pthreads.c | 211 ++++++++++++++++++ src/common/compat_threads.c | 42 ++++ src/common/compat_threads.h | 67 ++++++ src/common/compat_winthreads.c | 161 ++++++++++++++ src/common/include.am | 13 +- 8 files changed, 499 insertions(+), 450 deletions(-) create mode 100644 src/common/compat_pthreads.c create mode 100644 src/common/compat_threads.c create mode 100644 src/common/compat_threads.h create mode 100644 src/common/compat_winthreads.c diff --git a/configure.ac b/configure.ac index c254725c60..65b3ff245c 100644 --- a/configure.ac +++ b/configure.ac @@ -393,6 +393,10 @@ fi AC_SEARCH_LIBS(pthread_create, [pthread]) AC_SEARCH_LIBS(pthread_detach, [pthread]) +AM_CONDITIONAL(THREADS_WIN32, test "$enable_threads" = "yes" && test "$bwin32" = "true") +AM_CONDITIONAL(THREADS_PTHREADS, test "$enable_threads" = "yes" && test "$bwin32" = "false") +AM_CONDITIONAL(THREADS_NONE, test "$enable_threads" != "yes") + dnl ------------------------------------------------------------------- dnl Check for functions before libevent, since libevent-1.2 apparently dnl exports strlcpy without defining it in a header. diff --git a/src/common/compat.c b/src/common/compat.c index 6d36321193..a22a61ac4d 100644 --- a/src/common/compat.c +++ b/src/common/compat.c @@ -2544,109 +2544,6 @@ get_uname(void) * Process control */ -#if defined(USE_PTHREADS) -/** Wraps a void (*)(void*) function and its argument so we can - * invoke them in a way pthreads would expect. - */ -typedef struct tor_pthread_data_t { - void (*func)(void *); - void *data; -} tor_pthread_data_t; -/** Given a tor_pthread_data_t _data, call _data->func(d->data) - * and free _data. Used to make sure we can call functions the way pthread - * expects. */ -static void * -tor_pthread_helper_fn(void *_data) -{ - tor_pthread_data_t *data = _data; - void (*func)(void*); - void *arg; - /* mask signals to worker threads to avoid SIGPIPE, etc */ - sigset_t sigs; - /* We're in a subthread; don't handle any signals here. */ - sigfillset(&sigs); - pthread_sigmask(SIG_SETMASK, &sigs, NULL); - - func = data->func; - arg = data->data; - tor_free(_data); - func(arg); - return NULL; -} -/** - * A pthread attribute to make threads start detached. - */ -static pthread_attr_t attr_detached; -/** True iff we've called tor_threads_init() */ -static int threads_initialized = 0; -#endif - -/** Minimalist interface to run a void function in the background. On - * Unix calls fork, on win32 calls beginthread. Returns -1 on failure. - * func should not return, but rather should call spawn_exit. - * - * NOTE: if data is used, it should not be allocated on the stack, - * since in a multithreaded environment, there is no way to be sure that - * the caller's stack will still be around when the called function is - * running. - */ -int -spawn_func(void (*func)(void *), void *data) -{ -#if defined(USE_WIN32_THREADS) - int rv; - rv = (int)_beginthread(func, 0, data); - if (rv == (int)-1) - return -1; - return 0; -#elif defined(USE_PTHREADS) - pthread_t thread; - tor_pthread_data_t *d; - if (PREDICT_UNLIKELY(!threads_initialized)) - tor_threads_init(); - d = tor_malloc(sizeof(tor_pthread_data_t)); - d->data = data; - d->func = func; - if (pthread_create(&thread,&attr_detached,tor_pthread_helper_fn,d)) - return -1; - return 0; -#else - pid_t pid; - pid = fork(); - if (pid<0) - return -1; - if (pid==0) { - /* Child */ - func(data); - tor_assert(0); /* Should never reach here. */ - return 0; /* suppress "control-reaches-end-of-non-void" warning. */ - } else { - /* Parent */ - return 0; - } -#endif -} - -/** End the current thread/process. - */ -void -spawn_exit(void) -{ -#if defined(USE_WIN32_THREADS) - _endthread(); - //we should never get here. my compiler thinks that _endthread returns, this - //is an attempt to fool it. - tor_assert(0); - _exit(0); -#elif defined(USE_PTHREADS) - pthread_exit(NULL); -#else - /* http://www.erlenstar.demon.co.uk/unix/faq_2.html says we should - * call _exit, not exit, from child processes. */ - _exit(0); -#endif -} - /** Implementation logic for compute_num_cpus(). */ static int compute_num_cpus_impl(void) @@ -2935,280 +2832,6 @@ tor_gmtime_r(const time_t *timep, struct tm *result) } #endif -#if defined(USE_WIN32_THREADS) -void -tor_mutex_init(tor_mutex_t *m) -{ - InitializeCriticalSection(&m->mutex); -} -void -tor_mutex_uninit(tor_mutex_t *m) -{ - DeleteCriticalSection(&m->mutex); -} -void -tor_mutex_acquire(tor_mutex_t *m) -{ - tor_assert(m); - EnterCriticalSection(&m->mutex); -} -void -tor_mutex_release(tor_mutex_t *m) -{ - LeaveCriticalSection(&m->mutex); -} -unsigned long -tor_get_thread_id(void) -{ - return (unsigned long)GetCurrentThreadId(); -} -#elif defined(USE_PTHREADS) -/** A mutex attribute that we're going to use to tell pthreads that we want - * "reentrant" mutexes (i.e., once we can re-lock if we're already holding - * them.) */ -static pthread_mutexattr_t attr_reentrant; -/** Initialize mutex so it can be locked. Every mutex must be set - * up with tor_mutex_init() or tor_mutex_new(); not both. */ -void -tor_mutex_init(tor_mutex_t *mutex) -{ - int err; - if (PREDICT_UNLIKELY(!threads_initialized)) - tor_threads_init(); - err = pthread_mutex_init(&mutex->mutex, &attr_reentrant); - if (PREDICT_UNLIKELY(err)) { - log_err(LD_GENERAL, "Error %d creating a mutex.", err); - tor_fragile_assert(); - } -} -/** Wait until m is free, then acquire it. */ -void -tor_mutex_acquire(tor_mutex_t *m) -{ - int err; - tor_assert(m); - err = pthread_mutex_lock(&m->mutex); - if (PREDICT_UNLIKELY(err)) { - log_err(LD_GENERAL, "Error %d locking a mutex.", err); - tor_fragile_assert(); - } -} -/** Release the lock m so another thread can have it. */ -void -tor_mutex_release(tor_mutex_t *m) -{ - int err; - tor_assert(m); - err = pthread_mutex_unlock(&m->mutex); - if (PREDICT_UNLIKELY(err)) { - log_err(LD_GENERAL, "Error %d unlocking a mutex.", err); - tor_fragile_assert(); - } -} -/** Clean up the mutex m so that it no longer uses any system - * resources. Does not free m. This function must only be called on - * mutexes from tor_mutex_init(). */ -void -tor_mutex_uninit(tor_mutex_t *m) -{ - int err; - tor_assert(m); - err = pthread_mutex_destroy(&m->mutex); - if (PREDICT_UNLIKELY(err)) { - log_err(LD_GENERAL, "Error %d destroying a mutex.", err); - tor_fragile_assert(); - } -} -/** Return an integer representing this thread. */ -unsigned long -tor_get_thread_id(void) -{ - union { - pthread_t thr; - unsigned long id; - } r; - r.thr = pthread_self(); - return r.id; -} -#endif - -/** Return a newly allocated, ready-for-use mutex. */ -tor_mutex_t * -tor_mutex_new(void) -{ - tor_mutex_t *m = tor_malloc_zero(sizeof(tor_mutex_t)); - tor_mutex_init(m); - return m; -} -/** Release all storage and system resources held by m. */ -void -tor_mutex_free(tor_mutex_t *m) -{ - if (!m) - return; - tor_mutex_uninit(m); - tor_free(m); -} - -/* Conditions. */ -#ifdef USE_PTHREADS -#if 0 -/** Cross-platform condition implementation. */ -struct tor_cond_t { - pthread_cond_t cond; -}; -/** Return a newly allocated condition, with nobody waiting on it. */ -tor_cond_t * -tor_cond_new(void) -{ - tor_cond_t *cond = tor_malloc_zero(sizeof(tor_cond_t)); - if (pthread_cond_init(&cond->cond, NULL)) { - tor_free(cond); - return NULL; - } - return cond; -} -/** Release all resources held by cond. */ -void -tor_cond_free(tor_cond_t *cond) -{ - if (!cond) - return; - if (pthread_cond_destroy(&cond->cond)) { - log_warn(LD_GENERAL,"Error freeing condition: %s", strerror(errno)); - return; - } - tor_free(cond); -} -/** Wait until one of the tor_cond_signal functions is called on cond. - * All waiters on the condition must wait holding the same mutex. - * Returns 0 on success, negative on failure. */ -int -tor_cond_wait(tor_cond_t *cond, tor_mutex_t *mutex) -{ - return pthread_cond_wait(&cond->cond, &mutex->mutex) ? -1 : 0; -} -/** Wake up one of the waiters on cond. */ -void -tor_cond_signal_one(tor_cond_t *cond) -{ - pthread_cond_signal(&cond->cond); -} -/** Wake up all of the waiters on cond. */ -void -tor_cond_signal_all(tor_cond_t *cond) -{ - pthread_cond_broadcast(&cond->cond); -} -#endif -/** Set up common structures for use by threading. */ -void -tor_threads_init(void) -{ - if (!threads_initialized) { - pthread_mutexattr_init(&attr_reentrant); - pthread_mutexattr_settype(&attr_reentrant, PTHREAD_MUTEX_RECURSIVE); - tor_assert(0==pthread_attr_init(&attr_detached)); - tor_assert(0==pthread_attr_setdetachstate(&attr_detached, 1)); - threads_initialized = 1; - set_main_thread(); - } -} -#elif defined(USE_WIN32_THREADS) -#if 0 -static DWORD cond_event_tls_index; -struct tor_cond_t { - CRITICAL_SECTION mutex; - smartlist_t *events; -}; -tor_cond_t * -tor_cond_new(void) -{ - tor_cond_t *cond = tor_malloc_zero(sizeof(tor_cond_t)); - InitializeCriticalSection(&cond->mutex); - cond->events = smartlist_new(); - return cond; -} -void -tor_cond_free(tor_cond_t *cond) -{ - if (!cond) - return; - DeleteCriticalSection(&cond->mutex); - /* XXXX notify? */ - smartlist_free(cond->events); - tor_free(cond); -} -int -tor_cond_wait(tor_cond_t *cond, tor_mutex_t *mutex) -{ - HANDLE event; - int r; - tor_assert(cond); - tor_assert(mutex); - event = TlsGetValue(cond_event_tls_index); - if (!event) { - event = CreateEvent(0, FALSE, FALSE, NULL); - TlsSetValue(cond_event_tls_index, event); - } - EnterCriticalSection(&cond->mutex); - - tor_assert(WaitForSingleObject(event, 0) == WAIT_TIMEOUT); - tor_assert(!smartlist_contains(cond->events, event)); - smartlist_add(cond->events, event); - - LeaveCriticalSection(&cond->mutex); - - tor_mutex_release(mutex); - r = WaitForSingleObject(event, INFINITE); - tor_mutex_acquire(mutex); - - switch (r) { - case WAIT_OBJECT_0: /* we got the mutex normally. */ - break; - case WAIT_ABANDONED: /* holding thread exited. */ - case WAIT_TIMEOUT: /* Should never happen. */ - tor_assert(0); - break; - case WAIT_FAILED: - log_warn(LD_GENERAL, "Failed to acquire mutex: %d",(int) GetLastError()); - } - return 0; -} -void -tor_cond_signal_one(tor_cond_t *cond) -{ - HANDLE event; - tor_assert(cond); - - EnterCriticalSection(&cond->mutex); - - if ((event = smartlist_pop_last(cond->events))) - SetEvent(event); - - LeaveCriticalSection(&cond->mutex); -} -void -tor_cond_signal_all(tor_cond_t *cond) -{ - tor_assert(cond); - - EnterCriticalSection(&cond->mutex); - SMARTLIST_FOREACH(cond->events, HANDLE, event, SetEvent(event)); - smartlist_clear(cond->events); - LeaveCriticalSection(&cond->mutex); -} -#endif -void -tor_threads_init(void) -{ -#if 0 - cond_event_tls_index = TlsAlloc(); -#endif - set_main_thread(); -} -#endif - #if defined(HAVE_MLOCKALL) && HAVE_DECL_MLOCKALL && defined(RLIMIT_MEMLOCK) /** Attempt to raise the current and max rlimit to infinity for our process. * This only needs to be done once and can probably only be done when we have @@ -3292,23 +2915,6 @@ tor_mlockall(void) #endif } -/** Identity of the "main" thread */ -static unsigned long main_thread_id = -1; - -/** Start considering the current thread to be the 'main thread'. This has - * no effect on anything besides in_main_thread(). */ -void -set_main_thread(void) -{ - main_thread_id = tor_get_thread_id(); -} -/** Return true iff called from the main thread. */ -int -in_main_thread(void) -{ - return main_thread_id == tor_get_thread_id(); -} - /** * On Windows, WSAEWOULDBLOCK is not always correct: when you see it, * you need to ask the socket for its actual errno. Also, you need to diff --git a/src/common/compat.h b/src/common/compat.h index 04e8cb267c..23f8614196 100644 --- a/src/common/compat.h +++ b/src/common/compat.h @@ -36,9 +36,6 @@ #ifdef HAVE_STRING_H #include #endif -#if defined(HAVE_PTHREAD_H) && !defined(_WIN32) -#include -#endif #include #ifdef HAVE_SYS_RESOURCE_H #include @@ -642,61 +639,10 @@ char **get_environment(void); int get_total_system_memory(size_t *mem_out); -int spawn_func(void (*func)(void *), void *data); -void spawn_exit(void) ATTR_NORETURN; - -#if defined(_WIN32) -#define USE_WIN32_THREADS -#elif defined(HAVE_PTHREAD_H) && defined(HAVE_PTHREAD_CREATE) -#define USE_PTHREADS -#else -#error "No threading system was found" -#endif - int compute_num_cpus(void); -/* Because we use threads instead of processes on most platforms (Windows, - * Linux, etc), we need locking for them. On platforms with poor thread - * support or broken gethostbyname_r, these functions are no-ops. */ - -/** A generic lock structure for multithreaded builds. */ -typedef struct tor_mutex_t { -#if defined(USE_WIN32_THREADS) - /** Windows-only: on windows, we implement locks with CRITICAL_SECTIONS. */ - CRITICAL_SECTION mutex; -#elif defined(USE_PTHREADS) - /** Pthreads-only: with pthreads, we implement locks with - * pthread_mutex_t. */ - pthread_mutex_t mutex; -#else - /** No-threads only: Dummy variable so that tor_mutex_t takes up space. */ - int _unused; -#endif -} tor_mutex_t; - int tor_mlockall(void); -tor_mutex_t *tor_mutex_new(void); -void tor_mutex_init(tor_mutex_t *m); -void tor_mutex_acquire(tor_mutex_t *m); -void tor_mutex_release(tor_mutex_t *m); -void tor_mutex_free(tor_mutex_t *m); -void tor_mutex_uninit(tor_mutex_t *m); -unsigned long tor_get_thread_id(void); -void tor_threads_init(void); - -void set_main_thread(void); -int in_main_thread(void); - -#if 0 -typedef struct tor_cond_t tor_cond_t; -tor_cond_t *tor_cond_new(void); -void tor_cond_free(tor_cond_t *cond); -int tor_cond_wait(tor_cond_t *cond, tor_mutex_t *mutex); -void tor_cond_signal_one(tor_cond_t *cond); -void tor_cond_signal_all(tor_cond_t *cond); -#endif - /** Macros for MIN/MAX. Never use these when the arguments could have * side-effects. * {With GCC extensions we could probably define a safer MIN/MAX. But @@ -742,5 +688,8 @@ STATIC int tor_ersatz_socketpair(int family, int type, int protocol, #endif #endif +/* This needs some of the declarations above so we include it here. */ +#include "compat_threads.h" + #endif diff --git a/src/common/compat_pthreads.c b/src/common/compat_pthreads.c new file mode 100644 index 0000000000..276b2443c4 --- /dev/null +++ b/src/common/compat_pthreads.c @@ -0,0 +1,211 @@ +/* Copyright (c) 2003-2004, Roger Dingledine + * Copyright (c) 2004-2006, Roger Dingledine, Nick Mathewson. + * Copyright (c) 2007-2015, The Tor Project, Inc. */ +/* See LICENSE for licensing information */ + +#include "orconfig.h" +#include + +#include "compat.h" +#include "torlog.h" +#include "util.h" + +/** Wraps a void (*)(void*) function and its argument so we can + * invoke them in a way pthreads would expect. + */ +typedef struct tor_pthread_data_t { + void (*func)(void *); + void *data; +} tor_pthread_data_t; +/** Given a tor_pthread_data_t _data, call _data->func(d->data) + * and free _data. Used to make sure we can call functions the way pthread + * expects. */ +static void * +tor_pthread_helper_fn(void *_data) +{ + tor_pthread_data_t *data = _data; + void (*func)(void*); + void *arg; + /* mask signals to worker threads to avoid SIGPIPE, etc */ + sigset_t sigs; + /* We're in a subthread; don't handle any signals here. */ + sigfillset(&sigs); + pthread_sigmask(SIG_SETMASK, &sigs, NULL); + + func = data->func; + arg = data->data; + tor_free(_data); + func(arg); + return NULL; +} +/** + * A pthread attribute to make threads start detached. + */ +static pthread_attr_t attr_detached; +/** True iff we've called tor_threads_init() */ +static int threads_initialized = 0; + + +/** Minimalist interface to run a void function in the background. On + * Unix calls fork, on win32 calls beginthread. Returns -1 on failure. + * func should not return, but rather should call spawn_exit. + * + * NOTE: if data is used, it should not be allocated on the stack, + * since in a multithreaded environment, there is no way to be sure that + * the caller's stack will still be around when the called function is + * running. + */ +int +spawn_func(void (*func)(void *), void *data) +{ + pthread_t thread; + tor_pthread_data_t *d; + if (PREDICT_UNLIKELY(!threads_initialized)) + tor_threads_init(); + d = tor_malloc(sizeof(tor_pthread_data_t)); + d->data = data; + d->func = func; + if (pthread_create(&thread,&attr_detached,tor_pthread_helper_fn,d)) + return -1; + return 0; +} + +/** End the current thread/process. + */ +void +spawn_exit(void) +{ + pthread_exit(NULL); +} + +/** A mutex attribute that we're going to use to tell pthreads that we want + * "reentrant" mutexes (i.e., once we can re-lock if we're already holding + * them.) */ +static pthread_mutexattr_t attr_reentrant; +/** Initialize mutex so it can be locked. Every mutex must be set + * up with tor_mutex_init() or tor_mutex_new(); not both. */ +void +tor_mutex_init(tor_mutex_t *mutex) +{ + int err; + if (PREDICT_UNLIKELY(!threads_initialized)) + tor_threads_init(); + err = pthread_mutex_init(&mutex->mutex, &attr_reentrant); + if (PREDICT_UNLIKELY(err)) { + log_err(LD_GENERAL, "Error %d creating a mutex.", err); + tor_fragile_assert(); + } +} +/** Wait until m is free, then acquire it. */ +void +tor_mutex_acquire(tor_mutex_t *m) +{ + int err; + tor_assert(m); + err = pthread_mutex_lock(&m->mutex); + if (PREDICT_UNLIKELY(err)) { + log_err(LD_GENERAL, "Error %d locking a mutex.", err); + tor_fragile_assert(); + } +} +/** Release the lock m so another thread can have it. */ +void +tor_mutex_release(tor_mutex_t *m) +{ + int err; + tor_assert(m); + err = pthread_mutex_unlock(&m->mutex); + if (PREDICT_UNLIKELY(err)) { + log_err(LD_GENERAL, "Error %d unlocking a mutex.", err); + tor_fragile_assert(); + } +} +/** Clean up the mutex m so that it no longer uses any system + * resources. Does not free m. This function must only be called on + * mutexes from tor_mutex_init(). */ +void +tor_mutex_uninit(tor_mutex_t *m) +{ + int err; + tor_assert(m); + err = pthread_mutex_destroy(&m->mutex); + if (PREDICT_UNLIKELY(err)) { + log_err(LD_GENERAL, "Error %d destroying a mutex.", err); + tor_fragile_assert(); + } +} +/** Return an integer representing this thread. */ +unsigned long +tor_get_thread_id(void) +{ + union { + pthread_t thr; + unsigned long id; + } r; + r.thr = pthread_self(); + return r.id; +} + +/* Conditions. */ + +/** Cross-platform condition implementation. */ +struct tor_cond_t { + pthread_cond_t cond; +}; +/** Return a newly allocated condition, with nobody waiting on it. */ +tor_cond_t * +tor_cond_new(void) +{ + tor_cond_t *cond = tor_malloc_zero(sizeof(tor_cond_t)); + if (pthread_cond_init(&cond->cond, NULL)) { + tor_free(cond); + return NULL; + } + return cond; +} +/** Release all resources held by cond. */ +void +tor_cond_free(tor_cond_t *cond) +{ + if (!cond) + return; + if (pthread_cond_destroy(&cond->cond)) { + log_warn(LD_GENERAL,"Error freeing condition: %s", strerror(errno)); + return; + } + tor_free(cond); +} +/** Wait until one of the tor_cond_signal functions is called on cond. + * All waiters on the condition must wait holding the same mutex. + * Returns 0 on success, negative on failure. */ +int +tor_cond_wait(tor_cond_t *cond, tor_mutex_t *mutex) +{ + return pthread_cond_wait(&cond->cond, &mutex->mutex) ? -1 : 0; +} +/** Wake up one of the waiters on cond. */ +void +tor_cond_signal_one(tor_cond_t *cond) +{ + pthread_cond_signal(&cond->cond); +} +/** Wake up all of the waiters on cond. */ +void +tor_cond_signal_all(tor_cond_t *cond) +{ + pthread_cond_broadcast(&cond->cond); +} + +/** Set up common structures for use by threading. */ +void +tor_threads_init(void) +{ + if (!threads_initialized) { + pthread_mutexattr_init(&attr_reentrant); + pthread_mutexattr_settype(&attr_reentrant, PTHREAD_MUTEX_RECURSIVE); + tor_assert(0==pthread_attr_init(&attr_detached)); + tor_assert(0==pthread_attr_setdetachstate(&attr_detached, 1)); + threads_initialized = 1; + set_main_thread(); + } +} diff --git a/src/common/compat_threads.c b/src/common/compat_threads.c new file mode 100644 index 0000000000..84a8a21fe2 --- /dev/null +++ b/src/common/compat_threads.c @@ -0,0 +1,42 @@ +/* Copyright (c) 2003-2004, Roger Dingledine + * Copyright (c) 2004-2006, Roger Dingledine, Nick Mathewson. + * Copyright (c) 2007-2015, The Tor Project, Inc. */ +/* See LICENSE for licensing information */ + +#include "compat.h" +#include "util.h" + +/** Return a newly allocated, ready-for-use mutex. */ +tor_mutex_t * +tor_mutex_new(void) +{ + tor_mutex_t *m = tor_malloc_zero(sizeof(tor_mutex_t)); + tor_mutex_init(m); + return m; +} +/** Release all storage and system resources held by m. */ +void +tor_mutex_free(tor_mutex_t *m) +{ + if (!m) + return; + tor_mutex_uninit(m); + tor_free(m); +} + +/** Identity of the "main" thread */ +static unsigned long main_thread_id = -1; + +/** Start considering the current thread to be the 'main thread'. This has + * no effect on anything besides in_main_thread(). */ +void +set_main_thread(void) +{ + main_thread_id = tor_get_thread_id(); +} +/** Return true iff called from the main thread. */ +int +in_main_thread(void) +{ + return main_thread_id == tor_get_thread_id(); +} diff --git a/src/common/compat_threads.h b/src/common/compat_threads.h new file mode 100644 index 0000000000..f43e74ab8f --- /dev/null +++ b/src/common/compat_threads.h @@ -0,0 +1,67 @@ +/* Copyright (c) 2003-2004, Roger Dingledine + * Copyright (c) 2004-2006, Roger Dingledine, Nick Mathewson. + * Copyright (c) 2007-2015, The Tor Project, Inc. */ +/* See LICENSE for licensing information */ + +#ifndef TOR_COMPAT_THREADS_H +#define TOR_COMPAT_THREADS_H + +#include "orconfig.h" +#include "torint.h" +#include "testsupport.h" + +#if defined(HAVE_PTHREAD_H) && !defined(_WIN32) +#include +#endif + +#if defined(_WIN32) +#define USE_WIN32_THREADS +#elif defined(HAVE_PTHREAD_H) && defined(HAVE_PTHREAD_CREATE) +#define USE_PTHREADS +#else +#error "No threading system was found" +#endif + +int spawn_func(void (*func)(void *), void *data); +void spawn_exit(void) ATTR_NORETURN; + +/* Because we use threads instead of processes on most platforms (Windows, + * Linux, etc), we need locking for them. On platforms with poor thread + * support or broken gethostbyname_r, these functions are no-ops. */ + +/** A generic lock structure for multithreaded builds. */ +typedef struct tor_mutex_t { +#if defined(USE_WIN32_THREADS) + /** Windows-only: on windows, we implement locks with CRITICAL_SECTIONS. */ + CRITICAL_SECTION mutex; +#elif defined(USE_PTHREADS) + /** Pthreads-only: with pthreads, we implement locks with + * pthread_mutex_t. */ + pthread_mutex_t mutex; +#else + /** No-threads only: Dummy variable so that tor_mutex_t takes up space. */ + int _unused; +#endif +} tor_mutex_t; + + +tor_mutex_t *tor_mutex_new(void); +void tor_mutex_init(tor_mutex_t *m); +void tor_mutex_acquire(tor_mutex_t *m); +void tor_mutex_release(tor_mutex_t *m); +void tor_mutex_free(tor_mutex_t *m); +void tor_mutex_uninit(tor_mutex_t *m); +unsigned long tor_get_thread_id(void); +void tor_threads_init(void); + +void set_main_thread(void); +int in_main_thread(void); + +typedef struct tor_cond_t tor_cond_t; +tor_cond_t *tor_cond_new(void); +void tor_cond_free(tor_cond_t *cond); +int tor_cond_wait(tor_cond_t *cond, tor_mutex_t *mutex); +void tor_cond_signal_one(tor_cond_t *cond); +void tor_cond_signal_all(tor_cond_t *cond); + +#endif diff --git a/src/common/compat_winthreads.c b/src/common/compat_winthreads.c new file mode 100644 index 0000000000..01332fd944 --- /dev/null +++ b/src/common/compat_winthreads.c @@ -0,0 +1,161 @@ +/* Copyright (c) 2003-2004, Roger Dingledine + * Copyright (c) 2004-2006, Roger Dingledine, Nick Mathewson. + * Copyright (c) 2007-2015, The Tor Project, Inc. */ +/* See LICENSE for licensing information */ + +#include "compat.h" +#include +#include +#include "util.h" +#include "container.h" +#include "torlog.h" + +/** Minimalist interface to run a void function in the background. On + * Unix calls fork, on win32 calls beginthread. Returns -1 on failure. + * func should not return, but rather should call spawn_exit. + * + * NOTE: if data is used, it should not be allocated on the stack, + * since in a multithreaded environment, there is no way to be sure that + * the caller's stack will still be around when the called function is + * running. + */ +int +spawn_func(void (*func)(void *), void *data) +{ + int rv; + rv = (int)_beginthread(func, 0, data); + if (rv == (int)-1) + return -1; + return 0; +} + + +/** End the current thread/process. + */ +void +spawn_exit(void) +{ + _endthread(); + //we should never get here. my compiler thinks that _endthread returns, this + //is an attempt to fool it. + tor_assert(0); + _exit(0); +} + + +void +tor_mutex_init(tor_mutex_t *m) +{ + InitializeCriticalSection(&m->mutex); +} +void +tor_mutex_uninit(tor_mutex_t *m) +{ + DeleteCriticalSection(&m->mutex); +} +void +tor_mutex_acquire(tor_mutex_t *m) +{ + tor_assert(m); + EnterCriticalSection(&m->mutex); +} +void +tor_mutex_release(tor_mutex_t *m) +{ + LeaveCriticalSection(&m->mutex); +} +unsigned long +tor_get_thread_id(void) +{ + return (unsigned long)GetCurrentThreadId(); +} + +static DWORD cond_event_tls_index; +struct tor_cond_t { + CRITICAL_SECTION mutex; + smartlist_t *events; +}; +tor_cond_t * +tor_cond_new(void) +{ + tor_cond_t *cond = tor_malloc_zero(sizeof(tor_cond_t)); + InitializeCriticalSection(&cond->mutex); + cond->events = smartlist_new(); + return cond; +} +void +tor_cond_free(tor_cond_t *cond) +{ + if (!cond) + return; + DeleteCriticalSection(&cond->mutex); + /* XXXX notify? */ + smartlist_free(cond->events); + tor_free(cond); +} +int +tor_cond_wait(tor_cond_t *cond, tor_mutex_t *mutex) +{ + HANDLE event; + int r; + tor_assert(cond); + tor_assert(mutex); + event = TlsGetValue(cond_event_tls_index); + if (!event) { + event = CreateEvent(0, FALSE, FALSE, NULL); + TlsSetValue(cond_event_tls_index, event); + } + EnterCriticalSection(&cond->mutex); + + tor_assert(WaitForSingleObject(event, 0) == WAIT_TIMEOUT); + tor_assert(!smartlist_contains(cond->events, event)); + smartlist_add(cond->events, event); + + LeaveCriticalSection(&cond->mutex); + + tor_mutex_release(mutex); + r = WaitForSingleObject(event, INFINITE); + tor_mutex_acquire(mutex); + + switch (r) { + case WAIT_OBJECT_0: /* we got the mutex normally. */ + break; + case WAIT_ABANDONED: /* holding thread exited. */ + case WAIT_TIMEOUT: /* Should never happen. */ + tor_assert(0); + break; + case WAIT_FAILED: + log_warn(LD_GENERAL, "Failed to acquire mutex: %d",(int) GetLastError()); + } + return 0; +} +void +tor_cond_signal_one(tor_cond_t *cond) +{ + HANDLE event; + tor_assert(cond); + + EnterCriticalSection(&cond->mutex); + + if ((event = smartlist_pop_last(cond->events))) + SetEvent(event); + + LeaveCriticalSection(&cond->mutex); +} +void +tor_cond_signal_all(tor_cond_t *cond) +{ + tor_assert(cond); + + EnterCriticalSection(&cond->mutex); + SMARTLIST_FOREACH(cond->events, HANDLE, event, SetEvent(event)); + smartlist_clear(cond->events); + LeaveCriticalSection(&cond->mutex); +} + +void +tor_threads_init(void) +{ + cond_event_tls_index = TlsAlloc(); + set_main_thread(); +} diff --git a/src/common/include.am b/src/common/include.am index 6441596199..e4eeba6bbf 100644 --- a/src/common/include.am +++ b/src/common/include.am @@ -54,10 +54,18 @@ endif LIBDONNA += $(LIBED25519_REF10) +if THREADS_PTHREADS +threads_impl_source=src/common/compat_pthreads.c +endif +if THREADS_WIN32 +threads_impl_source=src/common/compat_winthreads.c +endif + LIBOR_A_SOURCES = \ src/common/address.c \ src/common/backtrace.c \ src/common/compat.c \ + src/common/compat_threads.c \ src/common/container.c \ src/common/di_ops.c \ src/common/log.c \ @@ -69,7 +77,8 @@ LIBOR_A_SOURCES = \ src/ext/csiphash.c \ src/ext/trunnel/trunnel.c \ $(libor_extra_source) \ - $(libor_mempool_source) + $(libor_mempool_source) \ + $(threads_impl_source) LIBOR_CRYPTO_A_SOURCES = \ src/common/aes.c \ @@ -102,7 +111,6 @@ src_common_libor_testing_a_CFLAGS = $(AM_CFLAGS) $(TEST_CFLAGS) src_common_libor_crypto_testing_a_CFLAGS = $(AM_CFLAGS) $(TEST_CFLAGS) src_common_libor_event_testing_a_CFLAGS = $(AM_CFLAGS) $(TEST_CFLAGS) - COMMONHEADERS = \ src/common/address.h \ src/common/backtrace.h \ @@ -110,6 +118,7 @@ COMMONHEADERS = \ src/common/ciphers.inc \ src/common/compat.h \ src/common/compat_libevent.h \ + src/common/compat_threads.h \ src/common/container.h \ src/common/crypto.h \ src/common/crypto_curve25519.h \ From e865248156a8512d756be003118de446d29611d1 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Sun, 22 Sep 2013 22:08:41 -0400 Subject: [PATCH 02/32] Add a timeout to tor_cond_wait; add tor_cond impl from libevent The windows code may need some tweaks for it to compile; I've not tested it yet. --- src/common/compat_pthreads.c | 24 ++++-- src/common/compat_threads.h | 19 ++++- src/common/compat_winthreads.c | 152 ++++++++++++++++++++------------- 3 files changed, 126 insertions(+), 69 deletions(-) diff --git a/src/common/compat_pthreads.c b/src/common/compat_pthreads.c index 276b2443c4..0e5d33a659 100644 --- a/src/common/compat_pthreads.c +++ b/src/common/compat_pthreads.c @@ -148,10 +148,6 @@ tor_get_thread_id(void) /* Conditions. */ -/** Cross-platform condition implementation. */ -struct tor_cond_t { - pthread_cond_t cond; -}; /** Return a newly allocated condition, with nobody waiting on it. */ tor_cond_t * tor_cond_new(void) @@ -177,11 +173,25 @@ tor_cond_free(tor_cond_t *cond) } /** Wait until one of the tor_cond_signal functions is called on cond. * All waiters on the condition must wait holding the same mutex. - * Returns 0 on success, negative on failure. */ + * Returns 0 on success, -1 on failure, 1 on timeout. */ int -tor_cond_wait(tor_cond_t *cond, tor_mutex_t *mutex) +tor_cond_wait(tor_cond_t *cond, tor_mutex_t *mutex, const struct timeval *tv) { - return pthread_cond_wait(&cond->cond, &mutex->mutex) ? -1 : 0; + if (tv == NULL) { + return pthread_cond_wait(&cond->cond, &mutex->mutex) ? -1 : 0; + } else { + struct timespec ts; + int r; + ts.tv_sec = tv->tv_sec; + ts.tv_nsec = tv->tv_usec * 1000; + r = pthread_cond_timedwait(&cond->cond, &mutex->mutex, &ts); + if (r == 0) + return 0; + else if (r == ETIMEDOUT) + return 1; + else + return -1; + } } /** Wake up one of the waiters on cond. */ void diff --git a/src/common/compat_threads.h b/src/common/compat_threads.h index f43e74ab8f..bbd782fd45 100644 --- a/src/common/compat_threads.h +++ b/src/common/compat_threads.h @@ -57,10 +57,25 @@ void tor_threads_init(void); void set_main_thread(void); int in_main_thread(void); -typedef struct tor_cond_t tor_cond_t; +typedef struct tor_cond_t { +#ifdef USE_PTHREADS + pthread_cond_t cond; +#elif defined(USE_WIN32_THREADS) + HANDLE event; + + CRITICAL_SECTION lock; + int n_waiting; + int n_to_wake; + int generation; +#else +#error no known condition implementation. +#endif +} tor_cond_t; + tor_cond_t *tor_cond_new(void); void tor_cond_free(tor_cond_t *cond); -int tor_cond_wait(tor_cond_t *cond, tor_mutex_t *mutex); +int tor_cond_wait(tor_cond_t *cond, tor_mutex_t *mutex, + const struct timeval *tv); void tor_cond_signal_one(tor_cond_t *cond); void tor_cond_signal_all(tor_cond_t *cond); diff --git a/src/common/compat_winthreads.c b/src/common/compat_winthreads.c index 01332fd944..634dfbed30 100644 --- a/src/common/compat_winthreads.c +++ b/src/common/compat_winthreads.c @@ -70,17 +70,20 @@ tor_get_thread_id(void) return (unsigned long)GetCurrentThreadId(); } -static DWORD cond_event_tls_index; -struct tor_cond_t { - CRITICAL_SECTION mutex; - smartlist_t *events; -}; tor_cond_t * tor_cond_new(void) { - tor_cond_t *cond = tor_malloc_zero(sizeof(tor_cond_t)); - InitializeCriticalSection(&cond->mutex); - cond->events = smartlist_new(); + tor_cond_t *cond = tor_malloc(sizeof(tor_cond_t)); + if (InitializeCriticalSectionAndSpinCount(&cond->lock, SPIN_COUNT)==0) { + tor_free(cond); + return NULL; + } + if ((cond->event = CreateEvent(NULL,TRUE,FALSE,NULL)) == NULL) { + DeleteCriticalSection(&cond->lock); + tor_free(cond); + return NULL; + } + cond->n_waiting = cond->n_to_wake = cond->generation = 0; return cond; } void @@ -88,74 +91,103 @@ tor_cond_free(tor_cond_t *cond) { if (!cond) return; - DeleteCriticalSection(&cond->mutex); - /* XXXX notify? */ - smartlist_free(cond->events); - tor_free(cond); + DeleteCriticalSection(&cond->lock); + CloseHandle(cond->event); + mm_free(cond); } -int -tor_cond_wait(tor_cond_t *cond, tor_mutex_t *mutex) + +static void +tor_cond_signal_impl(tor_cond_t *cond, int broadcast) { - HANDLE event; - int r; - tor_assert(cond); - tor_assert(mutex); - event = TlsGetValue(cond_event_tls_index); - if (!event) { - event = CreateEvent(0, FALSE, FALSE, NULL); - TlsSetValue(cond_event_tls_index, event); - } - EnterCriticalSection(&cond->mutex); - - tor_assert(WaitForSingleObject(event, 0) == WAIT_TIMEOUT); - tor_assert(!smartlist_contains(cond->events, event)); - smartlist_add(cond->events, event); - - LeaveCriticalSection(&cond->mutex); - - tor_mutex_release(mutex); - r = WaitForSingleObject(event, INFINITE); - tor_mutex_acquire(mutex); - - switch (r) { - case WAIT_OBJECT_0: /* we got the mutex normally. */ - break; - case WAIT_ABANDONED: /* holding thread exited. */ - case WAIT_TIMEOUT: /* Should never happen. */ - tor_assert(0); - break; - case WAIT_FAILED: - log_warn(LD_GENERAL, "Failed to acquire mutex: %d",(int) GetLastError()); - } + EnterCriticalSection(&cond->lock); + if (broadcast) + cond->n_to_wake = cond->n_waiting; + else + ++cond->n_to_wake; + cond->generation++; + SetEvent(cond->event); + LeaveCriticalSection(&cond->lock); return 0; } void tor_cond_signal_one(tor_cond_t *cond) { - HANDLE event; - tor_assert(cond); - - EnterCriticalSection(&cond->mutex); - - if ((event = smartlist_pop_last(cond->events))) - SetEvent(event); - - LeaveCriticalSection(&cond->mutex); + tor_cond_signal_impl(cond, 0); } void tor_cond_signal_all(tor_cond_t *cond) { - tor_assert(cond); + tor_cond_signal_impl(cond, 1); +} - EnterCriticalSection(&cond->mutex); - SMARTLIST_FOREACH(cond->events, HANDLE, event, SetEvent(event)); - smartlist_clear(cond->events); - LeaveCriticalSection(&cond->mutex); +int +tor_cond_wait(tor_cond_t *cond, tor_mutex_t *lock, const struct timeval *tv) +{ + CRITICAL_SECTION *lock = &lock->mutex; + int generation_at_start; + int waiting = 1; + int result = -1; + DWORD ms = INFINITE, ms_orig = INFINITE, startTime, endTime; + if (tv) + ms_orig = ms = evutil_tv_to_msec_(tv); + + EnterCriticalSection(&cond->lock); + ++cond->n_waiting; + generation_at_start = cond->generation; + LeaveCriticalSection(&cond->lock); + + LeaveCriticalSection(lock); + + startTime = GetTickCount(); + do { + DWORD res; + res = WaitForSingleObject(cond->event, ms); + EnterCriticalSection(&cond->lock); + if (cond->n_to_wake && + cond->generation != generation_at_start) { + --cond->n_to_wake; + --cond->n_waiting; + result = 0; + waiting = 0; + goto out; + } else if (res != WAIT_OBJECT_0) { + result = (res==WAIT_TIMEOUT) ? 1 : -1; + --cond->n_waiting; + waiting = 0; + goto out; + } else if (ms != INFINITE) { + endTime = GetTickCount(); + if (startTime + ms_orig <= endTime) { + result = 1; /* Timeout */ + --cond->n_waiting; + waiting = 0; + goto out; + } else { + ms = startTime + ms_orig - endTime; + } + } + /* If we make it here, we are still waiting. */ + if (cond->n_to_wake == 0) { + /* There is nobody else who should wake up; reset + * the event. */ + ResetEvent(cond->event); + } + out: + LeaveCriticalSection(&cond->lock); + } while (waiting); + + EnterCriticalSection(lock); + + EnterCriticalSection(&cond->lock); + if (!cond->n_waiting) + ResetEvent(cond->event); + LeaveCriticalSection(&cond->lock); + + return result; } void tor_threads_init(void) { - cond_event_tls_index = TlsAlloc(); set_main_thread(); } From 65016304d23503e230e8b097b5cdc1e4897b9b57 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Mon, 23 Sep 2013 01:15:30 -0400 Subject: [PATCH 03/32] Add tor_cond_init/uninit --- src/common/compat_pthreads.c | 17 ++++++----------- src/common/compat_threads.c | 17 +++++++++++++++++ src/common/compat_threads.h | 2 ++ src/common/compat_winthreads.c | 19 +++++++------------ 4 files changed, 32 insertions(+), 23 deletions(-) diff --git a/src/common/compat_pthreads.c b/src/common/compat_pthreads.c index 0e5d33a659..e58b3f7b57 100644 --- a/src/common/compat_pthreads.c +++ b/src/common/compat_pthreads.c @@ -148,28 +148,23 @@ tor_get_thread_id(void) /* Conditions. */ -/** Return a newly allocated condition, with nobody waiting on it. */ -tor_cond_t * -tor_cond_new(void) +int +tor_cond_init(tor_cond_t *cond) { - tor_cond_t *cond = tor_malloc_zero(sizeof(tor_cond_t)); + memset(cond, 0, sizeof(tor_cond_t)); if (pthread_cond_init(&cond->cond, NULL)) { - tor_free(cond); - return NULL; + return -1; } - return cond; + return 0; } /** Release all resources held by cond. */ void -tor_cond_free(tor_cond_t *cond) +tor_cond_uninit(tor_cond_t *cond) { - if (!cond) - return; if (pthread_cond_destroy(&cond->cond)) { log_warn(LD_GENERAL,"Error freeing condition: %s", strerror(errno)); return; } - tor_free(cond); } /** Wait until one of the tor_cond_signal functions is called on cond. * All waiters on the condition must wait holding the same mutex. diff --git a/src/common/compat_threads.c b/src/common/compat_threads.c index 84a8a21fe2..e0cbf5c1d8 100644 --- a/src/common/compat_threads.c +++ b/src/common/compat_threads.c @@ -24,6 +24,23 @@ tor_mutex_free(tor_mutex_t *m) tor_free(m); } +tor_cond_t * +tor_cond_new(void) +{ + tor_cond_t *cond = tor_malloc(sizeof(tor_cond_t)); + if (tor_cond_init(cond)<0) + tor_free(cond); + return cond; +} +void +tor_cond_free(tor_cond_t *c) +{ + if (!c) + return; + tor_cond_uninit(c); + tor_free(c); +} + /** Identity of the "main" thread */ static unsigned long main_thread_id = -1; diff --git a/src/common/compat_threads.h b/src/common/compat_threads.h index bbd782fd45..6d3ba3ae21 100644 --- a/src/common/compat_threads.h +++ b/src/common/compat_threads.h @@ -74,6 +74,8 @@ typedef struct tor_cond_t { tor_cond_t *tor_cond_new(void); void tor_cond_free(tor_cond_t *cond); +int tor_cond_init(tor_cond_t *cond); +void tor_cond_uninit(tor_cond_t *cond); int tor_cond_wait(tor_cond_t *cond, tor_mutex_t *mutex, const struct timeval *tv); void tor_cond_signal_one(tor_cond_t *cond); diff --git a/src/common/compat_winthreads.c b/src/common/compat_winthreads.c index 634dfbed30..11f91c63df 100644 --- a/src/common/compat_winthreads.c +++ b/src/common/compat_winthreads.c @@ -70,30 +70,25 @@ tor_get_thread_id(void) return (unsigned long)GetCurrentThreadId(); } -tor_cond_t * -tor_cond_new(void) +int +tor_cond_init(tor_cond_t *cond) { - tor_cond_t *cond = tor_malloc(sizeof(tor_cond_t)); + memset(cond, 0, sizeof(tor_cond_t)); if (InitializeCriticalSectionAndSpinCount(&cond->lock, SPIN_COUNT)==0) { - tor_free(cond); - return NULL; + return -1; } if ((cond->event = CreateEvent(NULL,TRUE,FALSE,NULL)) == NULL) { DeleteCriticalSection(&cond->lock); - tor_free(cond); - return NULL; + return -1; } cond->n_waiting = cond->n_to_wake = cond->generation = 0; - return cond; + return 0; } void -tor_cond_free(tor_cond_t *cond) +tor_cond_uninit(tor_cond_t *cond) { - if (!cond) - return; DeleteCriticalSection(&cond->lock); CloseHandle(cond->event); - mm_free(cond); } static void From 6c9363310aaea9d39fae4d9dd50e78d42c3598b3 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 24 Sep 2013 15:03:51 -0400 Subject: [PATCH 04/32] Specialize handling for mutexes allocated for condition variables (These must not be reentrant mutexes with pthreads.) --- src/common/compat_pthreads.c | 16 ++++++++++++++++ src/common/compat_threads.h | 1 + src/common/compat_winthreads.c | 6 ++++++ 3 files changed, 23 insertions(+) diff --git a/src/common/compat_pthreads.c b/src/common/compat_pthreads.c index e58b3f7b57..59b54a600a 100644 --- a/src/common/compat_pthreads.c +++ b/src/common/compat_pthreads.c @@ -96,6 +96,22 @@ tor_mutex_init(tor_mutex_t *mutex) tor_fragile_assert(); } } + +/** As tor_mutex_init, but initialize a mutex suitable for use with a + * condition variable. */ +void +tor_mutex_init_for_cond(tor_mutex_t *mutex) +{ + int err; + if (PREDICT_UNLIKELY(!threads_initialized)) + tor_threads_init(); + err = pthread_mutex_init(&mutex->mutex, NULL); + if (PREDICT_UNLIKELY(err)) { + log_err(LD_GENERAL, "Error %d creating a mutex.", err); + tor_fragile_assert(); + } +} + /** Wait until m is free, then acquire it. */ void tor_mutex_acquire(tor_mutex_t *m) diff --git a/src/common/compat_threads.h b/src/common/compat_threads.h index 6d3ba3ae21..581d8dd7b9 100644 --- a/src/common/compat_threads.h +++ b/src/common/compat_threads.h @@ -47,6 +47,7 @@ typedef struct tor_mutex_t { tor_mutex_t *tor_mutex_new(void); void tor_mutex_init(tor_mutex_t *m); +void tor_mutex_init_for_cond(tor_mutex_t *m); void tor_mutex_acquire(tor_mutex_t *m); void tor_mutex_release(tor_mutex_t *m); void tor_mutex_free(tor_mutex_t *m); diff --git a/src/common/compat_winthreads.c b/src/common/compat_winthreads.c index 11f91c63df..2b1527ad34 100644 --- a/src/common/compat_winthreads.c +++ b/src/common/compat_winthreads.c @@ -48,6 +48,12 @@ tor_mutex_init(tor_mutex_t *m) { InitializeCriticalSection(&m->mutex); } +void +tor_mutex_init_for_cond(tor_mutex_t *m) +{ + InitializeCriticalSection(&m->mutex); +} + void tor_mutex_uninit(tor_mutex_t *m) { From a82604b526a2a258e057d6d515ac17429eb6fb67 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Mon, 23 Sep 2013 01:19:16 -0400 Subject: [PATCH 05/32] Initial workqueue implemention, with a simple test. It seems to be working, but more tuning is needed. --- src/common/include.am | 2 + src/common/workqueue.c | 347 +++++++++++++++++++++++++++++++++++++ src/common/workqueue.h | 37 ++++ src/test/bench_workqueue.c | 298 +++++++++++++++++++++++++++++++ src/test/include.am | 13 +- 5 files changed, 696 insertions(+), 1 deletion(-) create mode 100644 src/common/workqueue.c create mode 100644 src/common/workqueue.h create mode 100644 src/test/bench_workqueue.c diff --git a/src/common/include.am b/src/common/include.am index e4eeba6bbf..14838ab555 100644 --- a/src/common/include.am +++ b/src/common/include.am @@ -74,6 +74,7 @@ LIBOR_A_SOURCES = \ src/common/util_codedigest.c \ src/common/util_process.c \ src/common/sandbox.c \ + src/common/workqueue.c \ src/ext/csiphash.c \ src/ext/trunnel/trunnel.c \ $(libor_extra_source) \ @@ -137,6 +138,7 @@ COMMONHEADERS = \ src/common/tortls.h \ src/common/util.h \ src/common/util_process.h \ + src/common/workqueue.h \ $(libor_mempool_header) noinst_HEADERS+= $(COMMONHEADERS) diff --git a/src/common/workqueue.c b/src/common/workqueue.c new file mode 100644 index 0000000000..ea8dcb0f9b --- /dev/null +++ b/src/common/workqueue.c @@ -0,0 +1,347 @@ +/* Copyright (c) 2013, The Tor Project, Inc. */ +/* See LICENSE for licensing information */ + +#include "orconfig.h" +#include "compat.h" +#include "compat_threads.h" +#include "util.h" +#include "workqueue.h" +#include "tor_queue.h" +#include "torlog.h" + +#ifdef HAVE_UNISTD_H +// XXXX move wherever we move the write/send stuff +#include +#endif + +/* + design: + + each thread has its own queue, try to keep at least elements min..max cycles + worth of work on each queue. + +keep array of threads; round-robin between them. + + When out of work, work-steal. + + alert threads with condition variables. + + alert main thread with fd, since it's libevent. + + + */ + +typedef struct workqueue_entry_s { + TOR_SIMPLEQ_ENTRY(workqueue_entry_s) next_work; + int (*fn)(int status, void *state, void *arg); + void (*reply_fn)(void *arg); + void *arg; +} workqueue_entry_t; + +struct replyqueue_s { + tor_mutex_t lock; + TOR_SIMPLEQ_HEAD(, workqueue_entry_s) answers; + + void (*alert_fn)(struct replyqueue_s *); // lock not held on this, next 2. + tor_socket_t write_sock; + tor_socket_t read_sock; +}; + +typedef struct workerthread_s { + tor_mutex_t lock; + tor_cond_t condition; + TOR_SIMPLEQ_HEAD(, workqueue_entry_s) work; + unsigned is_running; + unsigned is_shut_down; + unsigned waiting; + void *state; + replyqueue_t *reply_queue; +} workerthread_t; + +struct threadpool_s { + workerthread_t **threads; + int next_for_work; + + tor_mutex_t lock; + int n_threads; + + replyqueue_t *reply_queue; + + void *(*new_thread_state_fn)(void*); + void (*free_thread_state_fn)(void*); + void *new_thread_state_arg; + +}; + +static void queue_reply(replyqueue_t *queue, workqueue_entry_t *work); + +static workqueue_entry_t * +workqueue_entry_new(int (*fn)(int, void*, void*), + void (*reply_fn)(void*), + void *arg) +{ + workqueue_entry_t *ent = tor_malloc_zero(sizeof(workqueue_entry_t)); + ent->fn = fn; + ent->reply_fn = reply_fn; + ent->arg = arg; + return ent; +} + +static void +workqueue_entry_free(workqueue_entry_t *ent) +{ + if (!ent) + return; + tor_free(ent); +} + +static void +worker_thread_main(void *thread_) +{ + workerthread_t *thread = thread_; + workqueue_entry_t *work; + int result; + + tor_mutex_acquire(&thread->lock); + + thread->is_running = 1; + while (1) { + /* lock held. */ + while (!TOR_SIMPLEQ_EMPTY(&thread->work)) { + /* lock held. */ + + work = TOR_SIMPLEQ_FIRST(&thread->work); + TOR_SIMPLEQ_REMOVE_HEAD(&thread->work, next_work); + tor_mutex_release(&thread->lock); + + result = work->fn(WQ_CMD_RUN, thread->state, work->arg); + + if (result == WQ_RPL_QUEUE) { + queue_reply(thread->reply_queue, work); + } else { + workqueue_entry_free(work); + } + + tor_mutex_acquire(&thread->lock); + if (result >= WQ_RPL_ERROR) { + thread->is_running = 0; + thread->is_shut_down = 1; + tor_mutex_release(&thread->lock); + return; + } + } + /* Lock held; no work in this thread's queue. */ + + /* TODO: Try work-stealing. */ + + /* TODO: support an idle-function */ + + thread->waiting = 1; + if (tor_cond_wait(&thread->condition, &thread->lock, NULL) < 0) + /* ERR */ + thread->waiting = 0; + } +} + +static void +queue_reply(replyqueue_t *queue, workqueue_entry_t *work) +{ + int was_empty; + tor_mutex_acquire(&queue->lock); + was_empty = TOR_SIMPLEQ_EMPTY(&queue->answers); + TOR_SIMPLEQ_INSERT_TAIL(&queue->answers, work, next_work); + tor_mutex_release(&queue->lock); + + if (was_empty) { + queue->alert_fn(queue); + } +} + + +static void +alert_by_fd(replyqueue_t *queue) +{ + /* XXX extract this into new function */ +#ifndef _WIN32 + (void) send(queue->write_sock, "x", 1, 0); +#else + (void) write(queue->write_sock, "x", 1); +#endif +} + +static workerthread_t * +workerthread_new(void *state, replyqueue_t *replyqueue) +{ + workerthread_t *thr = tor_malloc_zero(sizeof(workerthread_t)); + tor_mutex_init_for_cond(&thr->lock); + tor_cond_init(&thr->condition); + TOR_SIMPLEQ_INIT(&thr->work); + thr->state = state; + thr->reply_queue = replyqueue; + + if (spawn_func(worker_thread_main, thr) < 0) { + log_err(LD_GENERAL, "Can't launch worker thread."); + return NULL; + } + + return thr; +} + +void * +threadpool_queue_work(threadpool_t *pool, + int (*fn)(int, void *, void *), + void (*reply_fn)(void *), + void *arg) +{ + workqueue_entry_t *ent; + workerthread_t *worker; + + tor_mutex_acquire(&pool->lock); + worker = pool->threads[pool->next_for_work++]; + if (!worker) { + tor_mutex_release(&pool->lock); + return NULL; + } + if (pool->next_for_work >= pool->n_threads) + pool->next_for_work = 0; + tor_mutex_release(&pool->lock); + + + ent = workqueue_entry_new(fn, reply_fn, arg); + + tor_mutex_acquire(&worker->lock); + TOR_SIMPLEQ_INSERT_TAIL(&worker->work, ent, next_work); + + if (worker->waiting) /* XXXX inside or outside of lock?? */ + tor_cond_signal_one(&worker->condition); + + tor_mutex_release(&worker->lock); + + return ent; +} + +int +threadpool_start_threads(threadpool_t *pool, int n) +{ + tor_mutex_acquire(&pool->lock); + + if (pool->n_threads < n) + pool->threads = tor_realloc(pool->threads, sizeof(workerthread_t*)*n); + + while (pool->n_threads < n) { + void *state = pool->new_thread_state_fn(pool->new_thread_state_arg); + workerthread_t *thr = workerthread_new(state, pool->reply_queue); + + if (!thr) { + tor_mutex_release(&pool->lock); + return -1; + } + pool->threads[pool->n_threads++] = thr; + } + tor_mutex_release(&pool->lock); + + return 0; +} + +threadpool_t * +threadpool_new(int n_threads, + replyqueue_t *replyqueue, + void *(*new_thread_state_fn)(void*), + void (*free_thread_state_fn)(void*), + void *arg) +{ + threadpool_t *pool; + pool = tor_malloc_zero(sizeof(threadpool_t)); + tor_mutex_init(&pool->lock); + pool->new_thread_state_fn = new_thread_state_fn; + pool->new_thread_state_arg = arg; + pool->free_thread_state_fn = free_thread_state_fn; + pool->reply_queue = replyqueue; + + if (threadpool_start_threads(pool, n_threads) < 0) { + tor_mutex_uninit(&pool->lock); + tor_free(pool); + return NULL; + } + + return pool; +} + +replyqueue_t * +threadpool_get_replyqueue(threadpool_t *tp) +{ + return tp->reply_queue; +} + +replyqueue_t * +replyqueue_new(void) +{ + tor_socket_t pair[2]; + replyqueue_t *rq; + int r; + + /* XXX extract this into new function */ +#ifdef _WIN32 + r = tor_socketpair(AF_UNIX, SOCK_STREAM, 0, pair); +#else + r = pipe(pair); +#endif + if (r < 0) + return NULL; + + set_socket_nonblocking(pair[0]); /* the read-size should be nonblocking. */ +#if defined(FD_CLOEXEC) + fcntl(pair[0], F_SETFD, FD_CLOEXEC); + fcntl(pair[1], F_SETFD, FD_CLOEXEC); +#endif + + rq = tor_malloc_zero(sizeof(replyqueue_t)); + + tor_mutex_init(&rq->lock); + TOR_SIMPLEQ_INIT(&rq->answers); + + rq->read_sock = pair[0]; + rq->write_sock = pair[1]; + rq->alert_fn = alert_by_fd; + + return rq; +} + +tor_socket_t +replyqueue_get_socket(replyqueue_t *rq) +{ + return rq->read_sock; +} + +void +replyqueue_process(replyqueue_t *queue) +{ + ssize_t r; + + /* XXX extract this into new function */ + do { + char buf[64]; +#ifdef _WIN32 + r = recv(queue->read_sock, buf, sizeof(buf), 0); +#else + r = read(queue->read_sock, buf, sizeof(buf)); +#endif + } while (r > 0); + + /* XXXX freak out on r == 0, or r == "error, not retryable". */ + + tor_mutex_acquire(&queue->lock); + while (!TOR_SIMPLEQ_EMPTY(&queue->answers)) { + /* lock held. */ + workqueue_entry_t *work = TOR_SIMPLEQ_FIRST(&queue->answers); + TOR_SIMPLEQ_REMOVE_HEAD(&queue->answers, next_work); + tor_mutex_release(&queue->lock); + + work->reply_fn(work->arg); + workqueue_entry_free(work); + + tor_mutex_acquire(&queue->lock); + } + + tor_mutex_release(&queue->lock); +} diff --git a/src/common/workqueue.h b/src/common/workqueue.h new file mode 100644 index 0000000000..e502734b84 --- /dev/null +++ b/src/common/workqueue.h @@ -0,0 +1,37 @@ +/* Copyright (c) 2013, The Tor Project, Inc. */ +/* See LICENSE for licensing information */ + +#ifndef TOR_WORKQUEUE_H +#define TOR_WORKQUEUE_H + +#include "compat.h" + +typedef struct replyqueue_s replyqueue_t; +typedef struct threadpool_s threadpool_t; + + +#define WQ_CMD_RUN 0 +#define WQ_CMD_CANCEL 1 + +#define WQ_RPL_QUEUE 0 +#define WQ_RPL_NOQUEUE 1 +#define WQ_RPL_ERROR 2 +#define WQ_RPL_SHUTDOWN 3 + +void *threadpool_queue_work(threadpool_t *pool, + int (*fn)(int, void *, void *), + void (*reply_fn)(void *), + void *arg); +int threadpool_start_threads(threadpool_t *pool, int n); +threadpool_t *threadpool_new(int n_threads, + replyqueue_t *replyqueue, + void *(*new_thread_state_fn)(void*), + void (*free_thread_state_fn)(void*), + void *arg); +replyqueue_t *threadpool_get_replyqueue(threadpool_t *tp); + +replyqueue_t *replyqueue_new(void); +tor_socket_t replyqueue_get_socket(replyqueue_t *rq); +void replyqueue_process(replyqueue_t *queue); + +#endif diff --git a/src/test/bench_workqueue.c b/src/test/bench_workqueue.c new file mode 100644 index 0000000000..1bdfbefb3e --- /dev/null +++ b/src/test/bench_workqueue.c @@ -0,0 +1,298 @@ +/* Copyright (c) 2001-2004, Roger Dingledine. + * Copyright (c) 2004-2006, Roger Dingledine, Nick Mathewson. + * Copyright (c) 2007-2013, The Tor Project, Inc. */ +/* See LICENSE for licensing information */ + +#include "or.h" +#include "compat_threads.h" +#include "onion.h" +#include "workqueue.h" +#include "crypto.h" +#include "crypto_curve25519.h" +#include "compat_libevent.h" + +#include +#ifdef HAVE_EVENT2_EVENT_H +#include +#else +#include +#endif + +#ifdef TRACK_RESPONSES +tor_mutex_t bitmap_mutex; +int handled_len; +bitarray_t *handled; +#endif + +#define N_ITEMS 10000 +#define N_INFLIGHT 1000 +#define RELAUNCH_AT 250 + +typedef struct state_s { + int magic; + int n_handled; + crypto_pk_t *rsa; + curve25519_secret_key_t ecdh; +} state_t; + +typedef struct rsa_work_s { + int serial; + uint8_t msg[128]; + uint8_t msglen; +} rsa_work_t; + +typedef struct ecdh_work_s { + int serial; + union { + curve25519_public_key_t pk; + uint8_t msg[32]; + } u; +} ecdh_work_t; + +static void +mark_handled(int serial) +{ +#ifdef TRACK_RESPONSES + tor_mutex_acquire(&bitmap_mutex); + tor_assert(serial < handled_len); + tor_assert(! bitarray_is_set(handled, serial)); + bitarray_set(handled, serial); + tor_mutex_release(&bitmap_mutex); +#else + (void)serial; +#endif +} + +static int +workqueue_do_rsa(int cmd, void *state, void *work) +{ + rsa_work_t *rw = work; + state_t *st = state; + crypto_pk_t *rsa = st->rsa; + uint8_t sig[256]; + int len; + + tor_assert(st->magic == 13371337); + + if (cmd == WQ_CMD_CANCEL) { + tor_free(work); + return WQ_RPL_NOQUEUE; + } + + len = crypto_pk_private_sign(rsa, (char*)sig, 256, + (char*)rw->msg, rw->msglen); + if (len < 0) { + tor_free(work); + return WQ_RPL_NOQUEUE; + } + + memset(rw->msg, 0, sizeof(rw->msg)); + rw->msglen = len; + memcpy(rw->msg, sig, len); + ++st->n_handled; + + mark_handled(rw->serial); + + return WQ_RPL_QUEUE; +} + +#if 0 +static int +workqueue_do_shutdown(int cmd, void *state, void *work) +{ + (void)state; + (void)work; + (void)cmd; + crypto_pk_free(((state_t*)state)->rsa); + tor_free(state); + return WQ_RPL_SHUTDOWN; +} +#endif + +static int +workqueue_do_ecdh(int cmd, void *state, void *work) +{ + ecdh_work_t *ew = work; + uint8_t output[CURVE25519_OUTPUT_LEN]; + state_t *st = state; + + tor_assert(st->magic == 13371337); + + if (cmd == WQ_CMD_CANCEL) { + tor_free(work); + return WQ_RPL_NOQUEUE; + } + + curve25519_handshake(output, &st->ecdh, &ew->u.pk); + memcpy(ew->u.msg, output, CURVE25519_OUTPUT_LEN); + ++st->n_handled; + mark_handled(ew->serial); + return WQ_RPL_QUEUE; +} + +static void * +new_state(void *arg) +{ + state_t *st; + (void)arg; + + st = tor_malloc(sizeof(*st)); + /* Every thread gets its own keys. not a problem for benchmarking */ + st->rsa = crypto_pk_new(); + if (crypto_pk_generate_key_with_bits(st->rsa, 1024) < 0) { + puts("keygen failed"); + crypto_pk_free(st->rsa); + tor_free(st); + return NULL; + } + curve25519_secret_key_generate(&st->ecdh, 0); + st->magic = 13371337; + return st; +} + +static void +free_state(void *arg) +{ + state_t *st = arg; + crypto_pk_free(st->rsa); + tor_free(st); +} + +static tor_weak_rng_t weak_rng; +static int n_sent = 0; +static int rsa_sent = 0; +static int ecdh_sent = 0; +static int n_received = 0; + +#ifdef TRACK_RESPONSES +bitarray_t *received; +#endif + +static void +handle_reply(void *arg) +{ +#ifdef TRACK_RESPONSES + rsa_work_t *rw = arg; /* Naughty cast, but only looking at serial. */ + tor_assert(! bitarray_is_set(received, rw->serial)); + bitarray_set(received,rw->serial); +#endif + + tor_free(arg); + ++n_received; +} + +static int +add_work(threadpool_t *tp) +{ + int add_rsa = tor_weak_random_range(&weak_rng, 5) == 0; + if (add_rsa) { + rsa_work_t *w = tor_malloc_zero(sizeof(*w)); + w->serial = n_sent++; + crypto_rand((char*)w->msg, 20); + w->msglen = 20; + ++rsa_sent; + return threadpool_queue_work(tp, workqueue_do_rsa, handle_reply, w) != NULL; + } else { + ecdh_work_t *w = tor_malloc_zero(sizeof(*w)); + w->serial = n_sent++; + /* Not strictly right, but this is just for benchmarks. */ + crypto_rand((char*)w->u.pk.public_key, 32); + ++ecdh_sent; + return threadpool_queue_work(tp, workqueue_do_ecdh, handle_reply, w) != NULL; + } +} + +static void +replysock_readable_cb(tor_socket_t sock, short what, void *arg) +{ + threadpool_t *tp = arg; + replyqueue_t *rq = threadpool_get_replyqueue(tp); + + int old_r = n_received; + (void) sock; + (void) what; + + replyqueue_process(rq); + if (old_r == n_received) + return; + + printf("%d / %d\n", n_received, n_sent); +#ifdef TRACK_RESPONSES + tor_mutex_acquire(&bitmap_mutex); + for (i = 0; i < N_ITEMS; ++i) { + if (bitarray_is_set(received, i)) + putc('o', stdout); + else if (bitarray_is_set(handled, i)) + putc('!', stdout); + else + putc('.', stdout); + } + puts(""); + tor_mutex_release(&bitmap_mutex); +#endif + + if (n_sent - n_received < RELAUNCH_AT) { + while (n_sent < n_received + N_INFLIGHT && n_sent < N_ITEMS) { + if (! add_work(tp)) { + puts("Couldn't add work."); + tor_event_base_loopexit(tor_libevent_get_base(), NULL); + } + } + } + + if (n_received == n_sent && n_sent >= N_ITEMS) { + tor_event_base_loopexit(tor_libevent_get_base(), NULL); + } +} + +int +main(int argc, char **argv) +{ + replyqueue_t *rq; + threadpool_t *tp; + int i; + tor_libevent_cfg evcfg; + struct event *ev; + + (void)argc; + (void)argv; + + init_logging(1); + crypto_global_init(1, NULL, NULL); + crypto_seed_rng(1); + + rq = replyqueue_new(); + tor_assert(rq); + tp = threadpool_new(16, + rq, new_state, free_state, NULL); + tor_assert(tp); + + crypto_seed_weak_rng(&weak_rng); + + memset(&evcfg, 0, sizeof(evcfg)); + tor_libevent_initialize(&evcfg); + + ev = tor_event_new(tor_libevent_get_base(), + replyqueue_get_socket(rq), EV_READ|EV_PERSIST, + replysock_readable_cb, tp); + + event_add(ev, NULL); + +#ifdef TRACK_RESPONSES + handled = bitarray_init_zero(N_ITEMS); + received = bitarray_init_zero(N_ITEMS); + tor_mutex_init(&bitmap_mutex); + handled_len = N_ITEMS; +#endif + + for (i = 0; i < N_INFLIGHT; ++i) { + if (! add_work(tp)) { + puts("Couldn't add work."); + return 1; + } + } + + event_base_loop(tor_libevent_get_base(), 0); + + return 0; +} diff --git a/src/test/include.am b/src/test/include.am index b9b381fdae..6ad1b552b7 100644 --- a/src/test/include.am +++ b/src/test/include.am @@ -1,6 +1,6 @@ TESTS += src/test/test -noinst_PROGRAMS+= src/test/bench +noinst_PROGRAMS+= src/test/bench src/test/bench_workqueue if UNITTESTS_ENABLED noinst_PROGRAMS+= src/test/test src/test/test-child endif @@ -62,6 +62,9 @@ src_test_test_CPPFLAGS= $(src_test_AM_CPPFLAGS) src_test_bench_SOURCES = \ src/test/bench.c +src_test_bench_workqueue_SOURCES = \ + src/test/bench_workqueue.c + src_test_test_LDFLAGS = @TOR_LDFLAGS_zlib@ @TOR_LDFLAGS_openssl@ \ @TOR_LDFLAGS_libevent@ src_test_test_LDADD = src/or/libtor-testing.a src/common/libor-testing.a \ @@ -80,6 +83,14 @@ src_test_bench_LDADD = src/or/libtor.a src/common/libor.a \ @TOR_OPENSSL_LIBS@ @TOR_LIB_WS32@ @TOR_LIB_GDI@ @CURVE25519_LIBS@ \ @TOR_SYSTEMD_LIBS@ +src_test_bench_workqueue_LDFLAGS = @TOR_LDFLAGS_zlib@ @TOR_LDFLAGS_openssl@ \ + @TOR_LDFLAGS_libevent@ +src_test_bench_workqueue_LDADD = src/or/libtor.a src/common/libor.a \ + src/common/libor-crypto.a $(LIBDONNA) \ + src/common/libor-event.a \ + @TOR_ZLIB_LIBS@ @TOR_LIB_MATH@ @TOR_LIBEVENT_LIBS@ \ + @TOR_OPENSSL_LIBS@ @TOR_LIB_WS32@ @TOR_LIB_GDI@ @CURVE25519_LIBS@ + noinst_HEADERS+= \ src/test/fakechans.h \ src/test/test.h \ From c7eebe237ddf0555a99b2ef10fd95def2a4bbbd4 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 24 Sep 2013 16:57:40 -0400 Subject: [PATCH 06/32] Make pending work cancellable. --- src/common/workqueue.c | 72 ++++++++++++++++++++++++-------------- src/common/workqueue.h | 18 +++++----- src/test/bench_workqueue.c | 24 ++++--------- 3 files changed, 61 insertions(+), 53 deletions(-) diff --git a/src/common/workqueue.c b/src/common/workqueue.c index ea8dcb0f9b..80e061dfb5 100644 --- a/src/common/workqueue.c +++ b/src/common/workqueue.c @@ -31,16 +31,18 @@ keep array of threads; round-robin between them. */ -typedef struct workqueue_entry_s { - TOR_SIMPLEQ_ENTRY(workqueue_entry_s) next_work; - int (*fn)(int status, void *state, void *arg); +struct workqueue_entry_s { + TOR_TAILQ_ENTRY(workqueue_entry_s) next_work; + struct workerthread_s *on_thread; + uint8_t pending; + int (*fn)(void *state, void *arg); void (*reply_fn)(void *arg); void *arg; -} workqueue_entry_t; +}; struct replyqueue_s { tor_mutex_t lock; - TOR_SIMPLEQ_HEAD(, workqueue_entry_s) answers; + TOR_TAILQ_HEAD(, workqueue_entry_s) answers; void (*alert_fn)(struct replyqueue_s *); // lock not held on this, next 2. tor_socket_t write_sock; @@ -50,7 +52,7 @@ struct replyqueue_s { typedef struct workerthread_s { tor_mutex_t lock; tor_cond_t condition; - TOR_SIMPLEQ_HEAD(, workqueue_entry_s) work; + TOR_TAILQ_HEAD(, workqueue_entry_s) work; unsigned is_running; unsigned is_shut_down; unsigned waiting; @@ -76,7 +78,7 @@ struct threadpool_s { static void queue_reply(replyqueue_t *queue, workqueue_entry_t *work); static workqueue_entry_t * -workqueue_entry_new(int (*fn)(int, void*, void*), +workqueue_entry_new(int (*fn)(void*, void*), void (*reply_fn)(void*), void *arg) { @@ -95,6 +97,23 @@ workqueue_entry_free(workqueue_entry_t *ent) tor_free(ent); } +int +workqueue_entry_cancel(workqueue_entry_t *ent) +{ + int cancelled = 0; + tor_mutex_acquire(&ent->on_thread->lock); + if (ent->pending) { + TOR_TAILQ_REMOVE(&ent->on_thread->work, ent, next_work); + cancelled = 1; + } + tor_mutex_release(&ent->on_thread->lock); + + if (cancelled) { + tor_free(ent); + } + return cancelled; +} + static void worker_thread_main(void *thread_) { @@ -107,20 +126,17 @@ worker_thread_main(void *thread_) thread->is_running = 1; while (1) { /* lock held. */ - while (!TOR_SIMPLEQ_EMPTY(&thread->work)) { + while (!TOR_TAILQ_EMPTY(&thread->work)) { /* lock held. */ - work = TOR_SIMPLEQ_FIRST(&thread->work); - TOR_SIMPLEQ_REMOVE_HEAD(&thread->work, next_work); + work = TOR_TAILQ_FIRST(&thread->work); + TOR_TAILQ_REMOVE(&thread->work, work, next_work); + work->pending = 0; tor_mutex_release(&thread->lock); - result = work->fn(WQ_CMD_RUN, thread->state, work->arg); + result = work->fn(thread->state, work->arg); - if (result == WQ_RPL_QUEUE) { - queue_reply(thread->reply_queue, work); - } else { - workqueue_entry_free(work); - } + queue_reply(thread->reply_queue, work); tor_mutex_acquire(&thread->lock); if (result >= WQ_RPL_ERROR) { @@ -148,8 +164,8 @@ queue_reply(replyqueue_t *queue, workqueue_entry_t *work) { int was_empty; tor_mutex_acquire(&queue->lock); - was_empty = TOR_SIMPLEQ_EMPTY(&queue->answers); - TOR_SIMPLEQ_INSERT_TAIL(&queue->answers, work, next_work); + was_empty = TOR_TAILQ_EMPTY(&queue->answers); + TOR_TAILQ_INSERT_TAIL(&queue->answers, work, next_work); tor_mutex_release(&queue->lock); if (was_empty) { @@ -175,7 +191,7 @@ workerthread_new(void *state, replyqueue_t *replyqueue) workerthread_t *thr = tor_malloc_zero(sizeof(workerthread_t)); tor_mutex_init_for_cond(&thr->lock); tor_cond_init(&thr->condition); - TOR_SIMPLEQ_INIT(&thr->work); + TOR_TAILQ_INIT(&thr->work); thr->state = state; thr->reply_queue = replyqueue; @@ -187,9 +203,9 @@ workerthread_new(void *state, replyqueue_t *replyqueue) return thr; } -void * +workqueue_entry_t * threadpool_queue_work(threadpool_t *pool, - int (*fn)(int, void *, void *), + int (*fn)(void *, void *), void (*reply_fn)(void *), void *arg) { @@ -206,11 +222,12 @@ threadpool_queue_work(threadpool_t *pool, pool->next_for_work = 0; tor_mutex_release(&pool->lock); - ent = workqueue_entry_new(fn, reply_fn, arg); tor_mutex_acquire(&worker->lock); - TOR_SIMPLEQ_INSERT_TAIL(&worker->work, ent, next_work); + ent->on_thread = worker; + ent->pending = 1; + TOR_TAILQ_INSERT_TAIL(&worker->work, ent, next_work); if (worker->waiting) /* XXXX inside or outside of lock?? */ tor_cond_signal_one(&worker->condition); @@ -298,7 +315,7 @@ replyqueue_new(void) rq = tor_malloc_zero(sizeof(replyqueue_t)); tor_mutex_init(&rq->lock); - TOR_SIMPLEQ_INIT(&rq->answers); + TOR_TAILQ_INIT(&rq->answers); rq->read_sock = pair[0]; rq->write_sock = pair[1]; @@ -331,10 +348,10 @@ replyqueue_process(replyqueue_t *queue) /* XXXX freak out on r == 0, or r == "error, not retryable". */ tor_mutex_acquire(&queue->lock); - while (!TOR_SIMPLEQ_EMPTY(&queue->answers)) { + while (!TOR_TAILQ_EMPTY(&queue->answers)) { /* lock held. */ - workqueue_entry_t *work = TOR_SIMPLEQ_FIRST(&queue->answers); - TOR_SIMPLEQ_REMOVE_HEAD(&queue->answers, next_work); + workqueue_entry_t *work = TOR_TAILQ_FIRST(&queue->answers); + TOR_TAILQ_REMOVE(&queue->answers, work, next_work); tor_mutex_release(&queue->lock); work->reply_fn(work->arg); @@ -345,3 +362,4 @@ replyqueue_process(replyqueue_t *queue) tor_mutex_release(&queue->lock); } + diff --git a/src/common/workqueue.h b/src/common/workqueue.h index e502734b84..47753cff12 100644 --- a/src/common/workqueue.h +++ b/src/common/workqueue.h @@ -8,20 +8,20 @@ typedef struct replyqueue_s replyqueue_t; typedef struct threadpool_s threadpool_t; - +typedef struct workqueue_entry_s workqueue_entry_t; #define WQ_CMD_RUN 0 #define WQ_CMD_CANCEL 1 -#define WQ_RPL_QUEUE 0 -#define WQ_RPL_NOQUEUE 1 -#define WQ_RPL_ERROR 2 -#define WQ_RPL_SHUTDOWN 3 +#define WQ_RPL_REPLY 0 +#define WQ_RPL_ERROR 1 +#define WQ_RPL_SHUTDOWN 2 -void *threadpool_queue_work(threadpool_t *pool, - int (*fn)(int, void *, void *), - void (*reply_fn)(void *), - void *arg); +workqueue_entry_t *threadpool_queue_work(threadpool_t *pool, + int (*fn)(void *, void *), + void (*reply_fn)(void *), + void *arg); +int workqueue_entry_cancel(workqueue_entry_t *pending_work); int threadpool_start_threads(threadpool_t *pool, int n); threadpool_t *threadpool_new(int n_threads, replyqueue_t *replyqueue, diff --git a/src/test/bench_workqueue.c b/src/test/bench_workqueue.c index 1bdfbefb3e..f190c613e5 100644 --- a/src/test/bench_workqueue.c +++ b/src/test/bench_workqueue.c @@ -64,7 +64,7 @@ mark_handled(int serial) } static int -workqueue_do_rsa(int cmd, void *state, void *work) +workqueue_do_rsa(void *state, void *work) { rsa_work_t *rw = work; state_t *st = state; @@ -74,16 +74,11 @@ workqueue_do_rsa(int cmd, void *state, void *work) tor_assert(st->magic == 13371337); - if (cmd == WQ_CMD_CANCEL) { - tor_free(work); - return WQ_RPL_NOQUEUE; - } - len = crypto_pk_private_sign(rsa, (char*)sig, 256, (char*)rw->msg, rw->msglen); if (len < 0) { - tor_free(work); - return WQ_RPL_NOQUEUE; + rw->msglen = 0; + return WQ_RPL_ERROR; } memset(rw->msg, 0, sizeof(rw->msg)); @@ -93,12 +88,12 @@ workqueue_do_rsa(int cmd, void *state, void *work) mark_handled(rw->serial); - return WQ_RPL_QUEUE; + return WQ_RPL_REPLY; } #if 0 static int -workqueue_do_shutdown(int cmd, void *state, void *work) +workqueue_do_shutdown(void *state, void *work) { (void)state; (void)work; @@ -110,7 +105,7 @@ workqueue_do_shutdown(int cmd, void *state, void *work) #endif static int -workqueue_do_ecdh(int cmd, void *state, void *work) +workqueue_do_ecdh(void *state, void *work) { ecdh_work_t *ew = work; uint8_t output[CURVE25519_OUTPUT_LEN]; @@ -118,16 +113,11 @@ workqueue_do_ecdh(int cmd, void *state, void *work) tor_assert(st->magic == 13371337); - if (cmd == WQ_CMD_CANCEL) { - tor_free(work); - return WQ_RPL_NOQUEUE; - } - curve25519_handshake(output, &st->ecdh, &ew->u.pk); memcpy(ew->u.msg, output, CURVE25519_OUTPUT_LEN); ++st->n_handled; mark_handled(ew->serial); - return WQ_RPL_QUEUE; + return WQ_RPL_REPLY; } static void * From 51bc0e7f3d612b099382500b434d31f179eaa8a8 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 24 Sep 2013 20:43:48 -0400 Subject: [PATCH 07/32] Isolate the "socketpair or a pipe" logic for alerting main thread This way we can use the linux eventfd extension where available. Using EVFILT_USER on the BSDs will be a teeny bit trickier, and will require libevent hacking. --- configure.ac | 4 + src/common/compat_threads.c | 150 ++++++++++++++++++++++++++++++++++++ src/common/compat_threads.h | 11 +++ src/common/workqueue.c | 68 +++------------- 4 files changed, 177 insertions(+), 56 deletions(-) diff --git a/configure.ac b/configure.ac index 65b3ff245c..69a266a717 100644 --- a/configure.ac +++ b/configure.ac @@ -407,6 +407,7 @@ AC_CHECK_FUNCS( backtrace \ backtrace_symbols_fd \ clock_gettime \ + eventfd \ flock \ ftime \ getaddrinfo \ @@ -421,6 +422,8 @@ AC_CHECK_FUNCS( localtime_r \ lround \ memmem \ + pipe \ + pipe2 \ prctl \ rint \ sigaction \ @@ -962,6 +965,7 @@ AC_CHECK_HEADERS( netinet/in6.h \ pwd.h \ stdint.h \ + sys/eventfd.h \ sys/file.h \ sys/ioctl.h \ sys/limits.h \ diff --git a/src/common/compat_threads.c b/src/common/compat_threads.c index e0cbf5c1d8..98bdbbcf5e 100644 --- a/src/common/compat_threads.c +++ b/src/common/compat_threads.c @@ -3,8 +3,24 @@ * Copyright (c) 2007-2015, The Tor Project, Inc. */ /* See LICENSE for licensing information */ +#include "orconfig.h" +#define _GNU_SOURCE +#include #include "compat.h" +#include "compat_threads.h" + #include "util.h" +#include "torlog.h" + +#ifdef HAVE_SYS_EVENTFD_H +#include +#endif +#ifdef HAVE_UNISTD_H +#include +#endif +#ifdef HAVE_FCNTL_H +#include +#endif /** Return a newly allocated, ready-for-use mutex. */ tor_mutex_t * @@ -57,3 +73,137 @@ in_main_thread(void) { return main_thread_id == tor_get_thread_id(); } + +#ifdef HAVE_EVENTFD +static int +eventfd_alert(int fd) +{ + uint64_t u = 1; + int r = write(fd, (void*)&u, sizeof(u)); + if (r < 0 && errno != EAGAIN) + return -1; + return 0; +} + +static int +eventfd_drain(int fd) +{ + uint64_t u = 0; + int r = read(fd, (void*)&u, sizeof(u)); + if (r < 0 && errno != EAGAIN) + return -1; + return 0; +} +#endif + +#ifdef HAVE_PIPE +static int +pipe_alert(int fd) +{ + ssize_t r = write(fd, "x", 1); + if (r < 0 && errno != EAGAIN) + return -1; + return 0; +} + +static int +pipe_drain(int fd) +{ + char buf[32]; + ssize_t r; + while ((r = read(fd, buf, sizeof(buf))) >= 0) + ; + if (r == 0 || errno != EAGAIN) + return -1; + return 0; +} +#endif + +static int +sock_alert(tor_socket_t fd) +{ + ssize_t r = send(fd, "x", 1, 0); + if (r < 0 && !ERRNO_IS_EAGAIN(tor_socket_errno(fd))) + return -1; + return 0; +} + +static int +sock_drain(tor_socket_t fd) +{ + char buf[32]; + ssize_t r; + while ((r = recv(fd, buf, sizeof(buf), 0)) >= 0) + ; + if (r == 0 || !ERRNO_IS_EAGAIN(tor_socket_errno(fd))) + return -1; + return 0; +} + +/** Allocate a new set of alert sockets. DOCDOC */ +int +alert_sockets_create(alert_sockets_t *socks_out) +{ + tor_socket_t socks[2]; + +#ifdef HAVE_EVENTFD +#if defined(EFD_CLOEXEC) && defined(EFD_NONBLOCK) + socks[0] = eventfd(0, EFD_CLOEXEC|EFD_NONBLOCK); +#else + socks[0] = -1; +#endif + if (socks[0] < 0) { + socks[0] = eventfd(0,0); + if (socks[0] >= 0) { + if (fcntl(socks[0], F_SETFD, FD_CLOEXEC) < 0 || + set_socket_nonblocking(socks[0]) < 0) { + close(socks[0]); + return -1; + } + } + } + if (socks[0] >= 0) { + socks_out->read_fd = socks_out->write_fd = socks[0]; + socks_out->alert_fn = eventfd_alert; + socks_out->drain_fn = eventfd_drain; + return 0; + } +#endif + +#ifdef HAVE_PIPE2 + if (pipe2(socks, O_NONBLOCK|O_CLOEXEC) == 0) { + socks_out->read_fd = socks[0]; + socks_out->write_fd = socks[1]; + socks_out->alert_fn = pipe_alert; + socks_out->drain_fn = pipe_drain; + return 0; + } +#endif + +#ifdef HAVE_PIPE + if (pipe(socks) == 0) { + if (fcntl(socks[0], F_SETFD, FD_CLOEXEC) < 0 || + fcntl(socks[1], F_SETFD, FD_CLOEXEC) < 0 || + set_socket_nonblocking(socks[0]) < 0 || + set_socket_nonblocking(socks[1]) < 0) { + close(socks[0]); + close(socks[1]); + return -1; + } + socks_out->read_fd = socks[0]; + socks_out->write_fd = socks[1]; + socks_out->alert_fn = pipe_alert; + socks_out->drain_fn = pipe_drain; + return 0; + } +#endif + + if (tor_socketpair(AF_UNIX, SOCK_STREAM, 0, socks) == 0) { + set_socket_nonblocking(socks[0]); + set_socket_nonblocking(socks[1]); + socks_out->alert_fn = sock_alert; + socks_out->drain_fn = sock_drain; + return 0; + } + return -1; +} diff --git a/src/common/compat_threads.h b/src/common/compat_threads.h index 581d8dd7b9..b053136c15 100644 --- a/src/common/compat_threads.h +++ b/src/common/compat_threads.h @@ -82,4 +82,15 @@ int tor_cond_wait(tor_cond_t *cond, tor_mutex_t *mutex, void tor_cond_signal_one(tor_cond_t *cond); void tor_cond_signal_all(tor_cond_t *cond); +/** DOCDOC */ +typedef struct alert_sockets_s { + /*XXX needs a better name */ + tor_socket_t read_fd; + tor_socket_t write_fd; + int (*alert_fn)(tor_socket_t write_fd); + int (*drain_fn)(tor_socket_t read_fd); +} alert_sockets_t; + +int alert_sockets_create(alert_sockets_t *socks_out); + #endif diff --git a/src/common/workqueue.c b/src/common/workqueue.c index 80e061dfb5..ed70d5e5fc 100644 --- a/src/common/workqueue.c +++ b/src/common/workqueue.c @@ -9,11 +9,6 @@ #include "tor_queue.h" #include "torlog.h" -#ifdef HAVE_UNISTD_H -// XXXX move wherever we move the write/send stuff -#include -#endif - /* design: @@ -44,9 +39,7 @@ struct replyqueue_s { tor_mutex_t lock; TOR_TAILQ_HEAD(, workqueue_entry_s) answers; - void (*alert_fn)(struct replyqueue_s *); // lock not held on this, next 2. - tor_socket_t write_sock; - tor_socket_t read_sock; + alert_sockets_t alert; // lock not held on this. }; typedef struct workerthread_s { @@ -169,22 +162,12 @@ queue_reply(replyqueue_t *queue, workqueue_entry_t *work) tor_mutex_release(&queue->lock); if (was_empty) { - queue->alert_fn(queue); + if (queue->alert.alert_fn(queue->alert.write_fd) < 0) { + /* XXXX complain! */ + } } } - -static void -alert_by_fd(replyqueue_t *queue) -{ - /* XXX extract this into new function */ -#ifndef _WIN32 - (void) send(queue->write_sock, "x", 1, 0); -#else - (void) write(queue->write_sock, "x", 1); -#endif -} - static workerthread_t * workerthread_new(void *state, replyqueue_t *replyqueue) { @@ -293,59 +276,32 @@ threadpool_get_replyqueue(threadpool_t *tp) replyqueue_t * replyqueue_new(void) { - tor_socket_t pair[2]; replyqueue_t *rq; - int r; - - /* XXX extract this into new function */ -#ifdef _WIN32 - r = tor_socketpair(AF_UNIX, SOCK_STREAM, 0, pair); -#else - r = pipe(pair); -#endif - if (r < 0) - return NULL; - - set_socket_nonblocking(pair[0]); /* the read-size should be nonblocking. */ -#if defined(FD_CLOEXEC) - fcntl(pair[0], F_SETFD, FD_CLOEXEC); - fcntl(pair[1], F_SETFD, FD_CLOEXEC); -#endif rq = tor_malloc_zero(sizeof(replyqueue_t)); + if (alert_sockets_create(&rq->alert) < 0) { + tor_free(rq); + return NULL; + } tor_mutex_init(&rq->lock); TOR_TAILQ_INIT(&rq->answers); - rq->read_sock = pair[0]; - rq->write_sock = pair[1]; - rq->alert_fn = alert_by_fd; - return rq; } tor_socket_t replyqueue_get_socket(replyqueue_t *rq) { - return rq->read_sock; + return rq->alert.read_fd; } void replyqueue_process(replyqueue_t *queue) { - ssize_t r; - - /* XXX extract this into new function */ - do { - char buf[64]; -#ifdef _WIN32 - r = recv(queue->read_sock, buf, sizeof(buf), 0); -#else - r = read(queue->read_sock, buf, sizeof(buf)); -#endif - } while (r > 0); - - /* XXXX freak out on r == 0, or r == "error, not retryable". */ + if (queue->alert.drain_fn(queue->alert.read_fd) < 0) { + /* XXXX complain! */ + } tor_mutex_acquire(&queue->lock); while (!TOR_TAILQ_EMPTY(&queue->answers)) { From 4abbf13f99dac9e15856dc4e458a8c9525acab4d Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 24 Sep 2013 20:55:09 -0400 Subject: [PATCH 08/32] Add a way to tell all threads to do something. --- src/common/workqueue.c | 59 ++++++++++++++++++++++++++++++++---------- src/common/workqueue.h | 6 ++++- 2 files changed, 50 insertions(+), 15 deletions(-) diff --git a/src/common/workqueue.c b/src/common/workqueue.c index ed70d5e5fc..c4b64de58b 100644 --- a/src/common/workqueue.c +++ b/src/common/workqueue.c @@ -186,13 +186,32 @@ workerthread_new(void *state, replyqueue_t *replyqueue) return thr; } +static workqueue_entry_t * +workerthread_queue_work(workerthread_t *worker, + int (*fn)(void *, void *), + void (*reply_fn)(void *), + void *arg) +{ + workqueue_entry_t *ent = workqueue_entry_new(fn, reply_fn, arg); + + tor_mutex_acquire(&worker->lock); + ent->on_thread = worker; + ent->pending = 1; + TOR_TAILQ_INSERT_TAIL(&worker->work, ent, next_work); + + if (worker->waiting) /* XXXX inside or outside of lock?? */ + tor_cond_signal_one(&worker->condition); + + tor_mutex_release(&worker->lock); + return ent; +} + workqueue_entry_t * threadpool_queue_work(threadpool_t *pool, int (*fn)(void *, void *), void (*reply_fn)(void *), void *arg) { - workqueue_entry_t *ent; workerthread_t *worker; tor_mutex_acquire(&pool->lock); @@ -205,22 +224,34 @@ threadpool_queue_work(threadpool_t *pool, pool->next_for_work = 0; tor_mutex_release(&pool->lock); - ent = workqueue_entry_new(fn, reply_fn, arg); - - tor_mutex_acquire(&worker->lock); - ent->on_thread = worker; - ent->pending = 1; - TOR_TAILQ_INSERT_TAIL(&worker->work, ent, next_work); - - if (worker->waiting) /* XXXX inside or outside of lock?? */ - tor_cond_signal_one(&worker->condition); - - tor_mutex_release(&worker->lock); - - return ent; + return workerthread_queue_work(worker, fn, reply_fn, arg); } int +threadpool_queue_for_all(threadpool_t *pool, + void *(*dup_fn)(void *), + int (*fn)(void *, void *), + void (*reply_fn)(void *), + void *arg) +{ + int i = 0; + workerthread_t *worker; + void *arg_copy; + while (1) { + tor_mutex_acquire(&pool->lock); + if (i >= pool->n_threads) { + tor_mutex_release(&pool->lock); + return 0; + } + worker = pool->threads[i++]; + tor_mutex_release(&pool->lock); + + arg_copy = dup_fn ? dup_fn(arg) : arg; + /* CHECK*/ workerthread_queue_work(worker, fn, reply_fn, arg_copy); + } +} + +static int threadpool_start_threads(threadpool_t *pool, int n) { tor_mutex_acquire(&pool->lock); diff --git a/src/common/workqueue.h b/src/common/workqueue.h index 47753cff12..684fb192ba 100644 --- a/src/common/workqueue.h +++ b/src/common/workqueue.h @@ -21,8 +21,12 @@ workqueue_entry_t *threadpool_queue_work(threadpool_t *pool, int (*fn)(void *, void *), void (*reply_fn)(void *), void *arg); +int threadpool_queue_for_all(threadpool_t *pool, + void *(*dup_fn)(void *), + int (*fn)(void *, void *), + void (*reply_fn)(void *), + void *arg); int workqueue_entry_cancel(workqueue_entry_t *pending_work); -int threadpool_start_threads(threadpool_t *pool, int n); threadpool_t *threadpool_new(int n_threads, replyqueue_t *replyqueue, void *(*new_thread_state_fn)(void*), From b2db3fb4627c8bd06489334f69b6d36d60fb418d Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 25 Sep 2013 11:05:27 -0400 Subject: [PATCH 09/32] Documentation for new workqueue and condition and locking stuff --- src/common/compat_pthreads.c | 11 ++- src/common/compat_threads.c | 39 +++++++- src/common/compat_threads.h | 12 ++- src/common/workqueue.c | 178 +++++++++++++++++++++++++++-------- src/common/workqueue.h | 14 ++- 5 files changed, 205 insertions(+), 49 deletions(-) diff --git a/src/common/compat_pthreads.c b/src/common/compat_pthreads.c index 59b54a600a..a2e406521f 100644 --- a/src/common/compat_pthreads.c +++ b/src/common/compat_pthreads.c @@ -164,6 +164,7 @@ tor_get_thread_id(void) /* Conditions. */ +/** Initialize an already-allocated condition variable. */ int tor_cond_init(tor_cond_t *cond) { @@ -173,7 +174,9 @@ tor_cond_init(tor_cond_t *cond) } return 0; } -/** Release all resources held by cond. */ + +/** Release all resources held by cond, but do not free cond + * itself. */ void tor_cond_uninit(tor_cond_t *cond) { @@ -183,7 +186,11 @@ tor_cond_uninit(tor_cond_t *cond) } } /** Wait until one of the tor_cond_signal functions is called on cond. - * All waiters on the condition must wait holding the same mutex. + * (If tv is set, and that amount of time passes with no signal to + * cond, return anyway. All waiters on the condition must wait holding + * the same mutex. All signallers should hold that mutex. The mutex + * needs to have been allocated with tor_mutex_init_for_cond(). + * * Returns 0 on success, -1 on failure, 1 on timeout. */ int tor_cond_wait(tor_cond_t *cond, tor_mutex_t *mutex, const struct timeval *tv) diff --git a/src/common/compat_threads.c b/src/common/compat_threads.c index 98bdbbcf5e..024c627cf1 100644 --- a/src/common/compat_threads.c +++ b/src/common/compat_threads.c @@ -40,6 +40,7 @@ tor_mutex_free(tor_mutex_t *m) tor_free(m); } +/** Allocate and return a new condition variable. */ tor_cond_t * tor_cond_new(void) { @@ -48,6 +49,8 @@ tor_cond_new(void) tor_free(cond); return cond; } + +/** Free all storage held in c. */ void tor_cond_free(tor_cond_t *c) { @@ -140,13 +143,16 @@ sock_drain(tor_socket_t fd) return 0; } -/** Allocate a new set of alert sockets. DOCDOC */ +/** Allocate a new set of alert sockets, and set the appropriate function + * pointers, in socks_out. */ int alert_sockets_create(alert_sockets_t *socks_out) { tor_socket_t socks[2]; #ifdef HAVE_EVENTFD + /* First, we try the Linux eventfd() syscall. This gives a 64-bit counter + * associated with a single file descriptor. */ #if defined(EFD_CLOEXEC) && defined(EFD_NONBLOCK) socks[0] = eventfd(0, EFD_CLOEXEC|EFD_NONBLOCK); #else @@ -171,6 +177,8 @@ alert_sockets_create(alert_sockets_t *socks_out) #endif #ifdef HAVE_PIPE2 + /* Now we're going to try pipes. First type the pipe2() syscall, if we + * have it, so we can save some calls... */ if (pipe2(socks, O_NONBLOCK|O_CLOEXEC) == 0) { socks_out->read_fd = socks[0]; socks_out->write_fd = socks[1]; @@ -181,6 +189,8 @@ alert_sockets_create(alert_sockets_t *socks_out) #endif #ifdef HAVE_PIPE + /* Now try the regular pipe() syscall. Pipes have a bit lower overhead than + * socketpairs, fwict. */ if (pipe(socks) == 0) { if (fcntl(socks[0], F_SETFD, FD_CLOEXEC) < 0 || fcntl(socks[1], F_SETFD, FD_CLOEXEC) < 0 || @@ -198,12 +208,35 @@ alert_sockets_create(alert_sockets_t *socks_out) } #endif + /* If nothing else worked, fall back on socketpair(). */ if (tor_socketpair(AF_UNIX, SOCK_STREAM, 0, socks) == 0) { - set_socket_nonblocking(socks[0]); - set_socket_nonblocking(socks[1]); + if (set_socket_nonblocking(socks[0]) < 0 || + set_socket_nonblocking(socks[1])) { + tor_close_socket(socks[0]); + tor_close_socket(socks[1]); + return -1; + } + socks_out->read_fd = socks[0]; + socks_out->write_fd = socks[1]; socks_out->alert_fn = sock_alert; socks_out->drain_fn = sock_drain; return 0; } return -1; } + +/** Close the sockets in socks. */ +void +alert_sockets_close(alert_sockets_t *socks) +{ + if (socks->alert_fn == sock_alert) { + /* they are sockets. */ + tor_close_socket(socks->read_fd); + tor_close_socket(socks->write_fd); + } else { + close(socks->read_fd); + if (socks->write_fd != socks->read_fd) + close(socks->write_fd); + } + socks->read_fd = socks->write_fd = -1; +} diff --git a/src/common/compat_threads.h b/src/common/compat_threads.h index b053136c15..9070f13e80 100644 --- a/src/common/compat_threads.h +++ b/src/common/compat_threads.h @@ -82,15 +82,23 @@ int tor_cond_wait(tor_cond_t *cond, tor_mutex_t *mutex, void tor_cond_signal_one(tor_cond_t *cond); void tor_cond_signal_all(tor_cond_t *cond); -/** DOCDOC */ +/** Helper type used to manage waking up the main thread while it's in + * the libevent main loop. Used by the work queue code. */ typedef struct alert_sockets_s { - /*XXX needs a better name */ + /* XXXX This structure needs a better name. */ + /** Socket that the main thread should listen for EV_READ events on. + * Note that this socket may be a regular fd on a non-Windows platform. + */ tor_socket_t read_fd; + /** Socket to use when alerting the main thread. */ tor_socket_t write_fd; + /** Function to alert the main thread */ int (*alert_fn)(tor_socket_t write_fd); + /** Function to make the main thread no longer alerted. */ int (*drain_fn)(tor_socket_t read_fd); } alert_sockets_t; int alert_sockets_create(alert_sockets_t *socks_out); +void alert_sockets_close(alert_sockets_t *socks); #endif diff --git a/src/common/workqueue.c b/src/common/workqueue.c index c4b64de58b..e07787b404 100644 --- a/src/common/workqueue.c +++ b/src/common/workqueue.c @@ -9,67 +9,84 @@ #include "tor_queue.h" #include "torlog.h" -/* - design: +struct threadpool_s { + /** An array of pointers to workerthread_t: one for each running worker + * thread. */ + struct workerthread_s **threads; + /** Index of the next thread that we'll give work to.*/ + int next_for_work; - each thread has its own queue, try to keep at least elements min..max cycles - worth of work on each queue. + /** Number of elements in threads. */ + int n_threads; + /** Mutex to protect all the above fields. */ + tor_mutex_t lock; -keep array of threads; round-robin between them. + /** A reply queue to use when constructing new threads. */ + replyqueue_t *reply_queue; - When out of work, work-steal. - - alert threads with condition variables. - - alert main thread with fd, since it's libevent. - - - */ + /** Functions used to allocate and free thread state. */ + void *(*new_thread_state_fn)(void*); + void (*free_thread_state_fn)(void*); + void *new_thread_state_arg; +}; struct workqueue_entry_s { + /** The next workqueue_entry_t that's pending on the same thread or + * reply queue. */ TOR_TAILQ_ENTRY(workqueue_entry_s) next_work; + /** The thread to which this workqueue_entry_t was assigned. This field + * is set when the workqueue_entry_t is created, and won't be cleared until + * after it's handled in the main thread. */ struct workerthread_s *on_thread; + /** True iff this entry is waiting for a worker to start processing it. */ uint8_t pending; + /** Function to run in the worker thread. */ int (*fn)(void *state, void *arg); + /** Function to run while processing the reply queue. */ void (*reply_fn)(void *arg); + /** Argument for the above functions. */ void *arg; }; struct replyqueue_s { + /** Mutex to protect the answers field */ tor_mutex_t lock; + /** Doubly-linked list of answers that the reply queue needs to handle. */ TOR_TAILQ_HEAD(, workqueue_entry_s) answers; - alert_sockets_t alert; // lock not held on this. + /** Mechanism to wake up the main thread when it is receiving answers. */ + alert_sockets_t alert; }; +/** A worker thread represents a single thread in a thread pool. To avoid + * contention, each gets its own queue. This breaks the guarantee that that + * queued work will get executed strictly in order. */ typedef struct workerthread_s { + /** Lock to protect all fields of this thread and its queue. */ tor_mutex_t lock; + /** Condition variable that we wait on when we have no work, and which + * gets signaled when our queue becomes nonempty. */ tor_cond_t condition; + /** Queue of pending work that we have to do. */ TOR_TAILQ_HEAD(, workqueue_entry_s) work; + /** True iff this thread is currently in its loop. */ unsigned is_running; + /** True iff this thread has crashed or is shut down for some reason. */ unsigned is_shut_down; + /** True if we're waiting for more elements to get added to the queue. */ unsigned waiting; + /** User-supplied state field that we pass to the worker functions of each + * work item. */ void *state; + /** Reply queue to which we pass our results. */ replyqueue_t *reply_queue; } workerthread_t; -struct threadpool_s { - workerthread_t **threads; - int next_for_work; - - tor_mutex_t lock; - int n_threads; - - replyqueue_t *reply_queue; - - void *(*new_thread_state_fn)(void*); - void (*free_thread_state_fn)(void*); - void *new_thread_state_arg; - -}; - static void queue_reply(replyqueue_t *queue, workqueue_entry_t *work); +/** Allocate and return a new workqueue_entry_t, set up to run the function + * fn in the worker thread, and reply_fn in the main + * thread. See threadpool_queue_work() for full documentation. */ static workqueue_entry_t * workqueue_entry_new(int (*fn)(void*, void*), void (*reply_fn)(void*), @@ -82,6 +99,10 @@ workqueue_entry_new(int (*fn)(void*, void*), return ent; } +/** + * Release all storage held in ent. Call only when ent is not on + * any queue. + */ static void workqueue_entry_free(workqueue_entry_t *ent) { @@ -90,6 +111,20 @@ workqueue_entry_free(workqueue_entry_t *ent) tor_free(ent); } +/** + * Cancel a workqueue_entry_t that has been returned from + * threadpool_queue_work. + * + * You must not call this function on any work whose reply function has been + * executed in the main thread; that will cause undefined behavior (probably, + * a crash). + * + * If the work is cancelled, this function return 1. It is the caller's + * responsibility to free any storage in the work function's arguments. + * + * This function will have no effect if the worker thread has already executed + * or begun to execute the work item. In that case, it will return 0. + */ int workqueue_entry_cancel(workqueue_entry_t *ent) { @@ -107,6 +142,9 @@ workqueue_entry_cancel(workqueue_entry_t *ent) return cancelled; } +/** + * Main function for the worker thread. + */ static void worker_thread_main(void *thread_) { @@ -115,23 +153,26 @@ worker_thread_main(void *thread_) int result; tor_mutex_acquire(&thread->lock); - thread->is_running = 1; while (1) { - /* lock held. */ + /* lock must be held at this point. */ while (!TOR_TAILQ_EMPTY(&thread->work)) { - /* lock held. */ + /* lock must be held at this point. */ work = TOR_TAILQ_FIRST(&thread->work); TOR_TAILQ_REMOVE(&thread->work, work, next_work); work->pending = 0; tor_mutex_release(&thread->lock); + /* We run the work function without holding the thread lock. This + * is the main thread's first opportunity to give us more work. */ result = work->fn(thread->state, work->arg); + /* Queue the reply for the main thread. */ queue_reply(thread->reply_queue, work); tor_mutex_acquire(&thread->lock); + /* We may need to exit the thread. */ if (result >= WQ_RPL_ERROR) { thread->is_running = 0; thread->is_shut_down = 1; @@ -139,19 +180,23 @@ worker_thread_main(void *thread_) return; } } - /* Lock held; no work in this thread's queue. */ + /* At this point the lock is held, and there is no work in this thread's + * queue. */ /* TODO: Try work-stealing. */ - /* TODO: support an idle-function */ + /* Okay. Now, wait till somebody has work for us. */ thread->waiting = 1; - if (tor_cond_wait(&thread->condition, &thread->lock, NULL) < 0) - /* ERR */ + if (tor_cond_wait(&thread->condition, &thread->lock, NULL) < 0) { + /* XXXX ERROR */ + } thread->waiting = 0; } } +/** Put a reply on the reply queue. The reply must not currently be on + * any thread's work queue. */ static void queue_reply(replyqueue_t *queue, workqueue_entry_t *work) { @@ -168,6 +213,8 @@ queue_reply(replyqueue_t *queue, workqueue_entry_t *work) } } +/** Allocate and start a new worker thread to use state object state, + * and send responses to replyqueue. */ static workerthread_t * workerthread_new(void *state, replyqueue_t *replyqueue) { @@ -186,6 +233,10 @@ workerthread_new(void *state, replyqueue_t *replyqueue) return thr; } +/** + * Add an item of work to a single worker thread. See threadpool_queue_work(*) + * for arguments. + */ static workqueue_entry_t * workerthread_queue_work(workerthread_t *worker, int (*fn)(void *, void *), @@ -206,6 +257,23 @@ workerthread_queue_work(workerthread_t *worker, return ent; } +/** + * Queue an item of work for a thread in a thread pool. The function + * fn will be run in a worker thread, and will receive as arguments the + * thread's state object, and the provided object arg. It must return + * one of WQ_RPL_REPLY, WQ_RPL_ERROR, or WQ_RPL_SHUTDOWN. + * + * Regardless of its return value, the function reply_fn will later be + * run in the main thread when it invokes replyqueue_process(), and will + * receive as its argument the same arg object. It's the reply + * function's responsibility to free the work object. + * + * On success, return a workqueue_entry_t object that can be passed to + * workqueue_entry_cancel(). On failure, return NULL. + * + * Note that because each thread has its own work queue, work items may not + * be executed strictly in order. + */ workqueue_entry_t * threadpool_queue_work(threadpool_t *pool, int (*fn)(void *, void *), @@ -215,6 +283,7 @@ threadpool_queue_work(threadpool_t *pool, workerthread_t *worker; tor_mutex_acquire(&pool->lock); + /* Pick the next thread in random-access order. */ worker = pool->threads[pool->next_for_work++]; if (!worker) { tor_mutex_release(&pool->lock); @@ -227,9 +296,19 @@ threadpool_queue_work(threadpool_t *pool, return workerthread_queue_work(worker, fn, reply_fn, arg); } +/** + * Queue a copy of a work item for every thread in a pool. This can be used, + * for example, to tell the threads to update some parameter in their states. + * + * Arguments are as for threadpool_queue_work, except that the + * arg value is passed to dup_fn once per each thread to + * make a copy of it. + * + * Return 0 on success, -1 on failure. + */ int threadpool_queue_for_all(threadpool_t *pool, - void *(*dup_fn)(void *), + void *(*dup_fn)(const void *), int (*fn)(void *, void *), void (*reply_fn)(void *), void *arg) @@ -251,6 +330,7 @@ threadpool_queue_for_all(threadpool_t *pool, } } +/** Launch threads until we have n. */ static int threadpool_start_threads(threadpool_t *pool, int n) { @@ -274,6 +354,13 @@ threadpool_start_threads(threadpool_t *pool, int n) return 0; } +/** + * Construct a new thread pool with n worker threads, configured to + * send their output to replyqueue. The threads' states will be + * constructed with the new_thread_state_fn call, receiving arg + * as its argument. When the threads close, they will call + * free_thread_state_fn on their states. + */ threadpool_t * threadpool_new(int n_threads, replyqueue_t *replyqueue, @@ -298,12 +385,17 @@ threadpool_new(int n_threads, return pool; } +/** Return the reply queue associated with a given thread pool. */ replyqueue_t * threadpool_get_replyqueue(threadpool_t *tp) { return tp->reply_queue; } +/** Allocate a new reply queue. Reply queues are used to pass results from + * worker threads to the main thread. Since the main thread is running an + * IO-centric event loop, it needs to get woken up with means other than a + * condition variable. */ replyqueue_t * replyqueue_new(void) { @@ -321,12 +413,22 @@ replyqueue_new(void) return rq; } +/** + * Return the "read socket" for a given reply queue. The main thread should + * listen for read events on this socket, and call replyqueue_process() every + * time it triggers. + */ tor_socket_t replyqueue_get_socket(replyqueue_t *rq) { return rq->alert.read_fd; } +/** + * Process all pending replies on a reply queue. The main thread should call + * this function every time the socket returned by replyqueue_get_socket() is + * readable. + */ void replyqueue_process(replyqueue_t *queue) { @@ -336,7 +438,7 @@ replyqueue_process(replyqueue_t *queue) tor_mutex_acquire(&queue->lock); while (!TOR_TAILQ_EMPTY(&queue->answers)) { - /* lock held. */ + /* lock must be held at this point.*/ workqueue_entry_t *work = TOR_TAILQ_FIRST(&queue->answers); TOR_TAILQ_REMOVE(&queue->answers, work, next_work); tor_mutex_release(&queue->lock); diff --git a/src/common/workqueue.h b/src/common/workqueue.h index 684fb192ba..dca947e915 100644 --- a/src/common/workqueue.h +++ b/src/common/workqueue.h @@ -6,15 +6,21 @@ #include "compat.h" +/** A replyqueue is used to tell the main thread about the outcome of + * work that we queued for the the workers. */ typedef struct replyqueue_s replyqueue_t; +/** A thread-pool manages starting threads and passing work to them. */ typedef struct threadpool_s threadpool_t; +/** A workqueue entry represents a request that has been passed to a thread + * pool. */ typedef struct workqueue_entry_s workqueue_entry_t; -#define WQ_CMD_RUN 0 -#define WQ_CMD_CANCEL 1 - +/** Possible return value from a work function: indicates success. */ #define WQ_RPL_REPLY 0 +/** Possible return value from a work function: indicates fatal error */ #define WQ_RPL_ERROR 1 +/** Possible return value from a work function: indicates thread is shutting + * down. */ #define WQ_RPL_SHUTDOWN 2 workqueue_entry_t *threadpool_queue_work(threadpool_t *pool, @@ -22,7 +28,7 @@ workqueue_entry_t *threadpool_queue_work(threadpool_t *pool, void (*reply_fn)(void *), void *arg); int threadpool_queue_for_all(threadpool_t *pool, - void *(*dup_fn)(void *), + void *(*dup_fn)(const void *), int (*fn)(void *, void *), void (*reply_fn)(void *), void *arg); From 93ad89e9d219d6cea764652a05c236210c7de3fa Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 25 Sep 2013 11:36:02 -0400 Subject: [PATCH 10/32] Rename bench_workqueue -> test_workqueue and make it a unit test. --- .gitignore | 2 + src/test/include.am | 19 ++-- .../{bench_workqueue.c => test_workqueue.c} | 90 +++++++++++++++---- 3 files changed, 85 insertions(+), 26 deletions(-) rename src/test/{bench_workqueue.c => test_workqueue.c} (71%) diff --git a/.gitignore b/.gitignore index 9ddd0c5385..e63576cfd4 100644 --- a/.gitignore +++ b/.gitignore @@ -163,10 +163,12 @@ cscope.* /src/test/test-bt-cl /src/test/test-child /src/test/test-ntor-cl +/src/test/test_workqueue /src/test/test.exe /src/test/test-bt-cl.exe /src/test/test-child.exe /src/test/test-ntor-cl.exe +/src/test/test_workqueue.exe # /src/tools/ /src/tools/tor-checkkey diff --git a/src/test/include.am b/src/test/include.am index 6ad1b552b7..2badc47a47 100644 --- a/src/test/include.am +++ b/src/test/include.am @@ -1,8 +1,8 @@ TESTS += src/test/test -noinst_PROGRAMS+= src/test/bench src/test/bench_workqueue +noinst_PROGRAMS+= src/test/bench if UNITTESTS_ENABLED -noinst_PROGRAMS+= src/test/test src/test/test-child +noinst_PROGRAMS+= src/test/test src/test/test-child src/test/test_workqueue endif src_test_AM_CPPFLAGS = -DSHARE_DATADIR="\"$(datadir)\"" \ @@ -62,8 +62,10 @@ src_test_test_CPPFLAGS= $(src_test_AM_CPPFLAGS) src_test_bench_SOURCES = \ src/test/bench.c -src_test_bench_workqueue_SOURCES = \ - src/test/bench_workqueue.c +src_test_test_workqueue_SOURCES = \ + src/test/test_workqueue.c +src_test_test_workqueue_CPPFLAGS= $(src_test_AM_CPPFLAGS) +src_test_test_workqueue_CFLAGS = $(AM_CFLAGS) $(TEST_CFLAGS) src_test_test_LDFLAGS = @TOR_LDFLAGS_zlib@ @TOR_LDFLAGS_openssl@ \ @TOR_LDFLAGS_libevent@ @@ -83,11 +85,12 @@ src_test_bench_LDADD = src/or/libtor.a src/common/libor.a \ @TOR_OPENSSL_LIBS@ @TOR_LIB_WS32@ @TOR_LIB_GDI@ @CURVE25519_LIBS@ \ @TOR_SYSTEMD_LIBS@ -src_test_bench_workqueue_LDFLAGS = @TOR_LDFLAGS_zlib@ @TOR_LDFLAGS_openssl@ \ +src_test_test_workqueue_LDFLAGS = @TOR_LDFLAGS_zlib@ @TOR_LDFLAGS_openssl@ \ @TOR_LDFLAGS_libevent@ -src_test_bench_workqueue_LDADD = src/or/libtor.a src/common/libor.a \ - src/common/libor-crypto.a $(LIBDONNA) \ - src/common/libor-event.a \ +src_test_test_workqueue_LDADD = src/or/libtor-testing.a \ + src/common/libor-testing.a \ + src/common/libor-crypto-testing.a $(LIBDONNA) \ + src/common/libor-event-testing.a \ @TOR_ZLIB_LIBS@ @TOR_LIB_MATH@ @TOR_LIBEVENT_LIBS@ \ @TOR_OPENSSL_LIBS@ @TOR_LIB_WS32@ @TOR_LIB_GDI@ @CURVE25519_LIBS@ diff --git a/src/test/bench_workqueue.c b/src/test/test_workqueue.c similarity index 71% rename from src/test/bench_workqueue.c rename to src/test/test_workqueue.c index f190c613e5..4077fb27a8 100644 --- a/src/test/bench_workqueue.c +++ b/src/test/test_workqueue.c @@ -18,16 +18,19 @@ #include #endif +static int opt_verbose = 0; +static int opt_n_threads = 8; +static int opt_n_items = 10000; +static int opt_n_inflight = 1000; +static int opt_n_lowwater = 250; +static int opt_ratio_rsa = 5; + #ifdef TRACK_RESPONSES tor_mutex_t bitmap_mutex; int handled_len; bitarray_t *handled; #endif -#define N_ITEMS 10000 -#define N_INFLIGHT 1000 -#define RELAUNCH_AT 250 - typedef struct state_s { int magic; int n_handled; @@ -174,7 +177,9 @@ handle_reply(void *arg) static int add_work(threadpool_t *tp) { - int add_rsa = tor_weak_random_range(&weak_rng, 5) == 0; + int add_rsa = + opt_ratio_rsa == 0 || + tor_weak_random_range(&weak_rng, opt_ratio_rsa) == 0; if (add_rsa) { rsa_work_t *w = tor_malloc_zero(sizeof(*w)); w->serial = n_sent++; @@ -206,10 +211,11 @@ replysock_readable_cb(tor_socket_t sock, short what, void *arg) if (old_r == n_received) return; - printf("%d / %d\n", n_received, n_sent); + if (opt_verbose) + printf("%d / %d\n", n_received, n_sent); #ifdef TRACK_RESPONSES tor_mutex_acquire(&bitmap_mutex); - for (i = 0; i < N_ITEMS; ++i) { + for (i = 0; i < opt_n_items; ++i) { if (bitarray_is_set(received, i)) putc('o', stdout); else if (bitarray_is_set(handled, i)) @@ -221,8 +227,8 @@ replysock_readable_cb(tor_socket_t sock, short what, void *arg) tor_mutex_release(&bitmap_mutex); #endif - if (n_sent - n_received < RELAUNCH_AT) { - while (n_sent < n_received + N_INFLIGHT && n_sent < N_ITEMS) { + if (n_sent - n_received < opt_n_lowwater) { + while (n_sent < n_received + opt_n_inflight && n_sent < opt_n_items) { if (! add_work(tp)) { puts("Couldn't add work."); tor_event_base_loopexit(tor_libevent_get_base(), NULL); @@ -230,11 +236,23 @@ replysock_readable_cb(tor_socket_t sock, short what, void *arg) } } - if (n_received == n_sent && n_sent >= N_ITEMS) { + if (n_received == n_sent && n_sent >= opt_n_items) { tor_event_base_loopexit(tor_libevent_get_base(), NULL); } } +static void +help(void) +{ + puts( + "Options:\n" + " -N Run this many items of work\n" + " -T Use this many threads\n" + " -I Have no more than this many requests queued at once\n" + " -L Add items whenever fewer than this many are pending.\n" + " -R Make one out of this many items be a slow (RSA) one"); +} + int main(int argc, char **argv) { @@ -244,8 +262,33 @@ main(int argc, char **argv) tor_libevent_cfg evcfg; struct event *ev; - (void)argc; - (void)argv; + for (i = 1; i < argc; ++i) { + if (!strcmp(argv[i], "-v")) { + opt_verbose = 1; + } else if (!strcmp(argv[i], "-T") && i+1 Date: Wed, 25 Sep 2013 11:41:40 -0400 Subject: [PATCH 11/32] Rename mutex_for_cond -> mutex_nonreentrant We'll want to use these for other stuff too. --- src/common/compat_pthreads.c | 6 +++--- src/common/compat_threads.c | 9 +++++++++ src/common/compat_threads.h | 6 +++++- src/common/compat_winthreads.c | 2 +- 4 files changed, 18 insertions(+), 5 deletions(-) diff --git a/src/common/compat_pthreads.c b/src/common/compat_pthreads.c index a2e406521f..8d3c60917a 100644 --- a/src/common/compat_pthreads.c +++ b/src/common/compat_pthreads.c @@ -97,10 +97,10 @@ tor_mutex_init(tor_mutex_t *mutex) } } -/** As tor_mutex_init, but initialize a mutex suitable for use with a - * condition variable. */ +/** As tor_mutex_init, but initialize a mutex suitable that may be + * non-reentrant, if the OS supports that. */ void -tor_mutex_init_for_cond(tor_mutex_t *mutex) +tor_mutex_init_nonreentrant(tor_mutex_t *mutex) { int err; if (PREDICT_UNLIKELY(!threads_initialized)) diff --git a/src/common/compat_threads.c b/src/common/compat_threads.c index 024c627cf1..f2a516a4a3 100644 --- a/src/common/compat_threads.c +++ b/src/common/compat_threads.c @@ -30,6 +30,15 @@ tor_mutex_new(void) tor_mutex_init(m); return m; } +/** Return a newly allocated, ready-for-use mutex. This one might be + * non-reentrant, if that's faster. */ +tor_mutex_t * +tor_mutex_new_nonreentrant(void) +{ + tor_mutex_t *m = tor_malloc_zero(sizeof(tor_mutex_t)); + tor_mutex_init_nonreentrant(m); + return m; +} /** Release all storage and system resources held by m. */ void tor_mutex_free(tor_mutex_t *m) diff --git a/src/common/compat_threads.h b/src/common/compat_threads.h index 9070f13e80..245df76178 100644 --- a/src/common/compat_threads.h +++ b/src/common/compat_threads.h @@ -46,8 +46,9 @@ typedef struct tor_mutex_t { tor_mutex_t *tor_mutex_new(void); +tor_mutex_t *tor_mutex_new_nonreentrant(void); void tor_mutex_init(tor_mutex_t *m); -void tor_mutex_init_for_cond(tor_mutex_t *m); +void tor_mutex_init_nonreentrant(tor_mutex_t *m); void tor_mutex_acquire(tor_mutex_t *m); void tor_mutex_release(tor_mutex_t *m); void tor_mutex_free(tor_mutex_t *m); @@ -55,6 +56,9 @@ void tor_mutex_uninit(tor_mutex_t *m); unsigned long tor_get_thread_id(void); void tor_threads_init(void); +/** Conditions need nonreentrant mutexes with pthreads. */ +#define tor_mutex_init_for_cond(m) tor_mutex_init_nonreentrant(m) + void set_main_thread(void); int in_main_thread(void); diff --git a/src/common/compat_winthreads.c b/src/common/compat_winthreads.c index 2b1527ad34..0ab26b7ebd 100644 --- a/src/common/compat_winthreads.c +++ b/src/common/compat_winthreads.c @@ -49,7 +49,7 @@ tor_mutex_init(tor_mutex_t *m) InitializeCriticalSection(&m->mutex); } void -tor_mutex_init_for_cond(tor_mutex_t *m) +tor_mutex_init_nonreentrant(tor_mutex_t *m) { InitializeCriticalSection(&m->mutex); } From c51f7c23e3af71466f9bd2ae57ae7a2b998ee3e2 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 25 Sep 2013 14:31:59 -0400 Subject: [PATCH 12/32] Test a little more of compat_threads.c --- src/common/compat_threads.c | 20 +++++++++++--------- src/common/compat_threads.h | 9 ++++++++- src/common/workqueue.c | 4 ++-- src/common/workqueue.h | 2 +- src/test/test_workqueue.c | 19 ++++++++++++++++--- 5 files changed, 38 insertions(+), 16 deletions(-) diff --git a/src/common/compat_threads.c b/src/common/compat_threads.c index f2a516a4a3..648eaa2d80 100644 --- a/src/common/compat_threads.c +++ b/src/common/compat_threads.c @@ -155,19 +155,18 @@ sock_drain(tor_socket_t fd) /** Allocate a new set of alert sockets, and set the appropriate function * pointers, in socks_out. */ int -alert_sockets_create(alert_sockets_t *socks_out) +alert_sockets_create(alert_sockets_t *socks_out, uint32_t flags) { - tor_socket_t socks[2]; + tor_socket_t socks[2] = { TOR_INVALID_SOCKET, TOR_INVALID_SOCKET }; #ifdef HAVE_EVENTFD /* First, we try the Linux eventfd() syscall. This gives a 64-bit counter * associated with a single file descriptor. */ #if defined(EFD_CLOEXEC) && defined(EFD_NONBLOCK) - socks[0] = eventfd(0, EFD_CLOEXEC|EFD_NONBLOCK); -#else - socks[0] = -1; + if (!(flags & ASOCKS_NOEVENTFD2)) + socks[0] = eventfd(0, EFD_CLOEXEC|EFD_NONBLOCK); #endif - if (socks[0] < 0) { + if (socks[0] < 0 && !(flags & ASOCKS_NOEVENTFD)) { socks[0] = eventfd(0,0); if (socks[0] >= 0) { if (fcntl(socks[0], F_SETFD, FD_CLOEXEC) < 0 || @@ -188,7 +187,8 @@ alert_sockets_create(alert_sockets_t *socks_out) #ifdef HAVE_PIPE2 /* Now we're going to try pipes. First type the pipe2() syscall, if we * have it, so we can save some calls... */ - if (pipe2(socks, O_NONBLOCK|O_CLOEXEC) == 0) { + if (!(flags & ASOCKS_NOPIPE2) && + pipe2(socks, O_NONBLOCK|O_CLOEXEC) == 0) { socks_out->read_fd = socks[0]; socks_out->write_fd = socks[1]; socks_out->alert_fn = pipe_alert; @@ -200,7 +200,8 @@ alert_sockets_create(alert_sockets_t *socks_out) #ifdef HAVE_PIPE /* Now try the regular pipe() syscall. Pipes have a bit lower overhead than * socketpairs, fwict. */ - if (pipe(socks) == 0) { + if (!(flags & ASOCKS_NOPIPE) && + pipe(socks) == 0) { if (fcntl(socks[0], F_SETFD, FD_CLOEXEC) < 0 || fcntl(socks[1], F_SETFD, FD_CLOEXEC) < 0 || set_socket_nonblocking(socks[0]) < 0 || @@ -218,7 +219,8 @@ alert_sockets_create(alert_sockets_t *socks_out) #endif /* If nothing else worked, fall back on socketpair(). */ - if (tor_socketpair(AF_UNIX, SOCK_STREAM, 0, socks) == 0) { + if (!(flags & ASOCKS_NOSOCKETPAIR) && + tor_socketpair(AF_UNIX, SOCK_STREAM, 0, socks) == 0) { if (set_socket_nonblocking(socks[0]) < 0 || set_socket_nonblocking(socks[1])) { tor_close_socket(socks[0]); diff --git a/src/common/compat_threads.h b/src/common/compat_threads.h index 245df76178..1b59391d3b 100644 --- a/src/common/compat_threads.h +++ b/src/common/compat_threads.h @@ -102,7 +102,14 @@ typedef struct alert_sockets_s { int (*drain_fn)(tor_socket_t read_fd); } alert_sockets_t; -int alert_sockets_create(alert_sockets_t *socks_out); +/* Flags to disable one or more alert_sockets backends. */ +#define ASOCKS_NOEVENTFD2 (1u<<0) +#define ASOCKS_NOEVENTFD (1u<<1) +#define ASOCKS_NOPIPE2 (1u<<2) +#define ASOCKS_NOPIPE (1u<<3) +#define ASOCKS_NOSOCKETPAIR (1u<<4) + +int alert_sockets_create(alert_sockets_t *socks_out, uint32_t flags); void alert_sockets_close(alert_sockets_t *socks); #endif diff --git a/src/common/workqueue.c b/src/common/workqueue.c index e07787b404..9293e1f9f0 100644 --- a/src/common/workqueue.c +++ b/src/common/workqueue.c @@ -397,12 +397,12 @@ threadpool_get_replyqueue(threadpool_t *tp) * IO-centric event loop, it needs to get woken up with means other than a * condition variable. */ replyqueue_t * -replyqueue_new(void) +replyqueue_new(uint32_t alertsocks_flags) { replyqueue_t *rq; rq = tor_malloc_zero(sizeof(replyqueue_t)); - if (alert_sockets_create(&rq->alert) < 0) { + if (alert_sockets_create(&rq->alert, alertsocks_flags) < 0) { tor_free(rq); return NULL; } diff --git a/src/common/workqueue.h b/src/common/workqueue.h index dca947e915..5a6cd80fb0 100644 --- a/src/common/workqueue.h +++ b/src/common/workqueue.h @@ -40,7 +40,7 @@ threadpool_t *threadpool_new(int n_threads, void *arg); replyqueue_t *threadpool_get_replyqueue(threadpool_t *tp); -replyqueue_t *replyqueue_new(void); +replyqueue_t *replyqueue_new(uint32_t alertsocks_flags); tor_socket_t replyqueue_get_socket(replyqueue_t *rq); void replyqueue_process(replyqueue_t *queue); diff --git a/src/test/test_workqueue.c b/src/test/test_workqueue.c index 4077fb27a8..7ef54ef22b 100644 --- a/src/test/test_workqueue.c +++ b/src/test/test_workqueue.c @@ -249,8 +249,10 @@ help(void) " -N Run this many items of work\n" " -T Use this many threads\n" " -I Have no more than this many requests queued at once\n" - " -L Add items whenever fewer than this many are pending.\n" - " -R Make one out of this many items be a slow (RSA) one"); + " -L Add items whenever fewer than this many are pending\n" + " -R Make one out of this many items be a slow (RSA) one\n" + " --no-{eventfd2,eventfd,pipe2,pipe,socketpair}\n" + " Disable one of the alert_socket backends."); } int @@ -261,6 +263,7 @@ main(int argc, char **argv) int i; tor_libevent_cfg evcfg; struct event *ev; + uint32_t as_flags = 0; for (i = 1; i < argc; ++i) { if (!strcmp(argv[i], "-v")) { @@ -275,6 +278,16 @@ main(int argc, char **argv) opt_n_lowwater = atoi(argv[++i]); } else if (!strcmp(argv[i], "-R") && i+1 Date: Wed, 25 Sep 2013 14:50:01 -0400 Subject: [PATCH 13/32] Move thread tests into their own module --- src/test/include.am | 1 + src/test/test.c | 2 + src/test/test_threads.c | 154 ++++++++++++++++++++++++++++++++++++++++ src/test/test_util.c | 137 ----------------------------------- 4 files changed, 157 insertions(+), 137 deletions(-) create mode 100644 src/test/test_threads.c diff --git a/src/test/include.am b/src/test/include.am index 2badc47a47..2e13454983 100644 --- a/src/test/include.am +++ b/src/test/include.am @@ -46,6 +46,7 @@ src_test_test_SOURCES = \ src/test/test_routerkeys.c \ src/test/test_scheduler.c \ src/test/test_socks.c \ + src/test/test_threads.c \ src/test/test_util.c \ src/test/test_config.c \ src/test/test_hs.c \ diff --git a/src/test/test.c b/src/test/test.c index de6efaf873..edc28cd2d4 100644 --- a/src/test/test.c +++ b/src/test/test.c @@ -1297,6 +1297,7 @@ extern struct testcase_t cell_queue_tests[]; extern struct testcase_t options_tests[]; extern struct testcase_t socks_tests[]; extern struct testcase_t entrynodes_tests[]; +extern struct testcase_t thread_tests[]; extern struct testcase_t extorport_tests[]; extern struct testcase_t controller_event_tests[]; extern struct testcase_t logging_tests[]; @@ -1323,6 +1324,7 @@ static struct testgroup_t testgroups[] = { { "container/", container_tests }, { "util/", util_tests }, { "util/logging/", logging_tests }, + { "util/thread/", thread_tests }, { "cellfmt/", cell_format_tests }, { "cellqueue/", cell_queue_tests }, { "dir/", dir_tests }, diff --git a/src/test/test_threads.c b/src/test/test_threads.c new file mode 100644 index 0000000000..2b4c93393f --- /dev/null +++ b/src/test/test_threads.c @@ -0,0 +1,154 @@ +/* Copyright (c) 2001-2004, Roger Dingledine. + * Copyright (c) 2004-2006, Roger Dingledine, Nick Mathewson. + * Copyright (c) 2007-2013, The Tor Project, Inc. */ +/* See LICENSE for licensing information */ + +#include "orconfig.h" +#include "or.h" +#include "compat_threads.h" +#include "test.h" + +/** mutex for thread test to stop the threads hitting data at the same time. */ +static tor_mutex_t *thread_test_mutex_ = NULL; +/** mutexes for the thread test to make sure that the threads have to + * interleave somewhat. */ +static tor_mutex_t *thread_test_start1_ = NULL, + *thread_test_start2_ = NULL; +/** Shared strmap for the thread test. */ +static strmap_t *thread_test_strmap_ = NULL; +/** The name of thread1 for the thread test */ +static char *thread1_name_ = NULL; +/** The name of thread2 for the thread test */ +static char *thread2_name_ = NULL; + +static void thread_test_func_(void* _s) ATTR_NORETURN; + +/** How many iterations have the threads in the unit test run? */ +static int t1_count = 0, t2_count = 0; + +/** Helper function for threading unit tests: This function runs in a + * subthread. It grabs its own mutex (start1 or start2) to make sure that it + * should start, then it repeatedly alters _test_thread_strmap protected by + * thread_test_mutex_. */ +static void +thread_test_func_(void* _s) +{ + char *s = _s; + int i, *count; + tor_mutex_t *m; + char buf[64]; + char **cp; + if (!strcmp(s, "thread 1")) { + m = thread_test_start1_; + cp = &thread1_name_; + count = &t1_count; + } else { + m = thread_test_start2_; + cp = &thread2_name_; + count = &t2_count; + } + + tor_snprintf(buf, sizeof(buf), "%lu", tor_get_thread_id()); + *cp = tor_strdup(buf); + + tor_mutex_acquire(m); + + for (i=0; i<10000; ++i) { + tor_mutex_acquire(thread_test_mutex_); + strmap_set(thread_test_strmap_, "last to run", *cp); + ++*count; + tor_mutex_release(thread_test_mutex_); + } + tor_mutex_acquire(thread_test_mutex_); + strmap_set(thread_test_strmap_, s, *cp); + tor_mutex_release(thread_test_mutex_); + + tor_mutex_release(m); + + spawn_exit(); +} + +/** Run unit tests for threading logic. */ +static void +test_threads_basic(void *arg) +{ + char *s1 = NULL, *s2 = NULL; + int done = 0, timedout = 0; + time_t started; +#ifndef _WIN32 + struct timeval tv; + tv.tv_sec=0; + tv.tv_usec=100*1000; +#endif + (void)arg; + thread_test_mutex_ = tor_mutex_new(); + thread_test_start1_ = tor_mutex_new(); + thread_test_start2_ = tor_mutex_new(); + thread_test_strmap_ = strmap_new(); + s1 = tor_strdup("thread 1"); + s2 = tor_strdup("thread 2"); + tor_mutex_acquire(thread_test_start1_); + tor_mutex_acquire(thread_test_start2_); + spawn_func(thread_test_func_, s1); + spawn_func(thread_test_func_, s2); + tor_mutex_release(thread_test_start2_); + tor_mutex_release(thread_test_start1_); + started = time(NULL); + while (!done) { + tor_mutex_acquire(thread_test_mutex_); + strmap_assert_ok(thread_test_strmap_); + if (strmap_get(thread_test_strmap_, "thread 1") && + strmap_get(thread_test_strmap_, "thread 2")) { + done = 1; + } else if (time(NULL) > started + 150) { + timedout = done = 1; + } + tor_mutex_release(thread_test_mutex_); +#ifndef _WIN32 + /* Prevent the main thread from starving the worker threads. */ + select(0, NULL, NULL, NULL, &tv); +#endif + } + tor_mutex_acquire(thread_test_start1_); + tor_mutex_release(thread_test_start1_); + tor_mutex_acquire(thread_test_start2_); + tor_mutex_release(thread_test_start2_); + + tor_mutex_free(thread_test_mutex_); + + if (timedout) { + printf("\nTimed out: %d %d", t1_count, t2_count); + tt_assert(strmap_get(thread_test_strmap_, "thread 1")); + tt_assert(strmap_get(thread_test_strmap_, "thread 2")); + tt_assert(!timedout); + } + + /* different thread IDs. */ + tt_assert(strcmp(strmap_get(thread_test_strmap_, "thread 1"), + strmap_get(thread_test_strmap_, "thread 2"))); + tt_assert(!strcmp(strmap_get(thread_test_strmap_, "thread 1"), + strmap_get(thread_test_strmap_, "last to run")) || + !strcmp(strmap_get(thread_test_strmap_, "thread 2"), + strmap_get(thread_test_strmap_, "last to run"))); + + done: + tor_free(s1); + tor_free(s2); + tor_free(thread1_name_); + tor_free(thread2_name_); + if (thread_test_strmap_) + strmap_free(thread_test_strmap_, NULL); + if (thread_test_start1_) + tor_mutex_free(thread_test_start1_); + if (thread_test_start2_) + tor_mutex_free(thread_test_start2_); +} + +#define THREAD_TEST(name) \ + { #name, test_threads_##name, TT_FORK, NULL, NULL } + +struct testcase_t thread_tests[] = { + THREAD_TEST(basic), + END_OF_TESTCASES +}; + diff --git a/src/test/test_util.c b/src/test/test_util.c index 15470e8efa..b4ee934698 100644 --- a/src/test/test_util.c +++ b/src/test/test_util.c @@ -1607,142 +1607,6 @@ test_util_pow2(void *arg) ; } -/** mutex for thread test to stop the threads hitting data at the same time. */ -static tor_mutex_t *thread_test_mutex_ = NULL; -/** mutexes for the thread test to make sure that the threads have to - * interleave somewhat. */ -static tor_mutex_t *thread_test_start1_ = NULL, - *thread_test_start2_ = NULL; -/** Shared strmap for the thread test. */ -static strmap_t *thread_test_strmap_ = NULL; -/** The name of thread1 for the thread test */ -static char *thread1_name_ = NULL; -/** The name of thread2 for the thread test */ -static char *thread2_name_ = NULL; - -static void thread_test_func_(void* _s) ATTR_NORETURN; - -/** How many iterations have the threads in the unit test run? */ -static int t1_count = 0, t2_count = 0; - -/** Helper function for threading unit tests: This function runs in a - * subthread. It grabs its own mutex (start1 or start2) to make sure that it - * should start, then it repeatedly alters _test_thread_strmap protected by - * thread_test_mutex_. */ -static void -thread_test_func_(void* _s) -{ - char *s = _s; - int i, *count; - tor_mutex_t *m; - char buf[64]; - char **cp; - if (!strcmp(s, "thread 1")) { - m = thread_test_start1_; - cp = &thread1_name_; - count = &t1_count; - } else { - m = thread_test_start2_; - cp = &thread2_name_; - count = &t2_count; - } - - tor_snprintf(buf, sizeof(buf), "%lu", tor_get_thread_id()); - *cp = tor_strdup(buf); - - tor_mutex_acquire(m); - - for (i=0; i<10000; ++i) { - tor_mutex_acquire(thread_test_mutex_); - strmap_set(thread_test_strmap_, "last to run", *cp); - ++*count; - tor_mutex_release(thread_test_mutex_); - } - tor_mutex_acquire(thread_test_mutex_); - strmap_set(thread_test_strmap_, s, *cp); - tor_mutex_release(thread_test_mutex_); - - tor_mutex_release(m); - - spawn_exit(); -} - -/** Run unit tests for threading logic. */ -static void -test_util_threads(void *arg) -{ - char *s1 = NULL, *s2 = NULL; - int done = 0, timedout = 0; - time_t started; -#ifndef _WIN32 - struct timeval tv; - tv.tv_sec=0; - tv.tv_usec=100*1000; -#endif - (void)arg; - thread_test_mutex_ = tor_mutex_new(); - thread_test_start1_ = tor_mutex_new(); - thread_test_start2_ = tor_mutex_new(); - thread_test_strmap_ = strmap_new(); - s1 = tor_strdup("thread 1"); - s2 = tor_strdup("thread 2"); - tor_mutex_acquire(thread_test_start1_); - tor_mutex_acquire(thread_test_start2_); - spawn_func(thread_test_func_, s1); - spawn_func(thread_test_func_, s2); - tor_mutex_release(thread_test_start2_); - tor_mutex_release(thread_test_start1_); - started = time(NULL); - while (!done) { - tor_mutex_acquire(thread_test_mutex_); - strmap_assert_ok(thread_test_strmap_); - if (strmap_get(thread_test_strmap_, "thread 1") && - strmap_get(thread_test_strmap_, "thread 2")) { - done = 1; - } else if (time(NULL) > started + 150) { - timedout = done = 1; - } - tor_mutex_release(thread_test_mutex_); -#ifndef _WIN32 - /* Prevent the main thread from starving the worker threads. */ - select(0, NULL, NULL, NULL, &tv); -#endif - } - tor_mutex_acquire(thread_test_start1_); - tor_mutex_release(thread_test_start1_); - tor_mutex_acquire(thread_test_start2_); - tor_mutex_release(thread_test_start2_); - - tor_mutex_free(thread_test_mutex_); - - if (timedout) { - printf("\nTimed out: %d %d", t1_count, t2_count); - tt_assert(strmap_get(thread_test_strmap_, "thread 1")); - tt_assert(strmap_get(thread_test_strmap_, "thread 2")); - tt_assert(!timedout); - } - - /* different thread IDs. */ - tt_assert(strcmp(strmap_get(thread_test_strmap_, "thread 1"), - strmap_get(thread_test_strmap_, "thread 2"))); - tt_assert(!strcmp(strmap_get(thread_test_strmap_, "thread 1"), - strmap_get(thread_test_strmap_, "last to run")) || - !strcmp(strmap_get(thread_test_strmap_, "thread 2"), - strmap_get(thread_test_strmap_, "last to run"))); - - done: - tor_free(s1); - tor_free(s2); - tor_free(thread1_name_); - tor_free(thread2_name_); - if (thread_test_strmap_) - strmap_free(thread_test_strmap_, NULL); - if (thread_test_start1_) - tor_mutex_free(thread_test_start1_); - if (thread_test_start2_) - tor_mutex_free(thread_test_start2_); -} - /** Run unit tests for compression functions */ static void test_util_gzip(void *arg) @@ -4927,7 +4791,6 @@ struct testcase_t util_tests[] = { UTIL_LEGACY(memarea), UTIL_LEGACY(control_formats), UTIL_LEGACY(mmap), - UTIL_LEGACY(threads), UTIL_LEGACY(sscanf), UTIL_LEGACY(format_time_interval), UTIL_LEGACY(path_is_relative), From d850ec8574761c0279df53f3b7a9811d1dab430f Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 25 Sep 2013 23:25:02 -0400 Subject: [PATCH 14/32] Fix linux compilation (pipe2 needs _GNU_SOURCE) --- src/common/compat_threads.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/common/compat_threads.c b/src/common/compat_threads.c index 648eaa2d80..ba48eb4d1f 100644 --- a/src/common/compat_threads.c +++ b/src/common/compat_threads.c @@ -3,6 +3,8 @@ * Copyright (c) 2007-2015, The Tor Project, Inc. */ /* See LICENSE for licensing information */ +#define _GNU_SOURCE + #include "orconfig.h" #define _GNU_SOURCE #include @@ -15,12 +17,12 @@ #ifdef HAVE_SYS_EVENTFD_H #include #endif -#ifdef HAVE_UNISTD_H -#include -#endif #ifdef HAVE_FCNTL_H #include #endif +#ifdef HAVE_UNISTD_H +#include +#endif /** Return a newly allocated, ready-for-use mutex. */ tor_mutex_t * From 9fdc0d059456146722dc81f2b58672b533a2bb71 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 25 Sep 2013 23:31:40 -0400 Subject: [PATCH 15/32] Fix windows compilation of condition code --- src/common/compat.c | 1 - src/common/compat_winthreads.c | 11 +++++++---- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/src/common/compat.c b/src/common/compat.c index a22a61ac4d..5575316b2b 100644 --- a/src/common/compat.c +++ b/src/common/compat.c @@ -27,7 +27,6 @@ #include "compat.h" #ifdef _WIN32 -#include #include #include #endif diff --git a/src/common/compat_winthreads.c b/src/common/compat_winthreads.c index 0ab26b7ebd..e19b1cae85 100644 --- a/src/common/compat_winthreads.c +++ b/src/common/compat_winthreads.c @@ -9,6 +9,10 @@ #include "util.h" #include "container.h" #include "torlog.h" +#include + +/* This value is more or less total cargo-cult */ +#define SPIN_COUNT 2000 /** Minimalist interface to run a void function in the background. On * Unix calls fork, on win32 calls beginthread. Returns -1 on failure. @@ -108,7 +112,6 @@ tor_cond_signal_impl(tor_cond_t *cond, int broadcast) cond->generation++; SetEvent(cond->event); LeaveCriticalSection(&cond->lock); - return 0; } void tor_cond_signal_one(tor_cond_t *cond) @@ -122,15 +125,15 @@ tor_cond_signal_all(tor_cond_t *cond) } int -tor_cond_wait(tor_cond_t *cond, tor_mutex_t *lock, const struct timeval *tv) +tor_cond_wait(tor_cond_t *cond, tor_mutex_t *lock_, const struct timeval *tv) { - CRITICAL_SECTION *lock = &lock->mutex; + CRITICAL_SECTION *lock = &lock_->mutex; int generation_at_start; int waiting = 1; int result = -1; DWORD ms = INFINITE, ms_orig = INFINITE, startTime, endTime; if (tv) - ms_orig = ms = evutil_tv_to_msec_(tv); + ms_orig = ms = tv->tv_sec*1000 + (tv->tv_usec+999)/1000; EnterCriticalSection(&cond->lock); ++cond->n_waiting; From d69717f61bd9ab4e0a6097f0201bd02fc96f88eb Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Fri, 27 Sep 2013 12:09:57 -0400 Subject: [PATCH 16/32] Use correct (absolute) time for pthread_cond_timedwait --- src/common/compat_pthreads.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/common/compat_pthreads.c b/src/common/compat_pthreads.c index 8d3c60917a..59834270a3 100644 --- a/src/common/compat_pthreads.c +++ b/src/common/compat_pthreads.c @@ -199,9 +199,12 @@ tor_cond_wait(tor_cond_t *cond, tor_mutex_t *mutex, const struct timeval *tv) return pthread_cond_wait(&cond->cond, &mutex->mutex) ? -1 : 0; } else { struct timespec ts; + struct timeval tvnow, tvsum; int r; - ts.tv_sec = tv->tv_sec; - ts.tv_nsec = tv->tv_usec * 1000; + gettimeofday(&tvnow, NULL); + timeradd(tv, &tvnow, &tvsum); + ts.tv_sec = tvsum.tv_sec; + ts.tv_nsec = tvsum.tv_usec * 1000; r = pthread_cond_timedwait(&cond->cond, &mutex->mutex, &ts); if (r == 0) return 0; From e47a90a976f883571bea6e58620aa13f058873e3 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Fri, 27 Sep 2013 12:32:19 -0400 Subject: [PATCH 17/32] "Recursive" locks, not "reentrant" locks. Duh. --- src/common/compat_pthreads.c | 16 ++++++++-------- src/common/compat_threads.c | 6 +++--- src/common/compat_threads.h | 8 ++++---- src/common/compat_winthreads.c | 2 +- 4 files changed, 16 insertions(+), 16 deletions(-) diff --git a/src/common/compat_pthreads.c b/src/common/compat_pthreads.c index 59834270a3..69f7bac9c9 100644 --- a/src/common/compat_pthreads.c +++ b/src/common/compat_pthreads.c @@ -45,7 +45,6 @@ static pthread_attr_t attr_detached; /** True iff we've called tor_threads_init() */ static int threads_initialized = 0; - /** Minimalist interface to run a void function in the background. On * Unix calls fork, on win32 calls beginthread. Returns -1 on failure. * func should not return, but rather should call spawn_exit. @@ -79,9 +78,10 @@ spawn_exit(void) } /** A mutex attribute that we're going to use to tell pthreads that we want - * "reentrant" mutexes (i.e., once we can re-lock if we're already holding + * "recursive" mutexes (i.e., once we can re-lock if we're already holding * them.) */ -static pthread_mutexattr_t attr_reentrant; +static pthread_mutexattr_t attr_recursive; + /** Initialize mutex so it can be locked. Every mutex must be set * up with tor_mutex_init() or tor_mutex_new(); not both. */ void @@ -90,7 +90,7 @@ tor_mutex_init(tor_mutex_t *mutex) int err; if (PREDICT_UNLIKELY(!threads_initialized)) tor_threads_init(); - err = pthread_mutex_init(&mutex->mutex, &attr_reentrant); + err = pthread_mutex_init(&mutex->mutex, &attr_recursive); if (PREDICT_UNLIKELY(err)) { log_err(LD_GENERAL, "Error %d creating a mutex.", err); tor_fragile_assert(); @@ -98,9 +98,9 @@ tor_mutex_init(tor_mutex_t *mutex) } /** As tor_mutex_init, but initialize a mutex suitable that may be - * non-reentrant, if the OS supports that. */ + * non-recursive, if the OS supports that. */ void -tor_mutex_init_nonreentrant(tor_mutex_t *mutex) +tor_mutex_init_nonrecursive(tor_mutex_t *mutex) { int err; if (PREDICT_UNLIKELY(!threads_initialized)) @@ -232,8 +232,8 @@ void tor_threads_init(void) { if (!threads_initialized) { - pthread_mutexattr_init(&attr_reentrant); - pthread_mutexattr_settype(&attr_reentrant, PTHREAD_MUTEX_RECURSIVE); + pthread_mutexattr_init(&attr_recursive); + pthread_mutexattr_settype(&attr_recursive, PTHREAD_MUTEX_RECURSIVE); tor_assert(0==pthread_attr_init(&attr_detached)); tor_assert(0==pthread_attr_setdetachstate(&attr_detached, 1)); threads_initialized = 1; diff --git a/src/common/compat_threads.c b/src/common/compat_threads.c index ba48eb4d1f..f018475e18 100644 --- a/src/common/compat_threads.c +++ b/src/common/compat_threads.c @@ -33,12 +33,12 @@ tor_mutex_new(void) return m; } /** Return a newly allocated, ready-for-use mutex. This one might be - * non-reentrant, if that's faster. */ + * non-recursive, if that's faster. */ tor_mutex_t * -tor_mutex_new_nonreentrant(void) +tor_mutex_new_nonrecursive(void) { tor_mutex_t *m = tor_malloc_zero(sizeof(tor_mutex_t)); - tor_mutex_init_nonreentrant(m); + tor_mutex_init_nonrecursive(m); return m; } /** Release all storage and system resources held by m. */ diff --git a/src/common/compat_threads.h b/src/common/compat_threads.h index 1b59391d3b..a5db72d45f 100644 --- a/src/common/compat_threads.h +++ b/src/common/compat_threads.h @@ -46,9 +46,9 @@ typedef struct tor_mutex_t { tor_mutex_t *tor_mutex_new(void); -tor_mutex_t *tor_mutex_new_nonreentrant(void); +tor_mutex_t *tor_mutex_new_nonrecursive(void); void tor_mutex_init(tor_mutex_t *m); -void tor_mutex_init_nonreentrant(tor_mutex_t *m); +void tor_mutex_init_nonrecursive(tor_mutex_t *m); void tor_mutex_acquire(tor_mutex_t *m); void tor_mutex_release(tor_mutex_t *m); void tor_mutex_free(tor_mutex_t *m); @@ -56,8 +56,8 @@ void tor_mutex_uninit(tor_mutex_t *m); unsigned long tor_get_thread_id(void); void tor_threads_init(void); -/** Conditions need nonreentrant mutexes with pthreads. */ -#define tor_mutex_init_for_cond(m) tor_mutex_init_nonreentrant(m) +/** Conditions need nonrecursive mutexes with pthreads. */ +#define tor_mutex_init_for_cond(m) tor_mutex_init_nonrecursive(m) void set_main_thread(void); int in_main_thread(void); diff --git a/src/common/compat_winthreads.c b/src/common/compat_winthreads.c index e19b1cae85..4820eb3481 100644 --- a/src/common/compat_winthreads.c +++ b/src/common/compat_winthreads.c @@ -53,7 +53,7 @@ tor_mutex_init(tor_mutex_t *m) InitializeCriticalSection(&m->mutex); } void -tor_mutex_init_nonreentrant(tor_mutex_t *m) +tor_mutex_init_nonrecursive(tor_mutex_t *m) { InitializeCriticalSection(&m->mutex); } From 7a63005220938b30df41b51334942d7d79c14cf9 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Fri, 27 Sep 2013 23:15:53 -0400 Subject: [PATCH 18/32] Basic unit test for condition variables. --- configure.ac | 2 +- src/test/test.c | 18 +++++ src/test/test.h | 2 + src/test/test_crypto.c | 36 +++------- src/test/test_threads.c | 150 +++++++++++++++++++++++++++++++++++++++- src/test/test_util.c | 21 +----- 6 files changed, 183 insertions(+), 46 deletions(-) diff --git a/configure.ac b/configure.ac index 69a266a717..929b701594 100644 --- a/configure.ac +++ b/configure.ac @@ -437,7 +437,7 @@ AC_CHECK_FUNCS( sysconf \ sysctl \ uname \ - usleep \ + usleep \ vasprintf \ _vscprintf ) diff --git a/src/test/test.c b/src/test/test.c index edc28cd2d4..9171306d18 100644 --- a/src/test/test.c +++ b/src/test/test.c @@ -1258,6 +1258,24 @@ test_stats(void *arg) tor_free(s); } + +static void * +passthrough_test_setup(const struct testcase_t *testcase) +{ + return testcase->setup_data; +} +static int +passthrough_test_cleanup(const struct testcase_t *testcase, void *ptr) +{ + (void)testcase; + (void)ptr; + return 1; +} + +const struct testcase_setup_t passthrough_setup = { + passthrough_test_setup, passthrough_test_cleanup +}; + #define ENT(name) \ { #name, test_ ## name , 0, NULL, NULL } #define FORK(name) \ diff --git a/src/test/test.h b/src/test/test.h index 48037a5ba3..b8057c59bf 100644 --- a/src/test/test.h +++ b/src/test/test.h @@ -158,5 +158,7 @@ crypto_pk_t *pk_generate(int idx); #define NS_MOCK(name) MOCK(name, NS(name)) #define NS_UNMOCK(name) UNMOCK(name) +extern const struct testcase_setup_t passthrough_setup; + #endif diff --git a/src/test/test_crypto.c b/src/test/test_crypto.c index 4a5a12c50a..8426c715a4 100644 --- a/src/test/test_crypto.c +++ b/src/test/test_crypto.c @@ -1975,30 +1975,14 @@ test_crypto_siphash(void *arg) ; } -static void * -pass_data_setup_fn(const struct testcase_t *testcase) -{ - return testcase->setup_data; -} -static int -pass_data_cleanup_fn(const struct testcase_t *testcase, void *ptr) -{ - (void)ptr; - (void)testcase; - return 1; -} -static const struct testcase_setup_t pass_data = { - pass_data_setup_fn, pass_data_cleanup_fn -}; - #define CRYPTO_LEGACY(name) \ { #name, test_crypto_ ## name , 0, NULL, NULL } struct testcase_t crypto_tests[] = { CRYPTO_LEGACY(formats), CRYPTO_LEGACY(rng), - { "aes_AES", test_crypto_aes, TT_FORK, &pass_data, (void*)"aes" }, - { "aes_EVP", test_crypto_aes, TT_FORK, &pass_data, (void*)"evp" }, + { "aes_AES", test_crypto_aes, TT_FORK, &passthrough_setup, (void*)"aes" }, + { "aes_EVP", test_crypto_aes, TT_FORK, &passthrough_setup, (void*)"evp" }, CRYPTO_LEGACY(sha), CRYPTO_LEGACY(pk), { "pk_fingerprints", test_crypto_pk_fingerprints, TT_FORK, NULL, NULL }, @@ -2006,23 +1990,25 @@ struct testcase_t crypto_tests[] = { CRYPTO_LEGACY(dh), CRYPTO_LEGACY(s2k_rfc2440), #ifdef HAVE_LIBSCRYPT_H - { "s2k_scrypt", test_crypto_s2k_general, 0, &pass_data, + { "s2k_scrypt", test_crypto_s2k_general, 0, &passthrough_setup, (void*)"scrypt" }, - { "s2k_scrypt_low", test_crypto_s2k_general, 0, &pass_data, + { "s2k_scrypt_low", test_crypto_s2k_general, 0, &passthrough_setup, (void*)"scrypt-low" }, #endif - { "s2k_pbkdf2", test_crypto_s2k_general, 0, &pass_data, + { "s2k_pbkdf2", test_crypto_s2k_general, 0, &passthrough_setup, (void*)"pbkdf2" }, - { "s2k_rfc2440_general", test_crypto_s2k_general, 0, &pass_data, + { "s2k_rfc2440_general", test_crypto_s2k_general, 0, &passthrough_setup, (void*)"rfc2440" }, - { "s2k_rfc2440_legacy", test_crypto_s2k_general, 0, &pass_data, + { "s2k_rfc2440_legacy", test_crypto_s2k_general, 0, &passthrough_setup, (void*)"rfc2440-legacy" }, { "s2k_errors", test_crypto_s2k_errors, 0, NULL, NULL }, { "scrypt_vectors", test_crypto_scrypt_vectors, 0, NULL, NULL }, { "pbkdf2_vectors", test_crypto_pbkdf2_vectors, 0, NULL, NULL }, { "pwbox", test_crypto_pwbox, 0, NULL, NULL }, - { "aes_iv_AES", test_crypto_aes_iv, TT_FORK, &pass_data, (void*)"aes" }, - { "aes_iv_EVP", test_crypto_aes_iv, TT_FORK, &pass_data, (void*)"evp" }, + { "aes_iv_AES", test_crypto_aes_iv, TT_FORK, &passthrough_setup, + (void*)"aes" }, + { "aes_iv_EVP", test_crypto_aes_iv, TT_FORK, &passthrough_setup, + (void*)"evp" }, CRYPTO_LEGACY(base32_decode), { "kdf_TAP", test_crypto_kdf_TAP, 0, NULL, NULL }, { "hkdf_sha256", test_crypto_hkdf_sha256, 0, NULL, NULL }, diff --git a/src/test/test_threads.c b/src/test/test_threads.c index 2b4c93393f..d2a61a17d0 100644 --- a/src/test/test_threads.c +++ b/src/test/test_threads.c @@ -144,11 +144,159 @@ test_threads_basic(void *arg) tor_mutex_free(thread_test_start2_); } -#define THREAD_TEST(name) \ +typedef struct cv_testinfo_s { + tor_cond_t *cond; + tor_mutex_t *mutex; + int value; + int addend; + int shutdown; + int n_shutdown; + int n_wakeups; + int n_timeouts; + int n_threads; + const struct timeval *tv; +} cv_testinfo_t; + +static cv_testinfo_t * +cv_testinfo_new(void) +{ + cv_testinfo_t *i = tor_malloc_zero(sizeof(*i)); + i->cond = tor_cond_new(); + i->mutex = tor_mutex_new_nonrecursive(); + return i; +} + +static void +cv_testinfo_free(cv_testinfo_t *i) +{ + if (!i) + return; + tor_cond_free(i->cond); + tor_mutex_free(i->mutex); + tor_free(i); +} + +static void cv_test_thr_fn_(void *arg) ATTR_NORETURN; + +static void +cv_test_thr_fn_(void *arg) +{ + cv_testinfo_t *i = arg; + int tid, r; + + tor_mutex_acquire(i->mutex); + tid = i->n_threads++; + tor_mutex_release(i->mutex); + (void) tid; + + tor_mutex_acquire(i->mutex); + while (1) { + if (i->addend) { + i->value += i->addend; + i->addend = 0; + } + + if (i->shutdown) { + ++i->n_shutdown; + i->shutdown = 0; + tor_mutex_release(i->mutex); + spawn_exit(); + } + r = tor_cond_wait(i->cond, i->mutex, i->tv); + ++i->n_wakeups; + if (r == 1) { + ++i->n_timeouts; + tor_mutex_release(i->mutex); + spawn_exit(); + } + } +} + +static void +test_threads_conditionvar(void *arg) +{ + cv_testinfo_t *ti=NULL; + const struct timeval msec100 = { 0, 100*1000 }; + const int timeout = !strcmp(arg, "tv"); + + ti = cv_testinfo_new(); + if (timeout) { + ti->tv = &msec100; + } + spawn_func(cv_test_thr_fn_, ti); + spawn_func(cv_test_thr_fn_, ti); + spawn_func(cv_test_thr_fn_, ti); + spawn_func(cv_test_thr_fn_, ti); + + tor_mutex_acquire(ti->mutex); + ti->addend = 7; + ti->shutdown = 1; + tor_cond_signal_one(ti->cond); + tor_mutex_release(ti->mutex); + +#define SPIN() \ + while (1) { \ + tor_mutex_acquire(ti->mutex); \ + if (ti->addend == 0) { \ + break; \ + } \ + tor_mutex_release(ti->mutex); \ + } + + SPIN(); + + ti->addend = 30; + ti->shutdown = 1; + tor_cond_signal_all(ti->cond); + tor_mutex_release(ti->mutex); + SPIN(); + + ti->addend = 1000; + if (! timeout) ti->shutdown = 1; + tor_cond_signal_one(ti->cond); + tor_mutex_release(ti->mutex); + SPIN(); + ti->addend = 300; + if (! timeout) ti->shutdown = 1; + tor_cond_signal_all(ti->cond); + tor_mutex_release(ti->mutex); + + SPIN(); + tor_mutex_release(ti->mutex); + + tt_int_op(ti->value, ==, 1337); + if (!timeout) { + tt_int_op(ti->n_shutdown, ==, 4); + } else { +#ifdef _WIN32 + Sleep(500); /* msec */ +#elif defined(HAVE_USLEEP) + usleep(500*1000); /* usec */ +#else + { + struct tv = { 0, 500*1000 }; + select(0, NULL, NULL, NULL, &tv); + } +#endif + tor_mutex_acquire(ti->mutex); + tt_int_op(ti->n_shutdown, ==, 2); + tt_int_op(ti->n_timeouts, ==, 2); + tor_mutex_release(ti->mutex); + } + + done: + cv_testinfo_free(ti); +} + +#define THREAD_TEST(name) \ { #name, test_threads_##name, TT_FORK, NULL, NULL } struct testcase_t thread_tests[] = { THREAD_TEST(basic), + { "conditionvar", test_threads_conditionvar, TT_FORK, + &passthrough_setup, (void*)"no-tv" }, + { "conditionvar_timeout", test_threads_conditionvar, TT_FORK, + &passthrough_setup, (void*)"tv" }, END_OF_TESTCASES }; diff --git a/src/test/test_util.c b/src/test/test_util.c index b4ee934698..97cf3870f4 100644 --- a/src/test/test_util.c +++ b/src/test/test_util.c @@ -4646,23 +4646,6 @@ test_util_socket(void *arg) tor_close_socket(fd4); } -static void * -socketpair_test_setup(const struct testcase_t *testcase) -{ - return testcase->setup_data; -} -static int -socketpair_test_cleanup(const struct testcase_t *testcase, void *ptr) -{ - (void)testcase; - (void)ptr; - return 1; -} - -static const struct testcase_setup_t socketpair_setup = { - socketpair_test_setup, socketpair_test_cleanup -}; - /* Test for socketpair and ersatz_socketpair(). We test them both, since * the latter is a tolerably good way to exersize tor_accept_socket(). */ static void @@ -4837,10 +4820,10 @@ struct testcase_t util_tests[] = { UTIL_TEST(mathlog, 0), UTIL_TEST(weak_random, 0), UTIL_TEST(socket, TT_FORK), - { "socketpair", test_util_socketpair, TT_FORK, &socketpair_setup, + { "socketpair", test_util_socketpair, TT_FORK, &passthrough_setup, (void*)"0" }, { "socketpair_ersatz", test_util_socketpair, TT_FORK, - &socketpair_setup, (void*)"1" }, + &passthrough_setup, (void*)"1" }, UTIL_TEST(max_mem, 0), UTIL_TEST(hostname_validation, 0), UTIL_TEST(ipv4_validation, 0), From 81354b081b7bb9deabd6c53e48623190b01aab1c Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Fri, 27 Sep 2013 23:20:22 -0400 Subject: [PATCH 19/32] Add unit test for thread IDs. --- src/test/test_threads.c | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-) diff --git a/src/test/test_threads.c b/src/test/test_threads.c index d2a61a17d0..2bc24e1edc 100644 --- a/src/test/test_threads.c +++ b/src/test/test_threads.c @@ -21,6 +21,10 @@ static char *thread1_name_ = NULL; /** The name of thread2 for the thread test */ static char *thread2_name_ = NULL; +static int thread_fns_failed = 0; + +static unsigned long thread_fn_tid1, thread_fn_tid2; + static void thread_test_func_(void* _s) ATTR_NORETURN; /** How many iterations have the threads in the unit test run? */ @@ -42,10 +46,12 @@ thread_test_func_(void* _s) m = thread_test_start1_; cp = &thread1_name_; count = &t1_count; + thread_fn_tid1 = tor_get_thread_id(); } else { m = thread_test_start2_; cp = &thread2_name_; count = &t2_count; + thread_fn_tid2 = tor_get_thread_id(); } tor_snprintf(buf, sizeof(buf), "%lu", tor_get_thread_id()); @@ -61,6 +67,8 @@ thread_test_func_(void* _s) } tor_mutex_acquire(thread_test_mutex_); strmap_set(thread_test_strmap_, s, *cp); + if (in_main_thread()) + ++thread_fns_failed; tor_mutex_release(thread_test_mutex_); tor_mutex_release(m); @@ -80,7 +88,10 @@ test_threads_basic(void *arg) tv.tv_sec=0; tv.tv_usec=100*1000; #endif - (void)arg; + (void) arg; + + set_main_thread(); + thread_test_mutex_ = tor_mutex_new(); thread_test_start1_ = tor_mutex_new(); thread_test_start2_ = tor_mutex_new(); @@ -131,6 +142,9 @@ test_threads_basic(void *arg) !strcmp(strmap_get(thread_test_strmap_, "thread 2"), strmap_get(thread_test_strmap_, "last to run"))); + tt_int_op(thread_fns_failed, ==, 0); + tt_int_op(thread_fn_tid1, !=, thread_fn_tid2); + done: tor_free(s1); tor_free(s2); @@ -188,7 +202,7 @@ cv_test_thr_fn_(void *arg) tid = i->n_threads++; tor_mutex_release(i->mutex); (void) tid; - + tor_mutex_acquire(i->mutex); while (1) { if (i->addend) { @@ -299,4 +313,3 @@ struct testcase_t thread_tests[] = { &passthrough_setup, (void*)"tv" }, END_OF_TESTCASES }; - From ebbc177005eaf9bd949daba657b2c703a7bd1769 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Sat, 28 Sep 2013 00:09:20 -0400 Subject: [PATCH 20/32] Add shutdown and broadcast support to test_workqueue. --- src/test/test_workqueue.c | 26 ++++++++++++++++++++------ 1 file changed, 20 insertions(+), 6 deletions(-) diff --git a/src/test/test_workqueue.c b/src/test/test_workqueue.c index 7ef54ef22b..cbf9d81950 100644 --- a/src/test/test_workqueue.c +++ b/src/test/test_workqueue.c @@ -36,6 +36,7 @@ typedef struct state_s { int n_handled; crypto_pk_t *rsa; curve25519_secret_key_t ecdh; + int is_shutdown; } state_t; typedef struct rsa_work_s { @@ -94,18 +95,15 @@ workqueue_do_rsa(void *state, void *work) return WQ_RPL_REPLY; } -#if 0 static int workqueue_do_shutdown(void *state, void *work) { (void)state; (void)work; - (void)cmd; crypto_pk_free(((state_t*)state)->rsa); tor_free(state); return WQ_RPL_SHUTDOWN; } -#endif static int workqueue_do_ecdh(void *state, void *work) @@ -197,6 +195,20 @@ add_work(threadpool_t *tp) } } +static int shutting_down = 0; +static int n_shutdowns_done = 0; + +static void +shutdown_reply(void *arg) +{ + (void)arg; + tor_assert(shutting_down); + ++n_shutdowns_done; + if (n_shutdowns_done == opt_n_threads) { + tor_event_base_loopexit(tor_libevent_get_base(), NULL); + } +} + static void replysock_readable_cb(tor_socket_t sock, short what, void *arg) { @@ -236,8 +248,9 @@ replysock_readable_cb(tor_socket_t sock, short what, void *arg) } } - if (n_received == n_sent && n_sent >= opt_n_items) { - tor_event_base_loopexit(tor_libevent_get_base(), NULL); + if (shutting_down == 0 && n_received == n_sent && n_sent >= opt_n_items) { + shutting_down = 1; + threadpool_queue_for_all(tp, NULL, workqueue_do_shutdown, shutdown_reply, NULL); } } @@ -345,7 +358,8 @@ main(int argc, char **argv) event_base_loop(tor_libevent_get_base(), 0); - if (n_sent != opt_n_items || n_received != n_sent) { + if (n_sent != opt_n_items || n_received != n_sent || + n_shutdowns_done != opt_n_threads) { puts("FAIL"); return 1; } else { From e5f8c772f4c468a20da8b9176c2b276ac76bbe78 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Sat, 28 Sep 2013 00:33:10 -0400 Subject: [PATCH 21/32] Test and fix workqueue_entry_cancel(). --- src/common/workqueue.c | 12 +++--- src/common/workqueue.h | 2 +- src/test/test_workqueue.c | 77 ++++++++++++++++++++++++++++++++------- 3 files changed, 71 insertions(+), 20 deletions(-) diff --git a/src/common/workqueue.c b/src/common/workqueue.c index 9293e1f9f0..44cf98d0dc 100644 --- a/src/common/workqueue.c +++ b/src/common/workqueue.c @@ -119,27 +119,29 @@ workqueue_entry_free(workqueue_entry_t *ent) * executed in the main thread; that will cause undefined behavior (probably, * a crash). * - * If the work is cancelled, this function return 1. It is the caller's - * responsibility to free any storage in the work function's arguments. + * If the work is cancelled, this function return the argument passed to the + * work function. It is the caller's responsibility to free this storage. * * This function will have no effect if the worker thread has already executed - * or begun to execute the work item. In that case, it will return 0. + * or begun to execute the work item. In that case, it will return NULL. */ -int +void * workqueue_entry_cancel(workqueue_entry_t *ent) { int cancelled = 0; + void *result = NULL; tor_mutex_acquire(&ent->on_thread->lock); if (ent->pending) { TOR_TAILQ_REMOVE(&ent->on_thread->work, ent, next_work); cancelled = 1; + result = ent->arg; } tor_mutex_release(&ent->on_thread->lock); if (cancelled) { tor_free(ent); } - return cancelled; + return result; } /** diff --git a/src/common/workqueue.h b/src/common/workqueue.h index 5a6cd80fb0..ec1f7c9000 100644 --- a/src/common/workqueue.h +++ b/src/common/workqueue.h @@ -32,7 +32,7 @@ int threadpool_queue_for_all(threadpool_t *pool, int (*fn)(void *, void *), void (*reply_fn)(void *), void *arg); -int workqueue_entry_cancel(workqueue_entry_t *pending_work); +void *workqueue_entry_cancel(workqueue_entry_t *pending_work); threadpool_t *threadpool_new(int n_threads, replyqueue_t *replyqueue, void *(*new_thread_state_fn)(void*), diff --git a/src/test/test_workqueue.c b/src/test/test_workqueue.c index cbf9d81950..6de6f03c33 100644 --- a/src/test/test_workqueue.c +++ b/src/test/test_workqueue.c @@ -23,6 +23,7 @@ static int opt_n_threads = 8; static int opt_n_items = 10000; static int opt_n_inflight = 1000; static int opt_n_lowwater = 250; +static int opt_n_cancel = 0; static int opt_ratio_rsa = 5; #ifdef TRACK_RESPONSES @@ -172,29 +173,70 @@ handle_reply(void *arg) ++n_received; } -static int +static workqueue_entry_t * add_work(threadpool_t *tp) { int add_rsa = opt_ratio_rsa == 0 || tor_weak_random_range(&weak_rng, opt_ratio_rsa) == 0; + if (add_rsa) { rsa_work_t *w = tor_malloc_zero(sizeof(*w)); w->serial = n_sent++; crypto_rand((char*)w->msg, 20); w->msglen = 20; ++rsa_sent; - return threadpool_queue_work(tp, workqueue_do_rsa, handle_reply, w) != NULL; + return threadpool_queue_work(tp, workqueue_do_rsa, handle_reply, w); } else { ecdh_work_t *w = tor_malloc_zero(sizeof(*w)); w->serial = n_sent++; /* Not strictly right, but this is just for benchmarks. */ crypto_rand((char*)w->u.pk.public_key, 32); ++ecdh_sent; - return threadpool_queue_work(tp, workqueue_do_ecdh, handle_reply, w) != NULL; + return threadpool_queue_work(tp, workqueue_do_ecdh, handle_reply, w); } } +static int n_failed_cancel = 0; +static int n_successful_cancel = 0; + +static int +add_n_work_items(threadpool_t *tp, int n) +{ + int n_queued = 0; + int n_try_cancel = 0, i; + workqueue_entry_t **to_cancel; + workqueue_entry_t *ent; + + to_cancel = tor_malloc(sizeof(workqueue_entry_t*) * opt_n_cancel); + + while (n_queued++ < n) { + ent = add_work(tp); + if (! ent) { + puts("Couldn't add work."); + tor_event_base_loopexit(tor_libevent_get_base(), NULL); + return -1; + } + if (n_try_cancel < opt_n_cancel && + tor_weak_random_range(&weak_rng, n) < opt_n_cancel) { + to_cancel[n_try_cancel++] = ent; + } + } + + for (i = 0; i < n_try_cancel; ++i) { + void *work = workqueue_entry_cancel(to_cancel[i]); + if (! work) { + n_failed_cancel++; + } else { + n_successful_cancel++; + tor_free(work); + } + } + + tor_free(to_cancel); + return 0; +} + static int shutting_down = 0; static int n_shutdowns_done = 0; @@ -223,8 +265,13 @@ replysock_readable_cb(tor_socket_t sock, short what, void *arg) if (old_r == n_received) return; - if (opt_verbose) - printf("%d / %d\n", n_received, n_sent); + if (opt_verbose) { + printf("%d / %d", n_received, n_sent); + if (opt_n_cancel) + printf(" (%d cancelled, %d uncancellable)", + n_successful_cancel, n_failed_cancel); + puts(""); + } #ifdef TRACK_RESPONSES tor_mutex_acquire(&bitmap_mutex); for (i = 0; i < opt_n_items; ++i) { @@ -239,16 +286,14 @@ replysock_readable_cb(tor_socket_t sock, short what, void *arg) tor_mutex_release(&bitmap_mutex); #endif - if (n_sent - n_received < opt_n_lowwater) { - while (n_sent < n_received + opt_n_inflight && n_sent < opt_n_items) { - if (! add_work(tp)) { - puts("Couldn't add work."); - tor_event_base_loopexit(tor_libevent_get_base(), NULL); - } - } + if (n_sent - (n_received+n_successful_cancel) < opt_n_lowwater) { + int n_to_send = n_received + opt_n_inflight - n_sent; + if (n_to_send > opt_n_items - n_sent) + n_to_send = opt_n_items - n_sent; + add_n_work_items(tp, n_to_send); } - if (shutting_down == 0 && n_received == n_sent && n_sent >= opt_n_items) { + if (shutting_down == 0 && n_received+n_successful_cancel == n_sent && n_sent >= opt_n_items) { shutting_down = 1; threadpool_queue_for_all(tp, NULL, workqueue_do_shutdown, shutdown_reply, NULL); } @@ -263,6 +308,7 @@ help(void) " -T Use this many threads\n" " -I Have no more than this many requests queued at once\n" " -L Add items whenever fewer than this many are pending\n" + " -C Try to cancel N items of every batch that we add\n" " -R Make one out of this many items be a slow (RSA) one\n" " --no-{eventfd2,eventfd,pipe2,pipe,socketpair}\n" " Disable one of the alert_socket backends."); @@ -291,6 +337,8 @@ main(int argc, char **argv) opt_n_lowwater = atoi(argv[++i]); } else if (!strcmp(argv[i], "-R") && i+1 opt_n_inflight || opt_ratio_rsa < 0) { help(); return 1; @@ -358,7 +407,7 @@ main(int argc, char **argv) event_base_loop(tor_libevent_get_base(), 0); - if (n_sent != opt_n_items || n_received != n_sent || + if (n_sent != opt_n_items || n_received+n_successful_cancel != n_sent || n_shutdowns_done != opt_n_threads) { puts("FAIL"); return 1; From cc6529e9bb7d7e01a25b5632d6d6c2424c6fc2b4 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Sat, 28 Sep 2013 00:52:28 -0400 Subject: [PATCH 22/32] Fix check-spaces --- src/common/compat_pthreads.c | 1 + src/common/compat_threads.c | 1 + src/common/compat_threads.h | 2 +- src/common/compat_winthreads.c | 3 +-- src/common/workqueue.h | 1 + src/test/test.c | 1 - src/test/test_threads.c | 1 + src/test/test_workqueue.c | 8 ++++++-- 8 files changed, 12 insertions(+), 6 deletions(-) diff --git a/src/common/compat_pthreads.c b/src/common/compat_pthreads.c index 69f7bac9c9..848bfe0973 100644 --- a/src/common/compat_pthreads.c +++ b/src/common/compat_pthreads.c @@ -240,3 +240,4 @@ tor_threads_init(void) set_main_thread(); } } + diff --git a/src/common/compat_threads.c b/src/common/compat_threads.c index f018475e18..79440070a2 100644 --- a/src/common/compat_threads.c +++ b/src/common/compat_threads.c @@ -253,3 +253,4 @@ alert_sockets_close(alert_sockets_t *socks) } socks->read_fd = socks->write_fd = -1; } + diff --git a/src/common/compat_threads.h b/src/common/compat_threads.h index a5db72d45f..acf3083f37 100644 --- a/src/common/compat_threads.h +++ b/src/common/compat_threads.h @@ -44,7 +44,6 @@ typedef struct tor_mutex_t { #endif } tor_mutex_t; - tor_mutex_t *tor_mutex_new(void); tor_mutex_t *tor_mutex_new_nonrecursive(void); void tor_mutex_init(tor_mutex_t *m); @@ -113,3 +112,4 @@ int alert_sockets_create(alert_sockets_t *socks_out, uint32_t flags); void alert_sockets_close(alert_sockets_t *socks); #endif + diff --git a/src/common/compat_winthreads.c b/src/common/compat_winthreads.c index 4820eb3481..71b994c4e4 100644 --- a/src/common/compat_winthreads.c +++ b/src/common/compat_winthreads.c @@ -33,7 +33,6 @@ spawn_func(void (*func)(void *), void *data) return 0; } - /** End the current thread/process. */ void @@ -46,7 +45,6 @@ spawn_exit(void) _exit(0); } - void tor_mutex_init(tor_mutex_t *m) { @@ -195,3 +193,4 @@ tor_threads_init(void) { set_main_thread(); } + diff --git a/src/common/workqueue.h b/src/common/workqueue.h index ec1f7c9000..aa8620ddb7 100644 --- a/src/common/workqueue.h +++ b/src/common/workqueue.h @@ -45,3 +45,4 @@ tor_socket_t replyqueue_get_socket(replyqueue_t *rq); void replyqueue_process(replyqueue_t *queue); #endif + diff --git a/src/test/test.c b/src/test/test.c index 9171306d18..2c2328c197 100644 --- a/src/test/test.c +++ b/src/test/test.c @@ -1258,7 +1258,6 @@ test_stats(void *arg) tor_free(s); } - static void * passthrough_test_setup(const struct testcase_t *testcase) { diff --git a/src/test/test_threads.c b/src/test/test_threads.c index 2bc24e1edc..c0293048fe 100644 --- a/src/test/test_threads.c +++ b/src/test/test_threads.c @@ -313,3 +313,4 @@ struct testcase_t thread_tests[] = { &passthrough_setup, (void*)"tv" }, END_OF_TESTCASES }; + diff --git a/src/test/test_workqueue.c b/src/test/test_workqueue.c index 6de6f03c33..410f43cce4 100644 --- a/src/test/test_workqueue.c +++ b/src/test/test_workqueue.c @@ -293,9 +293,12 @@ replysock_readable_cb(tor_socket_t sock, short what, void *arg) add_n_work_items(tp, n_to_send); } - if (shutting_down == 0 && n_received+n_successful_cancel == n_sent && n_sent >= opt_n_items) { + if (shutting_down == 0 && + n_received+n_successful_cancel == n_sent && + n_sent >= opt_n_items) { shutting_down = 1; - threadpool_queue_for_all(tp, NULL, workqueue_do_shutdown, shutdown_reply, NULL); + threadpool_queue_for_all(tp, NULL, + workqueue_do_shutdown, shutdown_reply, NULL); } } @@ -416,3 +419,4 @@ main(int argc, char **argv) return 0; } } + From 1e896214e7eb5ede65663486291252b171e9daea Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 2 Oct 2013 12:32:09 -0400 Subject: [PATCH 23/32] Refactor cpuworker to use workqueue/threadpool code. --- changes/better_workqueues | 10 + src/common/workqueue.c | 4 +- src/common/workqueue.h | 2 +- src/or/command.c | 2 +- src/or/config.c | 2 +- src/or/connection.c | 24 +- src/or/cpuworker.c | 542 ++++++++++++++------------------------ src/or/cpuworker.h | 10 +- src/or/main.c | 6 +- src/or/onion.c | 19 +- src/or/onion.h | 4 +- src/or/or.h | 17 +- 12 files changed, 243 insertions(+), 399 deletions(-) create mode 100644 changes/better_workqueues diff --git a/changes/better_workqueues b/changes/better_workqueues new file mode 100644 index 0000000000..32c984cb71 --- /dev/null +++ b/changes/better_workqueues @@ -0,0 +1,10 @@ + o Major features: + - Refactor the CPU worker implementation for better performance by + avoiding the kernel and lengthening pipelines. The original + implementation used sockets to transfer data from the main thread + to the worker threads, and didn't allow any thread to be assigned + more than a single piece of work at once. The new implementation + avoids communications overhead by making requests in shared + memory, avoiding kernel IO where possible, and keeping more + request in flight at once. Resolves issue #9682. + diff --git a/src/common/workqueue.c b/src/common/workqueue.c index 44cf98d0dc..f3ef67891d 100644 --- a/src/common/workqueue.c +++ b/src/common/workqueue.c @@ -108,6 +108,7 @@ workqueue_entry_free(workqueue_entry_t *ent) { if (!ent) return; + memset(ent, 0xf0, sizeof(*ent)); tor_free(ent); } @@ -310,7 +311,7 @@ threadpool_queue_work(threadpool_t *pool, */ int threadpool_queue_for_all(threadpool_t *pool, - void *(*dup_fn)(const void *), + void *(*dup_fn)(void *), int (*fn)(void *, void *), void (*reply_fn)(void *), void *arg) @@ -444,6 +445,7 @@ replyqueue_process(replyqueue_t *queue) workqueue_entry_t *work = TOR_TAILQ_FIRST(&queue->answers); TOR_TAILQ_REMOVE(&queue->answers, work, next_work); tor_mutex_release(&queue->lock); + work->on_thread = NULL; work->reply_fn(work->arg); workqueue_entry_free(work); diff --git a/src/common/workqueue.h b/src/common/workqueue.h index aa8620ddb7..aa1bcc518a 100644 --- a/src/common/workqueue.h +++ b/src/common/workqueue.h @@ -28,7 +28,7 @@ workqueue_entry_t *threadpool_queue_work(threadpool_t *pool, void (*reply_fn)(void *), void *arg); int threadpool_queue_for_all(threadpool_t *pool, - void *(*dup_fn)(const void *), + void *(*dup_fn)(void *), int (*fn)(void *, void *), void (*reply_fn)(void *), void *arg); diff --git a/src/or/command.c b/src/or/command.c index 6dde2a9b7e..c4a0f9baeb 100644 --- a/src/or/command.c +++ b/src/or/command.c @@ -310,7 +310,7 @@ command_process_create_cell(cell_t *cell, channel_t *chan) /* hand it off to the cpuworkers, and then return. */ if (connection_or_digest_is_known_relay(chan->identity_digest)) rep_hist_note_circuit_handshake_requested(create_cell->handshake_type); - if (assign_onionskin_to_cpuworker(NULL, circ, create_cell) < 0) { + if (assign_onionskin_to_cpuworker(circ, create_cell) < 0) { log_debug(LD_GENERAL,"Failed to hand off onionskin. Closing."); circuit_mark_for_close(TO_CIRCUIT(circ), END_CIRC_REASON_RESOURCELIMIT); return; diff --git a/src/or/config.c b/src/or/config.c index 5db065f000..5b8560b9e4 100644 --- a/src/or/config.c +++ b/src/or/config.c @@ -1729,7 +1729,7 @@ options_act(const or_options_t *old_options) if (have_completed_a_circuit() || !any_predicted_circuits(time(NULL))) inform_testing_reachability(); } - cpuworkers_rotate(); + cpuworkers_rotate_keyinfo(); if (dns_reset()) return -1; } else { diff --git a/src/or/connection.c b/src/or/connection.c index 11ff224e67..1b7426b588 100644 --- a/src/or/connection.c +++ b/src/or/connection.c @@ -29,7 +29,6 @@ #include "connection_edge.h" #include "connection_or.h" #include "control.h" -#include "cpuworker.h" #include "directory.h" #include "dirserv.h" #include "dns.h" @@ -130,7 +129,6 @@ conn_type_to_string(int type) case CONN_TYPE_AP: return "Socks"; case CONN_TYPE_DIR_LISTENER: return "Directory listener"; case CONN_TYPE_DIR: return "Directory"; - case CONN_TYPE_CPUWORKER: return "CPU worker"; case CONN_TYPE_CONTROL_LISTENER: return "Control listener"; case CONN_TYPE_CONTROL: return "Control"; case CONN_TYPE_EXT_OR: return "Extended OR"; @@ -213,12 +211,6 @@ conn_state_to_string(int type, int state) case DIR_CONN_STATE_SERVER_WRITING: return "writing"; } break; - case CONN_TYPE_CPUWORKER: - switch (state) { - case CPUWORKER_STATE_IDLE: return "idle"; - case CPUWORKER_STATE_BUSY_ONION: return "busy with onion"; - } - break; case CONN_TYPE_CONTROL: switch (state) { case CONTROL_CONN_STATE_OPEN: return "open (protocol v1)"; @@ -248,7 +240,6 @@ connection_type_uses_bufferevent(connection_t *conn) case CONN_TYPE_CONTROL: case CONN_TYPE_OR: case CONN_TYPE_EXT_OR: - case CONN_TYPE_CPUWORKER: return 1; default: return 0; @@ -2436,7 +2427,6 @@ connection_mark_all_noncontrol_connections(void) if (conn->marked_for_close) continue; switch (conn->type) { - case CONN_TYPE_CPUWORKER: case CONN_TYPE_CONTROL_LISTENER: case CONN_TYPE_CONTROL: break; @@ -4530,8 +4520,6 @@ connection_process_inbuf(connection_t *conn, int package_partial) package_partial); case CONN_TYPE_DIR: return connection_dir_process_inbuf(TO_DIR_CONN(conn)); - case CONN_TYPE_CPUWORKER: - return connection_cpu_process_inbuf(conn); case CONN_TYPE_CONTROL: return connection_control_process_inbuf(TO_CONTROL_CONN(conn)); default: @@ -4591,8 +4579,6 @@ connection_finished_flushing(connection_t *conn) return connection_edge_finished_flushing(TO_EDGE_CONN(conn)); case CONN_TYPE_DIR: return connection_dir_finished_flushing(TO_DIR_CONN(conn)); - case CONN_TYPE_CPUWORKER: - return connection_cpu_finished_flushing(conn); case CONN_TYPE_CONTROL: return connection_control_finished_flushing(TO_CONTROL_CONN(conn)); default: @@ -4648,8 +4634,6 @@ connection_reached_eof(connection_t *conn) return connection_edge_reached_eof(TO_EDGE_CONN(conn)); case CONN_TYPE_DIR: return connection_dir_reached_eof(TO_DIR_CONN(conn)); - case CONN_TYPE_CPUWORKER: - return connection_cpu_reached_eof(conn); case CONN_TYPE_CONTROL: return connection_control_reached_eof(TO_CONTROL_CONN(conn)); default: @@ -4855,10 +4839,6 @@ assert_connection_ok(connection_t *conn, time_t now) tor_assert(conn->purpose >= DIR_PURPOSE_MIN_); tor_assert(conn->purpose <= DIR_PURPOSE_MAX_); break; - case CONN_TYPE_CPUWORKER: - tor_assert(conn->state >= CPUWORKER_STATE_MIN_); - tor_assert(conn->state <= CPUWORKER_STATE_MAX_); - break; case CONN_TYPE_CONTROL: tor_assert(conn->state >= CONTROL_CONN_STATE_MIN_); tor_assert(conn->state <= CONTROL_CONN_STATE_MAX_); @@ -4959,9 +4939,7 @@ proxy_type_to_string(int proxy_type) } /** Call connection_free_() on every connection in our array, and release all - * storage held by connection.c. This is used by cpuworkers and dnsworkers - * when they fork, so they don't keep resources held open (especially - * sockets). + * storage held by connection.c. * * Don't do the checks in connection_free(), because they will * fail. diff --git a/src/or/cpuworker.c b/src/or/cpuworker.c index 340fbec620..f3f275d099 100644 --- a/src/or/cpuworker.c +++ b/src/or/cpuworker.c @@ -5,84 +5,98 @@ /** * \file cpuworker.c - * \brief Implements a farm of 'CPU worker' processes to perform - * CPU-intensive tasks in another thread or process, to not - * interrupt the main thread. + * \brief Uses the workqueue/threadpool code to farm CPU-intensive activities + * out to subprocesses. * * Right now, we only use this for processing onionskins. **/ #include "or.h" -#include "buffers.h" #include "channel.h" -#include "channeltls.h" #include "circuitbuild.h" #include "circuitlist.h" -#include "config.h" -#include "connection.h" #include "connection_or.h" +#include "config.h" #include "cpuworker.h" #include "main.h" #include "onion.h" #include "rephist.h" #include "router.h" +#include "workqueue.h" -/** The maximum number of cpuworker processes we will keep around. */ -#define MAX_CPUWORKERS 16 -/** The minimum number of cpuworker processes we will keep around. */ -#define MIN_CPUWORKERS 1 +#ifdef HAVE_EVENT2_EVENT_H +#include +#else +#include +#endif -/** The tag specifies which circuit this onionskin was from. */ -#define TAG_LEN 12 +static void queue_pending_tasks(void); -/** How many cpuworkers we have running right now. */ -static int num_cpuworkers=0; -/** How many of the running cpuworkers have an assigned task right now. */ -static int num_cpuworkers_busy=0; -/** We need to spawn new cpuworkers whenever we rotate the onion keys - * on platforms where execution contexts==processes. This variable stores - * the last time we got a key rotation event. */ -static time_t last_rotation_time=0; +typedef struct worker_state_s { + int generation; + server_onion_keys_t *onion_keys; +} worker_state_t; -static void cpuworker_main(void *data) ATTR_NORETURN; -static int spawn_cpuworker(void); -static void spawn_enough_cpuworkers(void); -static void process_pending_task(connection_t *cpuworker); +static void * +worker_state_new(void *arg) +{ + worker_state_t *ws; + (void)arg; + ws = tor_malloc_zero(sizeof(worker_state_t)); + ws->onion_keys = server_onion_keys_new(); + return ws; +} +static void +worker_state_free(void *arg) +{ + worker_state_t *ws = arg; + server_onion_keys_free(ws->onion_keys); + tor_free(ws); +} + +static replyqueue_t *replyqueue = NULL; +static threadpool_t *threadpool = NULL; +static struct event *reply_event = NULL; + +static tor_weak_rng_t request_sample_rng = TOR_WEAK_RNG_INIT; + +static int total_pending_tasks = 0; +static int max_pending_tasks = 128; + +static void +replyqueue_process_cb(evutil_socket_t sock, short events, void *arg) +{ + replyqueue_t *rq = arg; + (void) sock; + (void) events; + replyqueue_process(rq); +} /** Initialize the cpuworker subsystem. */ void cpu_init(void) { - cpuworkers_rotate(); -} - -/** Called when we're done sending a request to a cpuworker. */ -int -connection_cpu_finished_flushing(connection_t *conn) -{ - tor_assert(conn); - tor_assert(conn->type == CONN_TYPE_CPUWORKER); - return 0; -} - -/** Pack global_id and circ_id; set *tag to the result. (See note on - * cpuworker_main for wire format.) */ -static void -tag_pack(uint8_t *tag, uint64_t chan_id, circid_t circ_id) -{ - /*XXXX RETHINK THIS WHOLE MESS !!!! !NM NM NM NM*/ - /*XXXX DOUBLEPLUSTHIS!!!! AS AS AS AS*/ - set_uint64(tag, chan_id); - set_uint32(tag+8, circ_id); -} - -/** Unpack tag into addr, port, and circ_id. - */ -static void -tag_unpack(const uint8_t *tag, uint64_t *chan_id, circid_t *circ_id) -{ - *chan_id = get_uint64(tag); - *circ_id = get_uint32(tag+8); + if (!replyqueue) { + replyqueue = replyqueue_new(0); + } + if (!reply_event) { + reply_event = tor_event_new(tor_libevent_get_base(), + replyqueue_get_socket(replyqueue), + EV_READ|EV_PERSIST, + replyqueue_process_cb, + replyqueue); + event_add(reply_event, NULL); + } + if (!threadpool) { + threadpool = threadpool_new(get_num_cpus(get_options()), + replyqueue, + worker_state_new, + worker_state_free, + NULL); + } + /* Total voodoo. Can we make this more sensible? */ + max_pending_tasks = get_num_cpus(get_options()) * 64; + crypto_seed_weak_rng(&request_sample_rng); } /** Magic numbers to make sure our cpuworker_requests don't grow any @@ -94,10 +108,6 @@ tag_unpack(const uint8_t *tag, uint64_t *chan_id, circid_t *circ_id) typedef struct cpuworker_request_t { /** Magic number; must be CPUWORKER_REQUEST_MAGIC. */ uint32_t magic; - /** Opaque tag to identify the job */ - uint8_t tag[TAG_LEN]; - /** Task code. Must be one of CPUWORKER_TASK_* */ - uint8_t task; /** Flag: Are we timing this request? */ unsigned timed : 1; @@ -114,8 +124,7 @@ typedef struct cpuworker_request_t { typedef struct cpuworker_reply_t { /** Magic number; must be CPUWORKER_REPLY_MAGIC. */ uint32_t magic; - /** Opaque tag to identify the job; matches the request's tag.*/ - uint8_t tag[TAG_LEN]; + /** True iff we got a successful request. */ uint8_t success; @@ -142,42 +151,46 @@ typedef struct cpuworker_reply_t { uint8_t rend_auth_material[DIGEST_LEN]; } cpuworker_reply_t; +typedef struct cpuworker_job_u { + uint64_t chan_id; + uint32_t circ_id; + union { + cpuworker_request_t request; + cpuworker_reply_t reply; + } u; +} cpuworker_job_t; + +static int +update_state_threadfn(void *state_, void *work_) +{ + worker_state_t *state = state_; + worker_state_t *update = work_; + server_onion_keys_free(state->onion_keys); + state->onion_keys = update->onion_keys; + update->onion_keys = NULL; + ++state->generation; + return WQ_RPL_REPLY; +} +static void +update_state_replyfn(void *work_) +{ + tor_free(work_); +} + /** Called when the onion key has changed and we need to spawn new * cpuworkers. Close all currently idle cpuworkers, and mark the last * rotation time as now. */ void -cpuworkers_rotate(void) +cpuworkers_rotate_keyinfo(void) { - connection_t *cpuworker; - while ((cpuworker = connection_get_by_type_state(CONN_TYPE_CPUWORKER, - CPUWORKER_STATE_IDLE))) { - connection_mark_for_close(cpuworker); - --num_cpuworkers; + if (threadpool_queue_for_all(threadpool, + worker_state_new, + update_state_threadfn, + update_state_replyfn, + NULL)) { + log_warn(LD_OR, "Failed to queue key update for worker threads."); } - last_rotation_time = time(NULL); - if (server_mode(get_options())) - spawn_enough_cpuworkers(); -} - -/** If the cpuworker closes the connection, - * mark it as closed and spawn a new one as needed. */ -int -connection_cpu_reached_eof(connection_t *conn) -{ - log_warn(LD_GENERAL,"Read eof. CPU worker died unexpectedly."); - if (conn->state != CPUWORKER_STATE_IDLE) { - /* the circ associated with this cpuworker will have to wait until - * it gets culled in run_connection_housekeeping(), since we have - * no way to find out which circ it was. */ - log_warn(LD_GENERAL,"...and it left a circuit queued; abandoning circ."); - num_cpuworkers_busy--; - } - num_cpuworkers--; - spawn_enough_cpuworkers(); /* try to regrow. hope we don't end up - spinning. */ - connection_mark_for_close(conn); - return 0; } /** Indexed by handshake type: how many onionskins have we processed and @@ -197,8 +210,6 @@ static uint64_t onionskins_usec_roundtrip[MAX_ONION_HANDSHAKE_TYPE+1]; * time. (microseconds) */ #define MAX_BELIEVABLE_ONIONSKIN_DELAY (2*1000*1000) -static tor_weak_rng_t request_sample_rng = TOR_WEAK_RNG_INIT; - /** Return true iff we'd like to measure a handshake of type * onionskin_type. Call only from the main thread. */ static int @@ -286,31 +297,22 @@ cpuworker_log_onionskin_overhead(int severity, int onionskin_type, onionskin_type_name, (unsigned)overhead, relative_overhead*100); } -/** Called when we get data from a cpuworker. If the answer is not complete, - * wait for a complete answer. If the answer is complete, - * process it as appropriate. - */ -int -connection_cpu_process_inbuf(connection_t *conn) +/** */ +static void +cpuworker_onion_handshake_replyfn(void *work_) { + cpuworker_job_t *job = work_; + cpuworker_reply_t rpl; uint64_t chan_id; circid_t circ_id; channel_t *p_chan = NULL; - circuit_t *circ; + circuit_t *circ = NULL; - tor_assert(conn); - tor_assert(conn->type == CONN_TYPE_CPUWORKER); + --total_pending_tasks; - if (!connection_get_inbuf_len(conn)) - return 0; - - if (conn->state == CPUWORKER_STATE_BUSY_ONION) { - cpuworker_reply_t rpl; - if (connection_get_inbuf_len(conn) < sizeof(cpuworker_reply_t)) - return 0; /* not yet */ - tor_assert(connection_get_inbuf_len(conn) == sizeof(cpuworker_reply_t)); - - connection_fetch_from_buf((void*)&rpl,sizeof(cpuworker_reply_t),conn); + if (1) { + /* Could avoid this, but doesn't matter. */ + memcpy(&rpl, &job->u.reply, sizeof(rpl)); tor_assert(rpl.magic == CPUWORKER_REPLY_MAGIC); @@ -337,18 +339,21 @@ connection_cpu_process_inbuf(connection_t *conn) } } } - /* parse out the circ it was talking about */ - tag_unpack(rpl.tag, &chan_id, &circ_id); - circ = NULL; - log_debug(LD_OR, - "Unpacking cpuworker reply, chan_id is " U64_FORMAT - ", circ_id is %u", - U64_PRINTF_ARG(chan_id), (unsigned)circ_id); + /* Find the circ it was talking about */ + chan_id = job->chan_id; + circ_id = job->circ_id; + p_chan = channel_find_by_global_id(chan_id); if (p_chan) circ = circuit_get_by_circid_channel(circ_id, p_chan); + log_debug(LD_OR, + "Unpacking cpuworker reply %p, chan_id is " U64_FORMAT + ", circ_id is %u, p_chan=%p, circ=%p, success=%d", + job, U64_PRINTF_ARG(chan_id), (unsigned)circ_id, + p_chan, circ, rpl.success); + if (rpl.success == 0) { log_debug(LD_OR, "decoding onionskin failed. " @@ -367,6 +372,7 @@ connection_cpu_process_inbuf(connection_t *conn) goto done_processing; } tor_assert(! CIRCUIT_IS_ORIGIN(circ)); + TO_OR_CIRCUIT(circ)->workqueue_entry = NULL; if (onionskin_answer(TO_OR_CIRCUIT(circ), &rpl.created_cell, (const char*)rpl.keys, @@ -376,58 +382,33 @@ connection_cpu_process_inbuf(connection_t *conn) goto done_processing; } log_debug(LD_OR,"onionskin_answer succeeded. Yay."); - } else { - tor_assert(0); /* don't ask me to do handshakes yet */ } done_processing: - conn->state = CPUWORKER_STATE_IDLE; - num_cpuworkers_busy--; - if (conn->timestamp_created < last_rotation_time) { - connection_mark_for_close(conn); - num_cpuworkers--; - spawn_enough_cpuworkers(); - } else { - process_pending_task(conn); - } - return 0; + memwipe(&rpl, 0, sizeof(rpl)); + memwipe(job, 0, sizeof(*job)); + tor_free(job); + queue_pending_tasks(); } -/** Implement a cpuworker. 'data' is an fdarray as returned by socketpair. - * Read and writes from fdarray[1]. Reads requests, writes answers. - * - * Request format: - * cpuworker_request_t. - * Response format: - * cpuworker_reply_t - */ -static void -cpuworker_main(void *data) +/** Implementation function for onion handshake requests. */ +static int +cpuworker_onion_handshake_threadfn(void *state_, void *work_) { - /* For talking to the parent thread/process */ - tor_socket_t *fdarray = data; - tor_socket_t fd; + worker_state_t *state = state_; + cpuworker_job_t *job = work_; /* variables for onion processing */ - server_onion_keys_t onion_keys; + server_onion_keys_t *onion_keys = state->onion_keys; cpuworker_request_t req; cpuworker_reply_t rpl; - fd = fdarray[1]; /* this side is ours */ - tor_free(data); + memcpy(&req, &job->u.request, sizeof(req)); - setup_server_onion_keys(&onion_keys); + tor_assert(req.magic == CPUWORKER_REQUEST_MAGIC); + memset(&rpl, 0, sizeof(rpl)); - for (;;) { - if (read_all(fd, (void *)&req, sizeof(req), 1) != sizeof(req)) { - log_info(LD_OR, "read request failed. Exiting."); - goto end; - } - tor_assert(req.magic == CPUWORKER_REQUEST_MAGIC); - - memset(&rpl, 0, sizeof(rpl)); - - if (req.task == CPUWORKER_TASK_ONION) { + if (1) { const create_cell_t *cc = &req.create_cell; created_cell_t *cell_out = &rpl.created_cell; struct timeval tv_start = {0,0}, tv_end; @@ -439,7 +420,7 @@ cpuworker_main(void *data) tor_gettimeofday(&tv_start); n = onion_skin_server_handshake(cc->handshake_type, cc->onionskin, cc->handshake_len, - &onion_keys, + onion_keys, cell_out->reply, rpl.keys, CPATH_KEY_MATERIAL_LEN, rpl.rend_auth_material); @@ -447,12 +428,10 @@ cpuworker_main(void *data) /* failure */ log_debug(LD_OR,"onion_skin_server_handshake failed."); memset(&rpl, 0, sizeof(rpl)); - memcpy(rpl.tag, req.tag, TAG_LEN); rpl.success = 0; } else { /* success */ log_debug(LD_OR,"onion_skin_server_handshake succeeded."); - memcpy(rpl.tag, req.tag, TAG_LEN); cell_out->handshake_len = n; switch (cc->cell_type) { case CELL_CREATE: @@ -463,7 +442,7 @@ cpuworker_main(void *data) cell_out->cell_type = CELL_CREATED_FAST; break; default: tor_assert(0); - goto end; + return WQ_RPL_SHUTDOWN; } rpl.success = 1; } @@ -479,187 +458,55 @@ cpuworker_main(void *data) else rpl.n_usec = (uint32_t) usec; } - if (write_all(fd, (void*)&rpl, sizeof(rpl), 1) != sizeof(rpl)) { - log_err(LD_BUG,"writing response buf failed. Exiting."); - goto end; - } - log_debug(LD_OR,"finished writing response."); - } else if (req.task == CPUWORKER_TASK_SHUTDOWN) { - log_info(LD_OR,"Clean shutdown: exiting"); - goto end; - } - memwipe(&req, 0, sizeof(req)); - memwipe(&rpl, 0, sizeof(req)); } - end: + + memcpy(&job->u.reply, &rpl, sizeof(rpl)); + memwipe(&req, 0, sizeof(req)); memwipe(&rpl, 0, sizeof(req)); - release_server_onion_keys(&onion_keys); - tor_close_socket(fd); - crypto_thread_cleanup(); - spawn_exit(); + return WQ_RPL_REPLY; } -/** Launch a new cpuworker. Return 0 if we're happy, -1 if we failed. - */ -static int -spawn_cpuworker(void) -{ - tor_socket_t *fdarray; - tor_socket_t fd; - connection_t *conn; - int err; - - fdarray = tor_calloc(2, sizeof(tor_socket_t)); - if ((err = tor_socketpair(AF_UNIX, SOCK_STREAM, 0, fdarray)) < 0) { - log_warn(LD_NET, "Couldn't construct socketpair for cpuworker: %s", - tor_socket_strerror(-err)); - tor_free(fdarray); - return -1; - } - - tor_assert(SOCKET_OK(fdarray[0])); - tor_assert(SOCKET_OK(fdarray[1])); - - fd = fdarray[0]; - if (spawn_func(cpuworker_main, (void*)fdarray) < 0) { - tor_close_socket(fdarray[0]); - tor_close_socket(fdarray[1]); - tor_free(fdarray); - return -1; - } - log_debug(LD_OR,"just spawned a cpu worker."); - - conn = connection_new(CONN_TYPE_CPUWORKER, AF_UNIX); - - /* set up conn so it's got all the data we need to remember */ - conn->s = fd; - conn->address = tor_strdup("localhost"); - tor_addr_make_unspec(&conn->addr); - - if (set_socket_nonblocking(fd) == -1) { - connection_free(conn); /* this closes fd */ - return -1; - } - - if (connection_add(conn) < 0) { /* no space, forget it */ - log_warn(LD_NET,"connection_add for cpuworker failed. Giving up."); - connection_free(conn); /* this closes fd */ - return -1; - } - - conn->state = CPUWORKER_STATE_IDLE; - connection_start_reading(conn); - - return 0; /* success */ -} - -/** If we have too few or too many active cpuworkers, try to spawn new ones - * or kill idle ones. - */ +/** Take pending tasks from the queue and assign them to cpuworkers. */ static void -spawn_enough_cpuworkers(void) -{ - int num_cpuworkers_needed = get_num_cpus(get_options()); - int reseed = 0; - - if (num_cpuworkers_needed < MIN_CPUWORKERS) - num_cpuworkers_needed = MIN_CPUWORKERS; - if (num_cpuworkers_needed > MAX_CPUWORKERS) - num_cpuworkers_needed = MAX_CPUWORKERS; - - while (num_cpuworkers < num_cpuworkers_needed) { - if (spawn_cpuworker() < 0) { - log_warn(LD_GENERAL,"Cpuworker spawn failed. Will try again later."); - return; - } - num_cpuworkers++; - reseed++; - } - - if (reseed) - crypto_seed_weak_rng(&request_sample_rng); -} - -/** Take a pending task from the queue and assign it to 'cpuworker'. */ -static void -process_pending_task(connection_t *cpuworker) +queue_pending_tasks(void) { or_circuit_t *circ; create_cell_t *onionskin = NULL; - tor_assert(cpuworker); + while (total_pending_tasks < max_pending_tasks) { + circ = onion_next_task(&onionskin); - /* for now only process onion tasks */ + if (!circ) + return; - circ = onion_next_task(&onionskin); - if (!circ) - return; - if (assign_onionskin_to_cpuworker(cpuworker, circ, onionskin)) - log_warn(LD_OR,"assign_to_cpuworker failed. Ignoring."); -} - -/** How long should we let a cpuworker stay busy before we give - * up on it and decide that we have a bug or infinite loop? - * This value is high because some servers with low memory/cpu - * sometimes spend an hour or more swapping, and Tor starves. */ -#define CPUWORKER_BUSY_TIMEOUT (60*60*12) - -/** We have a bug that I can't find. Sometimes, very rarely, cpuworkers get - * stuck in the 'busy' state, even though the cpuworker process thinks of - * itself as idle. I don't know why. But here's a workaround to kill any - * cpuworker that's been busy for more than CPUWORKER_BUSY_TIMEOUT. - */ -static void -cull_wedged_cpuworkers(void) -{ - time_t now = time(NULL); - smartlist_t *conns = get_connection_array(); - SMARTLIST_FOREACH_BEGIN(conns, connection_t *, conn) { - if (!conn->marked_for_close && - conn->type == CONN_TYPE_CPUWORKER && - conn->state == CPUWORKER_STATE_BUSY_ONION && - conn->timestamp_lastwritten + CPUWORKER_BUSY_TIMEOUT < now) { - log_notice(LD_BUG, - "closing wedged cpuworker. Can somebody find the bug?"); - num_cpuworkers_busy--; - num_cpuworkers--; - connection_mark_for_close(conn); - } - } SMARTLIST_FOREACH_END(conn); + if (assign_onionskin_to_cpuworker(circ, onionskin)) + log_warn(LD_OR,"assign_to_cpuworker failed. Ignoring."); + } } /** Try to tell a cpuworker to perform the public key operations necessary to * respond to onionskin for the circuit circ. * - * If cpuworker is defined, assert that he's idle, and use him. Else, - * look for an idle cpuworker and use him. If none idle, queue task onto the - * pending onion list and return. Return 0 if we successfully assign the - * task, or -1 on failure. + * Return 0 if we successfully assign the task, or -1 on failure. */ int -assign_onionskin_to_cpuworker(connection_t *cpuworker, - or_circuit_t *circ, +assign_onionskin_to_cpuworker(or_circuit_t *circ, create_cell_t *onionskin) { + workqueue_entry_t *queue_entry; + cpuworker_job_t *job; cpuworker_request_t req; - time_t now = approx_time(); - static time_t last_culled_cpuworkers = 0; int should_time; - /* Checking for wedged cpuworkers requires a linear search over all - * connections, so let's do it only once a minute. - */ -#define CULL_CPUWORKERS_INTERVAL 60 - - if (last_culled_cpuworkers + CULL_CPUWORKERS_INTERVAL <= now) { - cull_wedged_cpuworkers(); - spawn_enough_cpuworkers(); - last_culled_cpuworkers = now; - } - if (1) { - if (num_cpuworkers_busy == num_cpuworkers) { + if (!circ->p_chan) { + log_info(LD_OR,"circ->p_chan gone. Failing circ."); + tor_free(onionskin); + return -1; + } + + if (total_pending_tasks >= max_pending_tasks) { log_debug(LD_OR,"No idle cpuworkers. Queuing."); if (onion_pending_add(circ, onionskin) < 0) { tor_free(onionskin); @@ -668,36 +515,14 @@ assign_onionskin_to_cpuworker(connection_t *cpuworker, return 0; } - if (!cpuworker) - cpuworker = connection_get_by_type_state(CONN_TYPE_CPUWORKER, - CPUWORKER_STATE_IDLE); - - tor_assert(cpuworker); - - if (!circ->p_chan) { - log_info(LD_OR,"circ->p_chan gone. Failing circ."); - tor_free(onionskin); - return -1; - } - if (connection_or_digest_is_known_relay(circ->p_chan->identity_digest)) rep_hist_note_circuit_handshake_assigned(onionskin->handshake_type); should_time = should_time_request(onionskin->handshake_type); memset(&req, 0, sizeof(req)); req.magic = CPUWORKER_REQUEST_MAGIC; - tag_pack(req.tag, circ->p_chan->global_identifier, - circ->p_circ_id); req.timed = should_time; - cpuworker->state = CPUWORKER_STATE_BUSY_ONION; - /* touch the lastwritten timestamp, since that's how we check to - * see how long it's been since we asked the question, and sometimes - * we check before the first call to connection_handle_write(). */ - cpuworker->timestamp_lastwritten = now; - num_cpuworkers_busy++; - - req.task = CPUWORKER_TASK_ONION; memcpy(&req.create_cell, onionskin, sizeof(create_cell_t)); tor_free(onionskin); @@ -705,9 +530,46 @@ assign_onionskin_to_cpuworker(connection_t *cpuworker, if (should_time) tor_gettimeofday(&req.started_at); - connection_write_to_buf((void*)&req, sizeof(req), cpuworker); + job = tor_malloc_zero(sizeof(cpuworker_job_t)); + job->chan_id = circ->p_chan->global_identifier; + job->circ_id = circ->p_circ_id; + memcpy(&job->u.request, &req, sizeof(req)); memwipe(&req, 0, sizeof(req)); + + ++total_pending_tasks; + queue_entry = threadpool_queue_work(threadpool, + cpuworker_onion_handshake_threadfn, + cpuworker_onion_handshake_replyfn, + job); + if (!queue_entry) { + log_warn(LD_BUG, "Couldn't queue work on threadpool"); + tor_free(job); + return -1; + } + log_debug(LD_OR, "Queued task %p (qe=%p, chanid="U64_FORMAT", circid=%u)", + job, queue_entry, U64_PRINTF_ARG(job->chan_id), job->circ_id); + + circ->workqueue_entry = queue_entry; } return 0; } +/** If circ has a pending handshake that hasn't been processed yet, + * remove it from the worker queue. */ +void +cpuworker_cancel_circ_handshake(or_circuit_t *circ) +{ + cpuworker_job_t *job; + if (circ->workqueue_entry == NULL) + return; + + job = workqueue_entry_cancel(circ->workqueue_entry); + if (job) { + /* It successfully cancelled. */ + memwipe(job, 0xe0, sizeof(*job)); + tor_free(job); + } + + circ->workqueue_entry = NULL; +} + diff --git a/src/or/cpuworker.h b/src/or/cpuworker.h index 2a2b37a975..70a595e472 100644 --- a/src/or/cpuworker.h +++ b/src/or/cpuworker.h @@ -13,19 +13,17 @@ #define TOR_CPUWORKER_H void cpu_init(void); -void cpuworkers_rotate(void); -int connection_cpu_finished_flushing(connection_t *conn); -int connection_cpu_reached_eof(connection_t *conn); -int connection_cpu_process_inbuf(connection_t *conn); +void cpuworkers_rotate_keyinfo(void); + struct create_cell_t; -int assign_onionskin_to_cpuworker(connection_t *cpuworker, - or_circuit_t *circ, +int assign_onionskin_to_cpuworker(or_circuit_t *circ, struct create_cell_t *onionskin); uint64_t estimated_usec_for_onionskins(uint32_t n_requests, uint16_t onionskin_type); void cpuworker_log_onionskin_overhead(int severity, int onionskin_type, const char *onionskin_type_name); +void cpuworker_cancel_circ_handshake(or_circuit_t *circ); #endif diff --git a/src/or/main.c b/src/or/main.c index abf3230c4c..136043c117 100644 --- a/src/or/main.c +++ b/src/or/main.c @@ -1271,7 +1271,7 @@ run_scheduled_events(time_t now) get_onion_key_set_at()+MIN_ONION_KEY_LIFETIME < now) { log_info(LD_GENERAL,"Rotating onion key."); rotate_onion_key(); - cpuworkers_rotate(); + cpuworkers_rotate_keyinfo(); if (router_rebuild_descriptor(1)<0) { log_info(LD_CONFIG, "Couldn't rebuild router descriptor"); } @@ -1960,9 +1960,9 @@ do_hup(void) * force a retry there. */ if (server_mode(options)) { - /* Restart cpuworker and dnsworker processes, so they get up-to-date + /* Update cpuworker and dnsworker processes, so they get up-to-date * configuration options. */ - cpuworkers_rotate(); + cpuworkers_rotate_keyinfo(); dns_reset(); } return 0; diff --git a/src/or/onion.c b/src/or/onion.c index 3723a3e11e..43fb63c832 100644 --- a/src/or/onion.c +++ b/src/or/onion.c @@ -295,6 +295,8 @@ onion_pending_remove(or_circuit_t *circ) victim = circ->onionqueue_entry; if (victim) onion_queue_entry_remove(victim); + + cpuworker_cancel_circ_handshake(circ); } /** Remove a queue entry victim from the queue, unlinking it from @@ -339,25 +341,25 @@ clear_pending_onions(void) /* ============================================================ */ -/** Fill in a server_onion_keys_t object at keys with all of the keys +/** Return a new server_onion_keys_t object with all of the keys * and other info we might need to do onion handshakes. (We make a copy of * our keys for each cpuworker to avoid race conditions with the main thread, * and to avoid locking) */ -void -setup_server_onion_keys(server_onion_keys_t *keys) +server_onion_keys_t * +server_onion_keys_new(void) { - memset(keys, 0, sizeof(server_onion_keys_t)); + server_onion_keys_t *keys = tor_malloc_zero(sizeof(server_onion_keys_t)); memcpy(keys->my_identity, router_get_my_id_digest(), DIGEST_LEN); dup_onion_keys(&keys->onion_key, &keys->last_onion_key); keys->curve25519_key_map = construct_ntor_key_map(); keys->junk_keypair = tor_malloc_zero(sizeof(curve25519_keypair_t)); curve25519_keypair_generate(keys->junk_keypair, 0); + return keys; } -/** Release all storage held in keys, but do not free keys - * itself (as it's likely to be stack-allocated.) */ +/** Release all storage held in keys. */ void -release_server_onion_keys(server_onion_keys_t *keys) +server_onion_keys_free(server_onion_keys_t *keys) { if (! keys) return; @@ -366,7 +368,8 @@ release_server_onion_keys(server_onion_keys_t *keys) crypto_pk_free(keys->last_onion_key); ntor_key_map_free(keys->curve25519_key_map); tor_free(keys->junk_keypair); - memset(keys, 0, sizeof(server_onion_keys_t)); + memwipe(keys, 0, sizeof(server_onion_keys_t)); + tor_free(keys); } /** Release whatever storage is held in state, depending on its diff --git a/src/or/onion.h b/src/or/onion.h index 35619879e4..96050083f8 100644 --- a/src/or/onion.h +++ b/src/or/onion.h @@ -30,8 +30,8 @@ typedef struct server_onion_keys_t { #define MAX_ONIONSKIN_CHALLENGE_LEN 255 #define MAX_ONIONSKIN_REPLY_LEN 255 -void setup_server_onion_keys(server_onion_keys_t *keys); -void release_server_onion_keys(server_onion_keys_t *keys); +server_onion_keys_t *server_onion_keys_new(void); +void server_onion_keys_free(server_onion_keys_t *keys); void onion_handshake_state_release(onion_handshake_state_t *state); diff --git a/src/or/or.h b/src/or/or.h index 8a15529336..4ff3555845 100644 --- a/src/or/or.h +++ b/src/or/or.h @@ -213,8 +213,7 @@ typedef enum { #define CONN_TYPE_DIR_LISTENER 8 /** Type for HTTP connections to the directory server. */ #define CONN_TYPE_DIR 9 -/** Connection from the main process to a CPU worker process. */ -#define CONN_TYPE_CPUWORKER 10 +/* Type 10 is unused. */ /** Type for listening for connections from user interface process. */ #define CONN_TYPE_CONTROL_LISTENER 11 /** Type for connections from user interface process. */ @@ -276,17 +275,6 @@ typedef enum { /** State for any listener connection. */ #define LISTENER_STATE_READY 0 -#define CPUWORKER_STATE_MIN_ 1 -/** State for a connection to a cpuworker process that's idle. */ -#define CPUWORKER_STATE_IDLE 1 -/** State for a connection to a cpuworker process that's processing a - * handshake. */ -#define CPUWORKER_STATE_BUSY_ONION 2 -#define CPUWORKER_STATE_MAX_ 2 - -#define CPUWORKER_TASK_ONION CPUWORKER_STATE_BUSY_ONION -#define CPUWORKER_TASK_SHUTDOWN 255 - #define OR_CONN_STATE_MIN_ 1 /** State for a connection to an OR: waiting for connect() to finish. */ #define OR_CONN_STATE_CONNECTING 1 @@ -3158,6 +3146,9 @@ typedef struct or_circuit_t { /** Pointer to an entry on the onion queue, if this circuit is waiting for a * chance to give an onionskin to a cpuworker. Used only in onion.c */ struct onion_queue_t *onionqueue_entry; + /** Pointer to a workqueue entry, if this circuit has given an onionskin to + * a cpuworker and is waiting for a response. Used only in cpuworker.c */ + struct workqueue_entry_s *workqueue_entry; /** The circuit_id used in the previous (backward) hop of this circuit. */ circid_t p_circ_id; From 6c9c54e7fa8841e3c4d4d24f5933d433171d1112 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 2 Oct 2013 12:34:08 -0400 Subject: [PATCH 24/32] Remove if (1) indentation in cpuworker.c To avoid having diffs turn out too big, I had replaced some unneeded ifs and fors with if (1), so that the indentation would still work out right. Now I might as well clean those up. --- src/or/cpuworker.c | 328 ++++++++++++++++++++++----------------------- 1 file changed, 162 insertions(+), 166 deletions(-) diff --git a/src/or/cpuworker.c b/src/or/cpuworker.c index f3f275d099..abf1f2290c 100644 --- a/src/or/cpuworker.c +++ b/src/or/cpuworker.c @@ -310,79 +310,78 @@ cpuworker_onion_handshake_replyfn(void *work_) --total_pending_tasks; - if (1) { - /* Could avoid this, but doesn't matter. */ - memcpy(&rpl, &job->u.reply, sizeof(rpl)); + /* Could avoid this, but doesn't matter. */ + memcpy(&rpl, &job->u.reply, sizeof(rpl)); - tor_assert(rpl.magic == CPUWORKER_REPLY_MAGIC); + tor_assert(rpl.magic == CPUWORKER_REPLY_MAGIC); - if (rpl.timed && rpl.success && - rpl.handshake_type <= MAX_ONION_HANDSHAKE_TYPE) { - /* Time how long this request took. The handshake_type check should be - needless, but let's leave it in to be safe. */ - struct timeval tv_end, tv_diff; - int64_t usec_roundtrip; - tor_gettimeofday(&tv_end); - timersub(&tv_end, &rpl.started_at, &tv_diff); - usec_roundtrip = ((int64_t)tv_diff.tv_sec)*1000000 + tv_diff.tv_usec; - if (usec_roundtrip >= 0 && - usec_roundtrip < MAX_BELIEVABLE_ONIONSKIN_DELAY) { - ++onionskins_n_processed[rpl.handshake_type]; - onionskins_usec_internal[rpl.handshake_type] += rpl.n_usec; - onionskins_usec_roundtrip[rpl.handshake_type] += usec_roundtrip; - if (onionskins_n_processed[rpl.handshake_type] >= 500000) { - /* Scale down every 500000 handshakes. On a busy server, that's - * less impressive than it sounds. */ - onionskins_n_processed[rpl.handshake_type] /= 2; - onionskins_usec_internal[rpl.handshake_type] /= 2; - onionskins_usec_roundtrip[rpl.handshake_type] /= 2; - } + if (rpl.timed && rpl.success && + rpl.handshake_type <= MAX_ONION_HANDSHAKE_TYPE) { + /* Time how long this request took. The handshake_type check should be + needless, but let's leave it in to be safe. */ + struct timeval tv_end, tv_diff; + int64_t usec_roundtrip; + tor_gettimeofday(&tv_end); + timersub(&tv_end, &rpl.started_at, &tv_diff); + usec_roundtrip = ((int64_t)tv_diff.tv_sec)*1000000 + tv_diff.tv_usec; + if (usec_roundtrip >= 0 && + usec_roundtrip < MAX_BELIEVABLE_ONIONSKIN_DELAY) { + ++onionskins_n_processed[rpl.handshake_type]; + onionskins_usec_internal[rpl.handshake_type] += rpl.n_usec; + onionskins_usec_roundtrip[rpl.handshake_type] += usec_roundtrip; + if (onionskins_n_processed[rpl.handshake_type] >= 500000) { + /* Scale down every 500000 handshakes. On a busy server, that's + * less impressive than it sounds. */ + onionskins_n_processed[rpl.handshake_type] /= 2; + onionskins_usec_internal[rpl.handshake_type] /= 2; + onionskins_usec_roundtrip[rpl.handshake_type] /= 2; } } - /* Find the circ it was talking about */ - chan_id = job->chan_id; - circ_id = job->circ_id; - - p_chan = channel_find_by_global_id(chan_id); - - if (p_chan) - circ = circuit_get_by_circid_channel(circ_id, p_chan); - - log_debug(LD_OR, - "Unpacking cpuworker reply %p, chan_id is " U64_FORMAT - ", circ_id is %u, p_chan=%p, circ=%p, success=%d", - job, U64_PRINTF_ARG(chan_id), (unsigned)circ_id, - p_chan, circ, rpl.success); - - if (rpl.success == 0) { - log_debug(LD_OR, - "decoding onionskin failed. " - "(Old key or bad software.) Closing."); - if (circ) - circuit_mark_for_close(circ, END_CIRC_REASON_TORPROTOCOL); - goto done_processing; - } - if (!circ) { - /* This happens because somebody sends us a destroy cell and the - * circuit goes away, while the cpuworker is working. This is also - * why our tag doesn't include a pointer to the circ, because we'd - * never know if it's still valid. - */ - log_debug(LD_OR,"processed onion for a circ that's gone. Dropping."); - goto done_processing; - } - tor_assert(! CIRCUIT_IS_ORIGIN(circ)); - TO_OR_CIRCUIT(circ)->workqueue_entry = NULL; - if (onionskin_answer(TO_OR_CIRCUIT(circ), - &rpl.created_cell, - (const char*)rpl.keys, - rpl.rend_auth_material) < 0) { - log_warn(LD_OR,"onionskin_answer failed. Closing."); - circuit_mark_for_close(circ, END_CIRC_REASON_INTERNAL); - goto done_processing; - } - log_debug(LD_OR,"onionskin_answer succeeded. Yay."); } + /* Find the circ it was talking about */ + chan_id = job->chan_id; + circ_id = job->circ_id; + + p_chan = channel_find_by_global_id(chan_id); + + if (p_chan) + circ = circuit_get_by_circid_channel(circ_id, p_chan); + + log_debug(LD_OR, + "Unpacking cpuworker reply %p, chan_id is " U64_FORMAT + ", circ_id is %u, p_chan=%p, circ=%p, success=%d", + job, U64_PRINTF_ARG(chan_id), (unsigned)circ_id, + p_chan, circ, rpl.success); + + if (rpl.success == 0) { + log_debug(LD_OR, + "decoding onionskin failed. " + "(Old key or bad software.) Closing."); + if (circ) + circuit_mark_for_close(circ, END_CIRC_REASON_TORPROTOCOL); + goto done_processing; + } + if (!circ) { + /* This happens because somebody sends us a destroy cell and the + * circuit goes away, while the cpuworker is working. This is also + * why our tag doesn't include a pointer to the circ, because we'd + * never know if it's still valid. + */ + log_debug(LD_OR,"processed onion for a circ that's gone. Dropping."); + goto done_processing; + } + tor_assert(! CIRCUIT_IS_ORIGIN(circ)); + TO_OR_CIRCUIT(circ)->workqueue_entry = NULL; + if (onionskin_answer(TO_OR_CIRCUIT(circ), + &rpl.created_cell, + (const char*)rpl.keys, + rpl.rend_auth_material) < 0) { + log_warn(LD_OR,"onionskin_answer failed. Closing."); + circuit_mark_for_close(circ, END_CIRC_REASON_INTERNAL); + goto done_processing; + } + log_debug(LD_OR,"onionskin_answer succeeded. Yay."); + done_processing: memwipe(&rpl, 0, sizeof(rpl)); @@ -408,56 +407,54 @@ cpuworker_onion_handshake_threadfn(void *state_, void *work_) tor_assert(req.magic == CPUWORKER_REQUEST_MAGIC); memset(&rpl, 0, sizeof(rpl)); - if (1) { - const create_cell_t *cc = &req.create_cell; - created_cell_t *cell_out = &rpl.created_cell; - struct timeval tv_start = {0,0}, tv_end; - int n; - rpl.timed = req.timed; - rpl.started_at = req.started_at; - rpl.handshake_type = cc->handshake_type; - if (req.timed) - tor_gettimeofday(&tv_start); - n = onion_skin_server_handshake(cc->handshake_type, - cc->onionskin, cc->handshake_len, - onion_keys, - cell_out->reply, - rpl.keys, CPATH_KEY_MATERIAL_LEN, - rpl.rend_auth_material); - if (n < 0) { - /* failure */ - log_debug(LD_OR,"onion_skin_server_handshake failed."); - memset(&rpl, 0, sizeof(rpl)); - rpl.success = 0; - } else { - /* success */ - log_debug(LD_OR,"onion_skin_server_handshake succeeded."); - cell_out->handshake_len = n; - switch (cc->cell_type) { - case CELL_CREATE: - cell_out->cell_type = CELL_CREATED; break; - case CELL_CREATE2: - cell_out->cell_type = CELL_CREATED2; break; - case CELL_CREATE_FAST: - cell_out->cell_type = CELL_CREATED_FAST; break; - default: - tor_assert(0); - return WQ_RPL_SHUTDOWN; - } - rpl.success = 1; - } - rpl.magic = CPUWORKER_REPLY_MAGIC; - if (req.timed) { - struct timeval tv_diff; - int64_t usec; - tor_gettimeofday(&tv_end); - timersub(&tv_end, &tv_start, &tv_diff); - usec = ((int64_t)tv_diff.tv_sec)*1000000 + tv_diff.tv_usec; - if (usec < 0 || usec > MAX_BELIEVABLE_ONIONSKIN_DELAY) - rpl.n_usec = MAX_BELIEVABLE_ONIONSKIN_DELAY; - else - rpl.n_usec = (uint32_t) usec; - } + const create_cell_t *cc = &req.create_cell; + created_cell_t *cell_out = &rpl.created_cell; + struct timeval tv_start = {0,0}, tv_end; + int n; + rpl.timed = req.timed; + rpl.started_at = req.started_at; + rpl.handshake_type = cc->handshake_type; + if (req.timed) + tor_gettimeofday(&tv_start); + n = onion_skin_server_handshake(cc->handshake_type, + cc->onionskin, cc->handshake_len, + onion_keys, + cell_out->reply, + rpl.keys, CPATH_KEY_MATERIAL_LEN, + rpl.rend_auth_material); + if (n < 0) { + /* failure */ + log_debug(LD_OR,"onion_skin_server_handshake failed."); + memset(&rpl, 0, sizeof(rpl)); + rpl.success = 0; + } else { + /* success */ + log_debug(LD_OR,"onion_skin_server_handshake succeeded."); + cell_out->handshake_len = n; + switch (cc->cell_type) { + case CELL_CREATE: + cell_out->cell_type = CELL_CREATED; break; + case CELL_CREATE2: + cell_out->cell_type = CELL_CREATED2; break; + case CELL_CREATE_FAST: + cell_out->cell_type = CELL_CREATED_FAST; break; + default: + tor_assert(0); + return WQ_RPL_SHUTDOWN; + } + rpl.success = 1; + } + rpl.magic = CPUWORKER_REPLY_MAGIC; + if (req.timed) { + struct timeval tv_diff; + int64_t usec; + tor_gettimeofday(&tv_end); + timersub(&tv_end, &tv_start, &tv_diff); + usec = ((int64_t)tv_diff.tv_sec)*1000000 + tv_diff.tv_usec; + if (usec < 0 || usec > MAX_BELIEVABLE_ONIONSKIN_DELAY) + rpl.n_usec = MAX_BELIEVABLE_ONIONSKIN_DELAY; + else + rpl.n_usec = (uint32_t) usec; } memcpy(&job->u.reply, &rpl, sizeof(rpl)); @@ -499,58 +496,57 @@ assign_onionskin_to_cpuworker(or_circuit_t *circ, cpuworker_request_t req; int should_time; - if (1) { - if (!circ->p_chan) { - log_info(LD_OR,"circ->p_chan gone. Failing circ."); + if (!circ->p_chan) { + log_info(LD_OR,"circ->p_chan gone. Failing circ."); + tor_free(onionskin); + return -1; + } + + if (total_pending_tasks >= max_pending_tasks) { + log_debug(LD_OR,"No idle cpuworkers. Queuing."); + if (onion_pending_add(circ, onionskin) < 0) { tor_free(onionskin); return -1; } - - if (total_pending_tasks >= max_pending_tasks) { - log_debug(LD_OR,"No idle cpuworkers. Queuing."); - if (onion_pending_add(circ, onionskin) < 0) { - tor_free(onionskin); - return -1; - } - return 0; - } - - if (connection_or_digest_is_known_relay(circ->p_chan->identity_digest)) - rep_hist_note_circuit_handshake_assigned(onionskin->handshake_type); - - should_time = should_time_request(onionskin->handshake_type); - memset(&req, 0, sizeof(req)); - req.magic = CPUWORKER_REQUEST_MAGIC; - req.timed = should_time; - - memcpy(&req.create_cell, onionskin, sizeof(create_cell_t)); - - tor_free(onionskin); - - if (should_time) - tor_gettimeofday(&req.started_at); - - job = tor_malloc_zero(sizeof(cpuworker_job_t)); - job->chan_id = circ->p_chan->global_identifier; - job->circ_id = circ->p_circ_id; - memcpy(&job->u.request, &req, sizeof(req)); - memwipe(&req, 0, sizeof(req)); - - ++total_pending_tasks; - queue_entry = threadpool_queue_work(threadpool, - cpuworker_onion_handshake_threadfn, - cpuworker_onion_handshake_replyfn, - job); - if (!queue_entry) { - log_warn(LD_BUG, "Couldn't queue work on threadpool"); - tor_free(job); - return -1; - } - log_debug(LD_OR, "Queued task %p (qe=%p, chanid="U64_FORMAT", circid=%u)", - job, queue_entry, U64_PRINTF_ARG(job->chan_id), job->circ_id); - - circ->workqueue_entry = queue_entry; + return 0; } + + if (connection_or_digest_is_known_relay(circ->p_chan->identity_digest)) + rep_hist_note_circuit_handshake_assigned(onionskin->handshake_type); + + should_time = should_time_request(onionskin->handshake_type); + memset(&req, 0, sizeof(req)); + req.magic = CPUWORKER_REQUEST_MAGIC; + req.timed = should_time; + + memcpy(&req.create_cell, onionskin, sizeof(create_cell_t)); + + tor_free(onionskin); + + if (should_time) + tor_gettimeofday(&req.started_at); + + job = tor_malloc_zero(sizeof(cpuworker_job_t)); + job->chan_id = circ->p_chan->global_identifier; + job->circ_id = circ->p_circ_id; + memcpy(&job->u.request, &req, sizeof(req)); + memwipe(&req, 0, sizeof(req)); + + ++total_pending_tasks; + queue_entry = threadpool_queue_work(threadpool, + cpuworker_onion_handshake_threadfn, + cpuworker_onion_handshake_replyfn, + job); + if (!queue_entry) { + log_warn(LD_BUG, "Couldn't queue work on threadpool"); + tor_free(job); + return -1; + } + log_debug(LD_OR, "Queued task %p (qe=%p, chanid="U64_FORMAT", circid=%u)", + job, queue_entry, U64_PRINTF_ARG(job->chan_id), job->circ_id); + + circ->workqueue_entry = queue_entry; + return 0; } From fb5ebfb50770062c77534d4db4c6a9c5ad475fa0 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 2 Oct 2013 15:11:34 -0400 Subject: [PATCH 25/32] Avoid chan/circ linear lookups for requests The solution I took is to not free a circuit with a pending uncancellable work item, but rather to set its magic number to a sentinel value. When we get a work item, we check whether the circuit has that magic sentinel, and if so, we free it rather than processing the reply. --- src/or/circuitlist.c | 17 ++++++++++-- src/or/cpuworker.c | 62 +++++++++++++++++++------------------------- src/or/or.h | 6 +++++ 3 files changed, 47 insertions(+), 38 deletions(-) diff --git a/src/or/circuitlist.c b/src/or/circuitlist.c index 36ba3bffb7..d964e66922 100644 --- a/src/or/circuitlist.c +++ b/src/or/circuitlist.c @@ -745,6 +745,7 @@ circuit_free(circuit_t *circ) { void *mem; size_t memlen; + int should_free = 1; if (!circ) return; @@ -784,6 +785,8 @@ circuit_free(circuit_t *circ) memlen = sizeof(or_circuit_t); tor_assert(circ->magic == OR_CIRCUIT_MAGIC); + should_free = (ocirc->workqueue_entry == NULL); + crypto_cipher_free(ocirc->p_crypto); crypto_digest_free(ocirc->p_digest); crypto_cipher_free(ocirc->n_crypto); @@ -826,8 +829,18 @@ circuit_free(circuit_t *circ) * "active" checks will be violated. */ cell_queue_clear(&circ->n_chan_cells); - memwipe(mem, 0xAA, memlen); /* poison memory */ - tor_free(mem); + if (should_free) { + memwipe(mem, 0xAA, memlen); /* poison memory */ + tor_free(mem); + } else { + /* If we made it here, this is an or_circuit_t that still has a pending + * cpuworker request which we weren't able to cancel. Instead, set up + * the magic value so that when the reply comes back, we'll know to discard + * the reply and free this structure. + */ + memwipe(mem, 0xAA, memlen); + circ->magic = DEAD_CIRCUIT_MAGIC; + } } /** Deallocate the linked list circ->cpath, and remove the cpath from diff --git a/src/or/cpuworker.c b/src/or/cpuworker.c index abf1f2290c..36ca505fe3 100644 --- a/src/or/cpuworker.c +++ b/src/or/cpuworker.c @@ -152,8 +152,7 @@ typedef struct cpuworker_reply_t { } cpuworker_reply_t; typedef struct cpuworker_job_u { - uint64_t chan_id; - uint32_t circ_id; + or_circuit_t *circ; union { cpuworker_request_t request; cpuworker_reply_t reply; @@ -297,16 +296,13 @@ cpuworker_log_onionskin_overhead(int severity, int onionskin_type, onionskin_type_name, (unsigned)overhead, relative_overhead*100); } -/** */ +/** Handle a reply from the worker threads. */ static void cpuworker_onion_handshake_replyfn(void *work_) { cpuworker_job_t *job = work_; cpuworker_reply_t rpl; - uint64_t chan_id; - circid_t circ_id; - channel_t *p_chan = NULL; - circuit_t *circ = NULL; + or_circuit_t *circ = NULL; --total_pending_tasks; @@ -338,46 +334,40 @@ cpuworker_onion_handshake_replyfn(void *work_) } } } - /* Find the circ it was talking about */ - chan_id = job->chan_id; - circ_id = job->circ_id; - p_chan = channel_find_by_global_id(chan_id); - - if (p_chan) - circ = circuit_get_by_circid_channel(circ_id, p_chan); + circ = job->circ; log_debug(LD_OR, - "Unpacking cpuworker reply %p, chan_id is " U64_FORMAT - ", circ_id is %u, p_chan=%p, circ=%p, success=%d", - job, U64_PRINTF_ARG(chan_id), (unsigned)circ_id, - p_chan, circ, rpl.success); + "Unpacking cpuworker reply %p, circ=%p, success=%d", + job, circ, rpl.success); + + if (circ->base_.magic == DEAD_CIRCUIT_MAGIC) { + /* The circuit was supposed to get freed while the reply was + * pending. Instead, it got left for us to free so that we wouldn't freak + * out when the job->circ field wound up pointing to nothing. */ + log_debug(LD_OR, "Circuit died while reply was pending. Freeing memory."); + circ->base_.magic = 0; + tor_free(circ); + goto done_processing; + } + + circ->workqueue_entry = NULL; if (rpl.success == 0) { log_debug(LD_OR, "decoding onionskin failed. " "(Old key or bad software.) Closing."); if (circ) - circuit_mark_for_close(circ, END_CIRC_REASON_TORPROTOCOL); + circuit_mark_for_close(TO_CIRCUIT(circ), END_CIRC_REASON_TORPROTOCOL); goto done_processing; } - if (!circ) { - /* This happens because somebody sends us a destroy cell and the - * circuit goes away, while the cpuworker is working. This is also - * why our tag doesn't include a pointer to the circ, because we'd - * never know if it's still valid. - */ - log_debug(LD_OR,"processed onion for a circ that's gone. Dropping."); - goto done_processing; - } - tor_assert(! CIRCUIT_IS_ORIGIN(circ)); - TO_OR_CIRCUIT(circ)->workqueue_entry = NULL; - if (onionskin_answer(TO_OR_CIRCUIT(circ), + + if (onionskin_answer(circ, &rpl.created_cell, (const char*)rpl.keys, rpl.rend_auth_material) < 0) { log_warn(LD_OR,"onionskin_answer failed. Closing."); - circuit_mark_for_close(circ, END_CIRC_REASON_INTERNAL); + circuit_mark_for_close(TO_CIRCUIT(circ), END_CIRC_REASON_INTERNAL); goto done_processing; } log_debug(LD_OR,"onionskin_answer succeeded. Yay."); @@ -527,8 +517,7 @@ assign_onionskin_to_cpuworker(or_circuit_t *circ, tor_gettimeofday(&req.started_at); job = tor_malloc_zero(sizeof(cpuworker_job_t)); - job->chan_id = circ->p_chan->global_identifier; - job->circ_id = circ->p_circ_id; + job->circ = circ; memcpy(&job->u.request, &req, sizeof(req)); memwipe(&req, 0, sizeof(req)); @@ -542,8 +531,9 @@ assign_onionskin_to_cpuworker(or_circuit_t *circ, tor_free(job); return -1; } - log_debug(LD_OR, "Queued task %p (qe=%p, chanid="U64_FORMAT", circid=%u)", - job, queue_entry, U64_PRINTF_ARG(job->chan_id), job->circ_id); + + log_debug(LD_OR, "Queued task %p (qe=%p, circ=%p)", + job, queue_entry, job->circ); circ->workqueue_entry = queue_entry; diff --git a/src/or/or.h b/src/or/or.h index 4ff3555845..5978504c18 100644 --- a/src/or/or.h +++ b/src/or/or.h @@ -2725,8 +2725,14 @@ typedef struct { time_t expiry_time; } cpath_build_state_t; +/** "magic" value for an origin_circuit_t */ #define ORIGIN_CIRCUIT_MAGIC 0x35315243u +/** "magic" value for an or_circuit_t */ #define OR_CIRCUIT_MAGIC 0x98ABC04Fu +/** "magic" value for a circuit that would have been freed by circuit_free, + * but which we're keeping around until a cpuworker reply arrives. See + * circuit_free() for more documentation. */ +#define DEAD_CIRCUIT_MAGIC 0xdeadc14c struct create_cell_t; From 051ad788e0ebcd0c99c1498e7e45faa71c4830c1 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Mon, 16 Dec 2013 10:20:40 -0500 Subject: [PATCH 26/32] Incorporate some comments based on notes from dgoulet --- src/common/workqueue.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/common/workqueue.c b/src/common/workqueue.c index f3ef67891d..7fa8967580 100644 --- a/src/common/workqueue.c +++ b/src/common/workqueue.c @@ -69,9 +69,10 @@ typedef struct workerthread_s { tor_cond_t condition; /** Queue of pending work that we have to do. */ TOR_TAILQ_HEAD(, workqueue_entry_s) work; - /** True iff this thread is currently in its loop. */ + /** True iff this thread is currently in its loop. (Not currently used.) */ unsigned is_running; - /** True iff this thread has crashed or is shut down for some reason. */ + /** True iff this thread has crashed or is shut down for some reason. (Not + * currently used.) */ unsigned is_shut_down; /** True if we're waiting for more elements to get added to the queue. */ unsigned waiting; @@ -190,6 +191,7 @@ worker_thread_main(void *thread_) /* TODO: support an idle-function */ /* Okay. Now, wait till somebody has work for us. */ + /* XXXX we could just omit waiting and instead */ thread->waiting = 1; if (tor_cond_wait(&thread->condition, &thread->lock, NULL) < 0) { /* XXXX ERROR */ From a52e549124adb09ad0b49b7d2b5b3fb79bfe7aeb Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 14 Jan 2015 13:29:58 -0500 Subject: [PATCH 27/32] Update workqueue implementation to use a single queue for the work Previously I used one queue per worker; now I use one queue for everyone. The "broadcast" code is gone, replaced with an idempotent 'update' operation. --- src/common/compat_pthreads.c | 1 + src/common/workqueue.c | 204 +++++++++++++++++++++-------------- src/common/workqueue.h | 10 +- src/or/cpuworker.c | 15 +-- src/test/test_workqueue.c | 23 +--- 5 files changed, 140 insertions(+), 113 deletions(-) diff --git a/src/common/compat_pthreads.c b/src/common/compat_pthreads.c index 848bfe0973..f43480539f 100644 --- a/src/common/compat_pthreads.c +++ b/src/common/compat_pthreads.c @@ -5,6 +5,7 @@ #include "orconfig.h" #include +#include #include "compat.h" #include "torlog.h" diff --git a/src/common/workqueue.c b/src/common/workqueue.c index 7fa8967580..5ba29e3b26 100644 --- a/src/common/workqueue.c +++ b/src/common/workqueue.c @@ -1,4 +1,4 @@ -/* Copyright (c) 2013, The Tor Project, Inc. */ +/* Copyright (c) 2013-2015, The Tor Project, Inc. */ /* See LICENSE for licensing information */ #include "orconfig.h" @@ -13,8 +13,23 @@ struct threadpool_s { /** An array of pointers to workerthread_t: one for each running worker * thread. */ struct workerthread_s **threads; - /** Index of the next thread that we'll give work to.*/ - int next_for_work; + + /** Condition variable that we wait on when we have no work, and which + * gets signaled when our queue becomes nonempty. */ + tor_cond_t condition; + /** Queue of pending work that we have to do. */ + TOR_TAILQ_HEAD(, workqueue_entry_s) work; + + /** The current 'update generation' of the threadpool. Any thread that is + * at an earlier generation needs to run the update function. */ + unsigned generation; + + /** Function that should be run for updates on each thread. */ + int (*update_fn)(void *, void *); + /** Function to free update arguments if they can't be run. */ + void (*free_update_arg_fn)(void *); + /** Array of n_threads update arguments. */ + void **update_args; /** Number of elements in threads. */ int n_threads; @@ -34,10 +49,10 @@ struct workqueue_entry_s { /** The next workqueue_entry_t that's pending on the same thread or * reply queue. */ TOR_TAILQ_ENTRY(workqueue_entry_s) next_work; - /** The thread to which this workqueue_entry_t was assigned. This field + /** The threadpool to which this workqueue_entry_t was assigned. This field * is set when the workqueue_entry_t is created, and won't be cleared until * after it's handled in the main thread. */ - struct workerthread_s *on_thread; + struct threadpool_s *on_pool; /** True iff this entry is waiting for a worker to start processing it. */ uint8_t pending; /** Function to run in the worker thread. */ @@ -62,13 +77,10 @@ struct replyqueue_s { * contention, each gets its own queue. This breaks the guarantee that that * queued work will get executed strictly in order. */ typedef struct workerthread_s { - /** Lock to protect all fields of this thread and its queue. */ - tor_mutex_t lock; - /** Condition variable that we wait on when we have no work, and which - * gets signaled when our queue becomes nonempty. */ - tor_cond_t condition; - /** Queue of pending work that we have to do. */ - TOR_TAILQ_HEAD(, workqueue_entry_s) work; + /** Which thread it this? In range 0..in_pool->n_threads-1 */ + int index; + /** The pool this thread is a part of. */ + struct threadpool_s *in_pool; /** True iff this thread is currently in its loop. (Not currently used.) */ unsigned is_running; /** True iff this thread has crashed or is shut down for some reason. (Not @@ -81,6 +93,8 @@ typedef struct workerthread_s { void *state; /** Reply queue to which we pass our results. */ replyqueue_t *reply_queue; + /** The current update generation of this thread */ + unsigned generation; } workerthread_t; static void queue_reply(replyqueue_t *queue, workqueue_entry_t *work); @@ -132,13 +146,13 @@ workqueue_entry_cancel(workqueue_entry_t *ent) { int cancelled = 0; void *result = NULL; - tor_mutex_acquire(&ent->on_thread->lock); + tor_mutex_acquire(&ent->on_pool->lock); if (ent->pending) { - TOR_TAILQ_REMOVE(&ent->on_thread->work, ent, next_work); + TOR_TAILQ_REMOVE(&ent->on_pool->work, ent, next_work); cancelled = 1; result = ent->arg; } - tor_mutex_release(&ent->on_thread->lock); + tor_mutex_release(&ent->on_pool->lock); if (cancelled) { tor_free(ent); @@ -146,6 +160,16 @@ workqueue_entry_cancel(workqueue_entry_t *ent) return result; } +/**DOCDOC + + must hold lock */ +static int +worker_thread_has_work(workerthread_t *thread) +{ + return !TOR_TAILQ_EMPTY(&thread->in_pool->work) || + thread->generation != thread->in_pool->generation; +} + /** * Main function for the worker thread. */ @@ -153,20 +177,39 @@ static void worker_thread_main(void *thread_) { workerthread_t *thread = thread_; + threadpool_t *pool = thread->in_pool; workqueue_entry_t *work; int result; - tor_mutex_acquire(&thread->lock); thread->is_running = 1; + + tor_mutex_acquire(&pool->lock); while (1) { /* lock must be held at this point. */ - while (!TOR_TAILQ_EMPTY(&thread->work)) { + while (worker_thread_has_work(thread)) { /* lock must be held at this point. */ + if (thread->in_pool->generation != thread->generation) { + void *arg = thread->in_pool->update_args[thread->index]; + thread->in_pool->update_args[thread->index] = NULL; + int (*update_fn)(void*,void*) = thread->in_pool->update_fn; + thread->generation = thread->in_pool->generation; + tor_mutex_release(&pool->lock); - work = TOR_TAILQ_FIRST(&thread->work); - TOR_TAILQ_REMOVE(&thread->work, work, next_work); + int r = update_fn(thread->state, arg); + + if (r < 0) { + thread->is_running = 0; + thread->is_shut_down = 1; + return; + } + + tor_mutex_acquire(&pool->lock); + continue; + } + work = TOR_TAILQ_FIRST(&pool->work); + TOR_TAILQ_REMOVE(&pool->work, work, next_work); work->pending = 0; - tor_mutex_release(&thread->lock); + tor_mutex_release(&pool->lock); /* We run the work function without holding the thread lock. This * is the main thread's first opportunity to give us more work. */ @@ -175,25 +218,23 @@ worker_thread_main(void *thread_) /* Queue the reply for the main thread. */ queue_reply(thread->reply_queue, work); - tor_mutex_acquire(&thread->lock); /* We may need to exit the thread. */ if (result >= WQ_RPL_ERROR) { thread->is_running = 0; thread->is_shut_down = 1; - tor_mutex_release(&thread->lock); return; } + tor_mutex_acquire(&pool->lock); } /* At this point the lock is held, and there is no work in this thread's * queue. */ - /* TODO: Try work-stealing. */ /* TODO: support an idle-function */ /* Okay. Now, wait till somebody has work for us. */ /* XXXX we could just omit waiting and instead */ thread->waiting = 1; - if (tor_cond_wait(&thread->condition, &thread->lock, NULL) < 0) { + if (tor_cond_wait(&pool->condition, &pool->lock, NULL) < 0) { /* XXXX ERROR */ } thread->waiting = 0; @@ -221,14 +262,12 @@ queue_reply(replyqueue_t *queue, workqueue_entry_t *work) /** Allocate and start a new worker thread to use state object state, * and send responses to replyqueue. */ static workerthread_t * -workerthread_new(void *state, replyqueue_t *replyqueue) +workerthread_new(void *state, threadpool_t *pool, replyqueue_t *replyqueue) { workerthread_t *thr = tor_malloc_zero(sizeof(workerthread_t)); - tor_mutex_init_for_cond(&thr->lock); - tor_cond_init(&thr->condition); - TOR_TAILQ_INIT(&thr->work); thr->state = state; thr->reply_queue = replyqueue; + thr->in_pool = pool; if (spawn_func(worker_thread_main, thr) < 0) { log_err(LD_GENERAL, "Can't launch worker thread."); @@ -238,30 +277,6 @@ workerthread_new(void *state, replyqueue_t *replyqueue) return thr; } -/** - * Add an item of work to a single worker thread. See threadpool_queue_work(*) - * for arguments. - */ -static workqueue_entry_t * -workerthread_queue_work(workerthread_t *worker, - int (*fn)(void *, void *), - void (*reply_fn)(void *), - void *arg) -{ - workqueue_entry_t *ent = workqueue_entry_new(fn, reply_fn, arg); - - tor_mutex_acquire(&worker->lock); - ent->on_thread = worker; - ent->pending = 1; - TOR_TAILQ_INSERT_TAIL(&worker->work, ent, next_work); - - if (worker->waiting) /* XXXX inside or outside of lock?? */ - tor_cond_signal_one(&worker->condition); - - tor_mutex_release(&worker->lock); - return ent; -} - /** * Queue an item of work for a thread in a thread pool. The function * fn will be run in a worker thread, and will receive as arguments the @@ -285,20 +300,19 @@ threadpool_queue_work(threadpool_t *pool, void (*reply_fn)(void *), void *arg) { - workerthread_t *worker; + workqueue_entry_t *ent = workqueue_entry_new(fn, reply_fn, arg); + ent->on_pool = pool; + ent->pending = 1; tor_mutex_acquire(&pool->lock); - /* Pick the next thread in random-access order. */ - worker = pool->threads[pool->next_for_work++]; - if (!worker) { - tor_mutex_release(&pool->lock); - return NULL; - } - if (pool->next_for_work >= pool->n_threads) - pool->next_for_work = 0; + + TOR_TAILQ_INSERT_TAIL(&pool->work, ent, next_work); + tor_mutex_release(&pool->lock); - return workerthread_queue_work(worker, fn, reply_fn, arg); + tor_cond_signal_one(&pool->condition); + + return ent; } /** @@ -309,30 +323,56 @@ threadpool_queue_work(threadpool_t *pool, * arg value is passed to dup_fn once per each thread to * make a copy of it. * + * UPDATE FUNCTIONS MUST BE IDEMPOTENT. We do not guarantee that every update + * will be run. If a new update is scheduled before the old update finishes + * running, then the new will replace the old in any threads that haven't run + * it yet. + * * Return 0 on success, -1 on failure. */ int -threadpool_queue_for_all(threadpool_t *pool, +threadpool_queue_update(threadpool_t *pool, void *(*dup_fn)(void *), int (*fn)(void *, void *), - void (*reply_fn)(void *), + void (*free_fn)(void *), void *arg) { - int i = 0; - workerthread_t *worker; - void *arg_copy; - while (1) { - tor_mutex_acquire(&pool->lock); - if (i >= pool->n_threads) { - tor_mutex_release(&pool->lock); - return 0; - } - worker = pool->threads[i++]; - tor_mutex_release(&pool->lock); + int i, n_threads; + void (*old_args_free_fn)(void *arg); + void **old_args; + void **new_args; - arg_copy = dup_fn ? dup_fn(arg) : arg; - /* CHECK*/ workerthread_queue_work(worker, fn, reply_fn, arg_copy); + tor_mutex_acquire(&pool->lock); + n_threads = pool->n_threads; + old_args = pool->update_args; + old_args_free_fn = pool->free_update_arg_fn; + + new_args = tor_calloc(n_threads, sizeof(void*)); + for (i = 0; i < n_threads; ++i) { + if (dup_fn) + new_args[i] = dup_fn(arg); + else + new_args[i] = arg; } + + pool->update_args = new_args; + pool->free_update_arg_fn = free_fn; + pool->update_fn = fn; + ++pool->generation; + + tor_mutex_release(&pool->lock); + + tor_cond_signal_all(&pool->condition); + + if (old_args) { + for (i = 0; i < n_threads; ++i) { + if (old_args[i] && old_args_free_fn) + old_args_free_fn(old_args[i]); + } + tor_free(old_args); + } + + return 0; } /** Launch threads until we have n. */ @@ -346,7 +386,8 @@ threadpool_start_threads(threadpool_t *pool, int n) while (pool->n_threads < n) { void *state = pool->new_thread_state_fn(pool->new_thread_state_arg); - workerthread_t *thr = workerthread_new(state, pool->reply_queue); + workerthread_t *thr = workerthread_new(state, pool, pool->reply_queue); + thr->index = pool->n_threads; if (!thr) { tor_mutex_release(&pool->lock); @@ -375,7 +416,10 @@ threadpool_new(int n_threads, { threadpool_t *pool; pool = tor_malloc_zero(sizeof(threadpool_t)); - tor_mutex_init(&pool->lock); + tor_mutex_init_nonrecursive(&pool->lock); + tor_cond_init(&pool->condition); + TOR_TAILQ_INIT(&pool->work); + pool->new_thread_state_fn = new_thread_state_fn; pool->new_thread_state_arg = arg; pool->free_thread_state_fn = free_thread_state_fn; @@ -447,7 +491,7 @@ replyqueue_process(replyqueue_t *queue) workqueue_entry_t *work = TOR_TAILQ_FIRST(&queue->answers); TOR_TAILQ_REMOVE(&queue->answers, work, next_work); tor_mutex_release(&queue->lock); - work->on_thread = NULL; + work->on_pool = NULL; work->reply_fn(work->arg); workqueue_entry_free(work); diff --git a/src/common/workqueue.h b/src/common/workqueue.h index aa1bcc518a..92e82b8a48 100644 --- a/src/common/workqueue.h +++ b/src/common/workqueue.h @@ -27,11 +27,11 @@ workqueue_entry_t *threadpool_queue_work(threadpool_t *pool, int (*fn)(void *, void *), void (*reply_fn)(void *), void *arg); -int threadpool_queue_for_all(threadpool_t *pool, - void *(*dup_fn)(void *), - int (*fn)(void *, void *), - void (*reply_fn)(void *), - void *arg); +int threadpool_queue_update(threadpool_t *pool, + void *(*dup_fn)(void *), + int (*fn)(void *, void *), + void (*free_fn)(void *), + void *arg); void *workqueue_entry_cancel(workqueue_entry_t *pending_work); threadpool_t *threadpool_new(int n_threads, replyqueue_t *replyqueue, diff --git a/src/or/cpuworker.c b/src/or/cpuworker.c index 36ca505fe3..3f129ded99 100644 --- a/src/or/cpuworker.c +++ b/src/or/cpuworker.c @@ -170,11 +170,6 @@ update_state_threadfn(void *state_, void *work_) ++state->generation; return WQ_RPL_REPLY; } -static void -update_state_replyfn(void *work_) -{ - tor_free(work_); -} /** Called when the onion key has changed and we need to spawn new * cpuworkers. Close all currently idle cpuworkers, and mark the last @@ -183,11 +178,11 @@ update_state_replyfn(void *work_) void cpuworkers_rotate_keyinfo(void) { - if (threadpool_queue_for_all(threadpool, - worker_state_new, - update_state_threadfn, - update_state_replyfn, - NULL)) { + if (threadpool_queue_update(threadpool, + worker_state_new, + update_state_threadfn, + worker_state_free, + NULL)) { log_warn(LD_OR, "Failed to queue key update for worker threads."); } } diff --git a/src/test/test_workqueue.c b/src/test/test_workqueue.c index 410f43cce4..8ce4405062 100644 --- a/src/test/test_workqueue.c +++ b/src/test/test_workqueue.c @@ -132,7 +132,6 @@ new_state(void *arg) /* Every thread gets its own keys. not a problem for benchmarking */ st->rsa = crypto_pk_new(); if (crypto_pk_generate_key_with_bits(st->rsa, 1024) < 0) { - puts("keygen failed"); crypto_pk_free(st->rsa); tor_free(st); return NULL; @@ -213,7 +212,6 @@ add_n_work_items(threadpool_t *tp, int n) while (n_queued++ < n) { ent = add_work(tp); if (! ent) { - puts("Couldn't add work."); tor_event_base_loopexit(tor_libevent_get_base(), NULL); return -1; } @@ -238,18 +236,6 @@ add_n_work_items(threadpool_t *tp, int n) } static int shutting_down = 0; -static int n_shutdowns_done = 0; - -static void -shutdown_reply(void *arg) -{ - (void)arg; - tor_assert(shutting_down); - ++n_shutdowns_done; - if (n_shutdowns_done == opt_n_threads) { - tor_event_base_loopexit(tor_libevent_get_base(), NULL); - } -} static void replysock_readable_cb(tor_socket_t sock, short what, void *arg) @@ -297,8 +283,8 @@ replysock_readable_cb(tor_socket_t sock, short what, void *arg) n_received+n_successful_cancel == n_sent && n_sent >= opt_n_items) { shutting_down = 1; - threadpool_queue_for_all(tp, NULL, - workqueue_do_shutdown, shutdown_reply, NULL); + threadpool_queue_update(tp, NULL, + workqueue_do_shutdown, NULL, NULL); } } @@ -410,8 +396,9 @@ main(int argc, char **argv) event_base_loop(tor_libevent_get_base(), 0); - if (n_sent != opt_n_items || n_received+n_successful_cancel != n_sent || - n_shutdowns_done != opt_n_threads) { + if (n_sent != opt_n_items || n_received+n_successful_cancel != n_sent) { + printf("%d vs %d\n", n_sent, opt_n_items); + printf("%d+%d vs %d\n", n_received, n_successful_cancel, n_sent); puts("FAIL"); return 1; } else { From ac5b70c700b211008853b5f212100a867f508dfd Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 21 Jan 2015 12:18:11 -0500 Subject: [PATCH 28/32] handle EINTR in compat_*threads.c --- src/common/compat_pthreads.c | 40 +++++++++++++++++--------- src/common/compat_threads.c | 55 +++++++++++++++++++++++++++++++++--- 2 files changed, 78 insertions(+), 17 deletions(-) diff --git a/src/common/compat_pthreads.c b/src/common/compat_pthreads.c index f43480539f..188a91f68d 100644 --- a/src/common/compat_pthreads.c +++ b/src/common/compat_pthreads.c @@ -196,23 +196,37 @@ tor_cond_uninit(tor_cond_t *cond) int tor_cond_wait(tor_cond_t *cond, tor_mutex_t *mutex, const struct timeval *tv) { + int r; if (tv == NULL) { - return pthread_cond_wait(&cond->cond, &mutex->mutex) ? -1 : 0; + while (1) { + r = pthread_cond_wait(&cond->cond, &mutex->mutex); + if (r == EINTR) { + /* EINTR should be impossible according to POSIX, but POSIX, like the + * Pirate's Code, is apparently treated "more like what you'd call + * guidelines than actual rules." */ + continue; + } + return r ? -1 : 0; + } } else { struct timespec ts; struct timeval tvnow, tvsum; - int r; - gettimeofday(&tvnow, NULL); - timeradd(tv, &tvnow, &tvsum); - ts.tv_sec = tvsum.tv_sec; - ts.tv_nsec = tvsum.tv_usec * 1000; - r = pthread_cond_timedwait(&cond->cond, &mutex->mutex, &ts); - if (r == 0) - return 0; - else if (r == ETIMEDOUT) - return 1; - else - return -1; + while (1) { + gettimeofday(&tvnow, NULL); + timeradd(tv, &tvnow, &tvsum); + ts.tv_sec = tvsum.tv_sec; + ts.tv_nsec = tvsum.tv_usec * 1000; + + r = pthread_cond_timedwait(&cond->cond, &mutex->mutex, &ts); + if (r == 0) + return 0; + else if (r == ETIMEDOUT) + return 1; + else if (r == EINTR) + continue; + else + return -1; + } } } /** Wake up one of the waiters on cond. */ diff --git a/src/common/compat_threads.c b/src/common/compat_threads.c index 79440070a2..3b79292cdb 100644 --- a/src/common/compat_threads.c +++ b/src/common/compat_threads.c @@ -88,12 +88,59 @@ in_main_thread(void) return main_thread_id == tor_get_thread_id(); } +#if defined(HAVE_EVENTFD) || defined(HAVE_PIPE) +/* non-interruptable versions */ +static int +write_ni(int fd, const void *buf, size_t n) +{ + int r; + again: + r = write(fd, buf, n); + if (r < 0 && errno == EINTR) + goto again; + return r; +} +static int +read_ni(int fd, void *buf, size_t n) +{ + int r; + again: + r = read(fd, buf, n); + if (r < 0 && errno == EINTR) + goto again; + return r; +} +#endif + +/* non-interruptable versions */ +static int +send_ni(int fd, const void *buf, size_t n, int flags) +{ + int r; + again: + r = send(fd, buf, n, flags); + if (r < 0 && errno == EINTR) + goto again; + return r; +} + +static int +recv_ni(int fd, void *buf, size_t n, int flags) +{ + int r; + again: + r = recv(fd, buf, n, flags); + if (r < 0 && errno == EINTR) + goto again; + return r; +} + #ifdef HAVE_EVENTFD static int eventfd_alert(int fd) { uint64_t u = 1; - int r = write(fd, (void*)&u, sizeof(u)); + int r = write_ni(fd, (void*)&u, sizeof(u)); if (r < 0 && errno != EAGAIN) return -1; return 0; @@ -103,7 +150,7 @@ static int eventfd_drain(int fd) { uint64_t u = 0; - int r = read(fd, (void*)&u, sizeof(u)); + int r = read_ni(fd, (void*)&u, sizeof(u)); if (r < 0 && errno != EAGAIN) return -1; return 0; @@ -136,7 +183,7 @@ pipe_drain(int fd) static int sock_alert(tor_socket_t fd) { - ssize_t r = send(fd, "x", 1, 0); + ssize_t r = send_ni(fd, "x", 1, 0); if (r < 0 && !ERRNO_IS_EAGAIN(tor_socket_errno(fd))) return -1; return 0; @@ -147,7 +194,7 @@ sock_drain(tor_socket_t fd) { char buf[32]; ssize_t r; - while ((r = recv(fd, buf, sizeof(buf), 0)) >= 0) + while ((r = recv_ni(fd, buf, sizeof(buf), 0)) >= 0) ; if (r == 0 || !ERRNO_IS_EAGAIN(tor_socket_errno(fd))) return -1; From 3c8dabf69aa950c2df49f48aebbe02aac5b519f3 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 21 Jan 2015 12:22:41 -0500 Subject: [PATCH 29/32] Fix up some workqueue/threading issues spotted by dgoulet. --- src/common/compat_pthreads.c | 3 ++- src/common/workqueue.c | 24 +++++------------------- 2 files changed, 7 insertions(+), 20 deletions(-) diff --git a/src/common/compat_pthreads.c b/src/common/compat_pthreads.c index 188a91f68d..c217c5145b 100644 --- a/src/common/compat_pthreads.c +++ b/src/common/compat_pthreads.c @@ -212,7 +212,8 @@ tor_cond_wait(tor_cond_t *cond, tor_mutex_t *mutex, const struct timeval *tv) struct timespec ts; struct timeval tvnow, tvsum; while (1) { - gettimeofday(&tvnow, NULL); + if (gettimeofday(&tvnow, NULL) < 0) + return -1; timeradd(tv, &tvnow, &tvsum); ts.tv_sec = tvsum.tv_sec; ts.tv_nsec = tvsum.tv_usec * 1000; diff --git a/src/common/workqueue.c b/src/common/workqueue.c index 5ba29e3b26..77a4fbc3f6 100644 --- a/src/common/workqueue.c +++ b/src/common/workqueue.c @@ -1,4 +1,4 @@ -/* Copyright (c) 2013-2015, The Tor Project, Inc. */ +/* copyright (c) 2013-2015, The Tor Project, Inc. */ /* See LICENSE for licensing information */ #include "orconfig.h" @@ -81,13 +81,6 @@ typedef struct workerthread_s { int index; /** The pool this thread is a part of. */ struct threadpool_s *in_pool; - /** True iff this thread is currently in its loop. (Not currently used.) */ - unsigned is_running; - /** True iff this thread has crashed or is shut down for some reason. (Not - * currently used.) */ - unsigned is_shut_down; - /** True if we're waiting for more elements to get added to the queue. */ - unsigned waiting; /** User-supplied state field that we pass to the worker functions of each * work item. */ void *state; @@ -181,8 +174,6 @@ worker_thread_main(void *thread_) workqueue_entry_t *work; int result; - thread->is_running = 1; - tor_mutex_acquire(&pool->lock); while (1) { /* lock must be held at this point. */ @@ -198,8 +189,6 @@ worker_thread_main(void *thread_) int r = update_fn(thread->state, arg); if (r < 0) { - thread->is_running = 0; - thread->is_shut_down = 1; return; } @@ -220,8 +209,6 @@ worker_thread_main(void *thread_) /* We may need to exit the thread. */ if (result >= WQ_RPL_ERROR) { - thread->is_running = 0; - thread->is_shut_down = 1; return; } tor_mutex_acquire(&pool->lock); @@ -232,12 +219,9 @@ worker_thread_main(void *thread_) /* TODO: support an idle-function */ /* Okay. Now, wait till somebody has work for us. */ - /* XXXX we could just omit waiting and instead */ - thread->waiting = 1; if (tor_cond_wait(&pool->condition, &pool->lock, NULL) < 0) { - /* XXXX ERROR */ + log_warn(LD_GENERAL, "Fail tor_cond_wait."); } - thread->waiting = 0; } } @@ -482,7 +466,9 @@ void replyqueue_process(replyqueue_t *queue) { if (queue->alert.drain_fn(queue->alert.read_fd) < 0) { - /* XXXX complain! */ + static ratelim_t warn_limit = RATELIM_INIT(7200); + log_fn_ratelim(&warn_limit, LOG_WARN, LD_GENERAL, + "Failure from drain_fd"); } tor_mutex_acquire(&queue->lock); From d684dbb0c76c4de0b21fabee99fc08833249bd87 Mon Sep 17 00:00:00 2001 From: David Goulet Date: Wed, 21 Jan 2015 13:18:56 -0500 Subject: [PATCH 30/32] Support monotonic time for pthread_cond_timedwait This is to avoid that the pthread_cond_timedwait() is not affected by time adjustment which could make the waiting period very long or very short which is not what we want in any cases. Signed-off-by: David Goulet --- src/common/compat_pthreads.c | 30 ++++++++++++++++++++++++++++-- 1 file changed, 28 insertions(+), 2 deletions(-) diff --git a/src/common/compat_pthreads.c b/src/common/compat_pthreads.c index c217c5145b..e1d4b0e79b 100644 --- a/src/common/compat_pthreads.c +++ b/src/common/compat_pthreads.c @@ -6,6 +6,7 @@ #include "orconfig.h" #include #include +#include #include "compat.h" #include "torlog.h" @@ -169,8 +170,23 @@ tor_get_thread_id(void) int tor_cond_init(tor_cond_t *cond) { + pthread_condattr_t condattr; + memset(cond, 0, sizeof(tor_cond_t)); - if (pthread_cond_init(&cond->cond, NULL)) { + /* Default condition attribute. Might be used if clock monotonic is + * available else this won't affect anything. */ + if (pthread_condattr_init(&condattr)) { + return -1; + } + +#if defined(HAVE_CLOCK_GETTIME) && defined(CLOCK_MONOTONIC) + /* Use monotonic time so when we timedwait() on it, any clock adjustment + * won't affect the timeout value. */ + if (pthread_condattr_setclock(&condattr, CLOCK_MONOTONIC)) { + return -1; + } +#endif + if (pthread_cond_init(&cond->cond, &condattr)) { return -1; } return 0; @@ -209,12 +225,22 @@ tor_cond_wait(tor_cond_t *cond, tor_mutex_t *mutex, const struct timeval *tv) return r ? -1 : 0; } } else { - struct timespec ts; struct timeval tvnow, tvsum; + struct timespec ts; while (1) { +#if defined(HAVE_CLOCK_GETTIME) && defined(CLOCK_MONOTONIC) + if (clock_gettime(CLOCK_MONOTONIC, &ts) < 0) { + return -1; + } + tvnow.tv_sec = ts.tv_sec; + tvnow.tv_usec = ts.tv_nsec / 1000; + timeradd(tv, &tvnow, &tvsum); +#else if (gettimeofday(&tvnow, NULL) < 0) return -1; timeradd(tv, &tvnow, &tvsum); +#endif /* HAVE_CLOCK_GETTIME, CLOCK_MONOTONIC */ + ts.tv_sec = tvsum.tv_sec; ts.tv_nsec = tvsum.tv_usec * 1000; From f52ac5be74b3cb6f657a6d7a1fa7db2c9595728d Mon Sep 17 00:00:00 2001 From: David Goulet Date: Wed, 21 Jan 2015 13:58:18 -0500 Subject: [PATCH 31/32] Fix: change copyright year in workqueue and thread tests Signed-off-by: David Goulet --- src/test/test_threads.c | 2 +- src/test/test_workqueue.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/test/test_threads.c b/src/test/test_threads.c index c0293048fe..2ac08d4d28 100644 --- a/src/test/test_threads.c +++ b/src/test/test_threads.c @@ -1,6 +1,6 @@ /* Copyright (c) 2001-2004, Roger Dingledine. * Copyright (c) 2004-2006, Roger Dingledine, Nick Mathewson. - * Copyright (c) 2007-2013, The Tor Project, Inc. */ + * Copyright (c) 2007-2015, The Tor Project, Inc. */ /* See LICENSE for licensing information */ #include "orconfig.h" diff --git a/src/test/test_workqueue.c b/src/test/test_workqueue.c index 8ce4405062..aaff5069be 100644 --- a/src/test/test_workqueue.c +++ b/src/test/test_workqueue.c @@ -1,6 +1,6 @@ /* Copyright (c) 2001-2004, Roger Dingledine. * Copyright (c) 2004-2006, Roger Dingledine, Nick Mathewson. - * Copyright (c) 2007-2013, The Tor Project, Inc. */ + * Copyright (c) 2007-2015, The Tor Project, Inc. */ /* See LICENSE for licensing information */ #include "or.h" From 84f5cb749d614deeb66f9032c54cd9885e300493 Mon Sep 17 00:00:00 2001 From: David Goulet Date: Wed, 21 Jan 2015 14:29:03 -0500 Subject: [PATCH 32/32] Fix: remove whitespace and update a comment in cpuworker.c Signed-off-by: David Goulet --- src/or/cpuworker.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/or/cpuworker.c b/src/or/cpuworker.c index 3f129ded99..39d2079994 100644 --- a/src/or/cpuworker.c +++ b/src/or/cpuworker.c @@ -171,9 +171,8 @@ update_state_threadfn(void *state_, void *work_) return WQ_RPL_REPLY; } -/** Called when the onion key has changed and we need to spawn new - * cpuworkers. Close all currently idle cpuworkers, and mark the last - * rotation time as now. +/** Called when the onion key has changed so update all CPU worker(s) with + * new function pointers with which a new state will be generated. */ void cpuworkers_rotate_keyinfo(void) @@ -336,7 +335,7 @@ cpuworker_onion_handshake_replyfn(void *work_) "Unpacking cpuworker reply %p, circ=%p, success=%d", job, circ, rpl.success); - if (circ->base_.magic == DEAD_CIRCUIT_MAGIC) { + if (circ->base_.magic == DEAD_CIRCUIT_MAGIC) { /* The circuit was supposed to get freed while the reply was * pending. Instead, it got left for us to free so that we wouldn't freak * out when the job->circ field wound up pointing to nothing. */