From: Wouter Wijngaards Date: Thu, 8 Mar 2007 14:59:41 +0000 (+0000) Subject: review of checklocks. X-Git-Tag: release-0.1~5 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=78ab5f41d716ea4a17d0118a5cd2c486f8992ede;p=thirdparty%2Funbound.git review of checklocks. git-svn-id: file:///svn/unbound/trunk@166 be551aaa-1e26-0410-a405-d3ace91eadb9 --- diff --git a/doc/Changelog b/doc/Changelog index 19cd2f1d0..786549ca9 100644 --- a/doc/Changelog +++ b/doc/Changelog @@ -1,3 +1,6 @@ +8 March 2007: Wouter + - Reviewed checklock code. + 7 March 2007: Wouter - created a wrapper around thread calls that performs some basic checking for data race and deadlock, and basic performance diff --git a/testcode/checklocks.c b/testcode/checklocks.c index 917e30395..4d420f7fd 100644 --- a/testcode/checklocks.c +++ b/testcode/checklocks.c @@ -42,11 +42,12 @@ * \file * Locks that are checked. * - * Ugly hack: uses the fact that workers are passed to thread_create to make - * the thread numbers here the same as those used for logging which is nice. + * Ugly hack: uses the fact that workers start with an int thread_num, and + * are passed to thread_create to make the thread numbers here the same as + * those used for logging which is nice. * * Todo: - check global ordering of instances of locks. - * - refcount statistics. + * - refcount statistics. * - debug status print, of thread lock stacks, and current waiting. */ #ifdef USE_THREAD_DEBUG @@ -55,7 +56,7 @@ static int key_created = 0; /** we hide the thread debug info with this key. */ static ub_thread_key_t thr_debug_key; -/** the list of threads, so all threads can be examined. NULL at start. */ +/** the list of threads, so all threads can be examined. NULL if unused. */ static struct thr_check* thread_infos[THRDEBUG_MAX_THREADS]; /** print pretty lock error and exit */ @@ -63,17 +64,20 @@ static void lock_error(struct checked_lock* lock, const char* func, const char* file, int line, const char* err) { log_err("lock error (description follows)"); - log_err("Created at %s %s %d", lock->create_func, lock->create_file, lock->create_line); - log_err("Previously %s %s %d", lock->holder_func, lock->holder_file, lock->holder_line); - log_err("At %s %s %d", func, file, line); + log_err("Created at %s %s:%d", lock->create_func, + lock->create_file, lock->create_line); + log_err("Previously %s %s:%d", lock->holder_func, + lock->holder_file, lock->holder_line); + log_err("At %s %s:%d", func, file, line); log_err("Error for %s lock: %s", (lock->type==check_lock_mutex)?"mutex": ( (lock->type==check_lock_spinlock)?"spinlock": "rwlock"), err); fatal_exit("bailing out"); } -/** obtain lock on debug lock structure. This could be a deadlock. - * (could it?) Anyway, check with timeouts. +/** + * Obtain lock on debug lock structure. This could be a deadlock by the caller. + * The debug code itself does not deadlock. Anyway, check with timeouts. * @param lock: on what to acquire lock. * @param func: user level caller identification. * @param file: user level caller identification. @@ -101,6 +105,7 @@ acquire_locklock(struct checked_lock* lock, log_err("in acquiring locklock: %s", strerror(err)); lock_error(lock, func, file, line, "acquire locklock"); } + /* since we hold the lock, we can edit the contention_count */ lock->contention_count += contend; } @@ -108,13 +113,13 @@ acquire_locklock(struct checked_lock* lock, void lock_protect(struct checked_lock* lock, void* area, size_t size) { - struct protected_area* e = (struct protected_area*)calloc(1, + struct protected_area* e = (struct protected_area*)malloc( sizeof(struct protected_area)); if(!e) fatal_exit("lock_protect: out of memory"); e->region = area; e->size = size; - e->hold = calloc(1, size); + e->hold = malloc(size); if(!e->hold) fatal_exit("lock_protect: out of memory"); memcpy(e->hold, e->region, e->size); @@ -189,7 +194,7 @@ checklock_init(enum check_lock_type type, struct checked_lock** lock, } /** delete prot items */ -static void prot_delete(struct checked_lock* lock) +static void prot_clear(struct checked_lock* lock) { struct protected_area* p=lock->prot, *np; while(p) { @@ -222,11 +227,11 @@ checklock_destroy(enum check_lock_type type, struct checked_lock** lock, e = *lock; if(!e) return; - *lock = NULL; /* use after free will fail */ checktype(type, e, func, file, line); /* check if delete is OK */ acquire_locklock(e, func, file, line); + *lock = NULL; /* use after free will fail */ if(e->hold_count != 0) lock_error(e, func, file, line, "delete while locked."); if(e->wait_count != 0) @@ -243,8 +248,9 @@ checklock_destroy(enum check_lock_type type, struct checked_lock** lock, /* delete it */ LOCKRET(pthread_mutex_destroy(&e->lock)); - prot_delete(e); - /* since nobody holds the lock - see check above, no need to unlink */ + prot_clear(e); + /* since nobody holds the lock - see check above, no need to unlink + * from the thread-held locks list. */ switch(e->type) { case check_lock_mutex: LOCKRET(pthread_mutex_destroy(&e->mutex)); @@ -258,7 +264,7 @@ checklock_destroy(enum check_lock_type type, struct checked_lock** lock, default: log_assert(0); } - memset(e, 0, sizeof(*lock)); + memset(e, 0, sizeof(struct checked_lock)); free(e); } @@ -329,7 +335,7 @@ checklock_lockit(enum check_lock_type type, struct checked_lock* lock, if((err=timedfunc(arg, &to))) { if(err == ETIMEDOUT) lock_error(lock, func, file, line, - "timeout, deadlock?"); + "timeout possible deadlock"); log_err("timedlock: %s", strerror(err)); } contend ++; @@ -458,7 +464,7 @@ checklock_unlock(enum check_lock_type type, struct checked_lock* lock, /* delete from thread holder list */ /* no need to lock other lockstructs, because they are all on the - * held-locks list, and this threads holds their locks. + * held-locks list, and this thread holds their locks. * we only touch the thr->num members, so it is safe. */ if(thr->holding_first == lock) thr->holding_first = lock->next_held_lock[thr->num]; diff --git a/testcode/checklocks.h b/testcode/checklocks.h index af14fc06e..ed0a29af3 100644 --- a/testcode/checklocks.h +++ b/testcode/checklocks.h @@ -50,6 +50,7 @@ * memcmp'ed to ascertain no race conditions. * o checks that locks are unlocked properly (before deletion). * keeps which func, file, line that locked it. + * o checks deadlocks with timeout so it can print errors for them. * * Limitations: * o Detects unprotected memory access when the lock is locked or freed, @@ -59,13 +60,13 @@ * o Does not check order of locking. * o Uses a lot of memory. * o The checks use locks themselves, changing scheduling, - * thus affecting the races that you see. + * thus changing what races you see. * o for rwlocks does not detect exclusive writelock, or double locking. */ #ifdef USE_THREAD_DEBUG #ifndef HAVE_PTHREAD -/* really pretty arbitrary, since it will work with solaris threads too */ +/* we need the *timed*lock() routines to use for deadlock detection. */ #error "Need pthreads for checked locks" #endif /******************* THREAD DEBUG ************************/ @@ -75,7 +76,7 @@ #define CHECK_LOCK_TIMEOUT 5 /* seconds */ /** How long to wait before join attempt is a failure. */ #define CHECK_JOIN_TIMEOUT 120 /* seconds */ -/** How many trheads to allocate for */ +/** How many threads to allocate for */ #define THRDEBUG_MAX_THREADS 32 /* threads */ /** @@ -95,7 +96,7 @@ struct protected_area { }; /** - * per thread information for locking debug wrappers. + * Per thread information for locking debug wrappers. */ struct thr_check { /** thread id */ @@ -107,9 +108,9 @@ struct thr_check { /** number of thread in list structure */ int num; /** - * list of locks that this thread is holding, double - * linked list, which first element the most recent lock acquired. - * So a represents the stack of locks acquired. (of all types). + * List of locks that this thread is holding, double + * linked list. The first element is the most recent lock acquired. + * So it represents the stack of locks acquired. (of all types). */ struct checked_lock *holding_first, *holding_last; /** if the thread is currently waiting for a lock, which one */ @@ -169,9 +170,13 @@ struct checked_lock { /** * Additional call for the user to specify what areas are protected * @param lock: the lock that protects the area. It can be inside the area. + * The lock must be inited. * @param area: ptr to mem. * @param size: length of area. * You can call it multiple times with the same lock to give several areas. + * Call it when you are done initialising the area, since it will be copied + * at this time and protected right away against unauthorised changes until + * the next lock() call is done. */ void lock_protect(struct checked_lock* lock, void* area, size_t size); @@ -259,6 +264,8 @@ void checklock_thrjoin(pthread_t thread); /** structures to enable compiler type checking on the locks. * Also the pointer makes it so that the lock can be part of the protected * region without any possible problem (since the ptr will stay the same.) + * i.e. there can be contention and readlocks stored in checked_lock, while + * the protected area stays the same, even though it contains (ptr to) lock. */ struct checked_lock_rw { struct checked_lock* c_rw; }; /** structures to enable compiler type checking on the locks. */