review of checklocks.

git-svn-id: file:///svn/unbound/trunk@166 be551aaa-1e26-0410-a405-d3ace91eadb9

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 

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];

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. */