[asterisk-commits] dhubbard: branch group/taskprocessors r115261 - in /team/group/taskprocessors...

SVN commits to the Asterisk project asterisk-commits at lists.digium.com
Fri May 2 17:33:23 CDT 2008


Author: dhubbard
Date: Fri May  2 17:33:23 2008
New Revision: 115261

URL: http://svn.digium.com/view/asterisk?view=rev&rev=115261
Log:
improvements suggested by russell and mark michelson; I think taskprocessors are ready for trunk with this commit

Modified:
    team/group/taskprocessors/include/asterisk/taskprocessor.h
    team/group/taskprocessors/main/taskprocessor.c

Modified: team/group/taskprocessors/include/asterisk/taskprocessor.h
URL: http://svn.digium.com/view/asterisk/team/group/taskprocessors/include/asterisk/taskprocessor.h?view=diff&rev=115261&r1=115260&r2=115261
==============================================================================
--- team/group/taskprocessors/include/asterisk/taskprocessor.h (original)
+++ team/group/taskprocessors/include/asterisk/taskprocessor.h Fri May  2 17:33:23 2008
@@ -31,26 +31,30 @@
  * \author Dwayne M. Hubbard <dhubbard at digium.com>
  *
  * \note A taskprocessor is a named singleton containing a processing thread and
- * a task queue that serializes tasks pushed into it by one or more independent modules.  
+ * a task queue that serializes tasks pushed into it by [a] module(s) that reference the taskprocessor.  
  * A taskprocessor is created the first time its name is requested via the ast_taskprocessor_get()
  * function and destroyed when the taskprocessor reference count reaches zero.
  *
  * Modules that obtain a reference to a taskprocessor can queue tasks into the taskprocessor
- * where tasks are processed by the singleton processing thread when the task reaches the front 
+ * to be processed by the singleton processing thread when the task is popped off the front 
  * of the queue.  A task is a wrapper around a task-handling function pointer and a data
  * pointer.  It is the responsibility of the task handling function to free memory allocated for
- * the task data pointer.  A task is obtained using the ast_task_alloc() and freed by the 
- * taskprocessor after the task has been processed.  A module releases its reference to a
- * taskprocessor using the ast_taskprocessor_unreference() function.  Tasks waiting to be
- * processed in the taskprocessor queue when the taskprocessor reference count reaches zero
+ * the task data pointer.  A task is pushed into a taskprocessor queue using the 
+ * ast_taskprocessor_push(taskprocessor, taskhandler, taskdata) function and freed by the
+ * taskprocessor after the task handling function returns.  A module releases its reference to a
+ * taskprocessor using the ast_taskprocessor_unreference() function which may result in the
+ * destruction of the taskprocessor if the taskprocessor's reference count reaches zero.  Tasks waiting
+ * to be processed in the taskprocessor queue when the taskprocessor reference count reaches zero
  * will be purged and released from the taskprocessor queue without being processed.
  */
 struct ast_taskprocessor;
 
-/*! \brief ast_tps_options is used to specify whether a taskprocessor should be created
- * on an attempt to ast_taskprocessor_get() if the taskprocessor does not already exist.
- * The default behavior is to create a taskprocessor if it does not already exist and provide
- * a reference to the taskprocessor if it already exists. */
+/*! \brief ast_tps_options for specification of taskprocessor options
+ *
+ * Specify whether a taskprocessor should be created via ast_taskprocessor_get() if the taskprocessor 
+ * does not already exist.  The default behavior is to create a taskprocessor if it does not already exist 
+ * and provide its reference to the calling function.  To only return a reference to a taskprocessor if 
+ * and only if it exists, use the TPS_REF_IF_EXISTS option in ast_taskprocessor_get(). */
 enum ast_tps_options {
 	/*! \brief return a reference to a taskprocessor, create one if it does not exist */
 	TPS_REF_DEFAULT = 0,
@@ -58,30 +62,33 @@
 	TPS_REF_IF_EXISTS = (1 << 0),
 };
 
-/*! \brief Get a reference to a taskprocessor with the specified name; create a taskprocessor
- * if one with the specified name does not already exist.  
+/*! \brief Get a reference to a taskprocessor with the specified name and create the taskprocessor if necessary
+ *
  * The default behavior of instantiating a taskprocessor if one does not already exist can be
- * disabled by specifying the TPS_REF_IF_EXISTS reference type as the third argument to ast_taskprocessor_get().
+ * disabled by specifying the TPS_REF_IF_EXISTS ast_tps_options as the second argument to ast_taskprocessor_get().
  * \param name The name of the taskprocessor
  * \param create Use 0 by default or specify TPS_REF_IF_EXISTS to return NULL if the taskprocessor does 
  * not already exist
  * return A pointer to a reference counted taskprocessor under normal conditions, or NULL if the
- * TPS_REF_IF_EXISTS reference type is specified */
+ * TPS_REF_IF_EXISTS reference type is specified and the taskprocessor does not exist */
 struct ast_taskprocessor *ast_taskprocessor_get(char *name, enum ast_tps_options create);
 
 /*! \brief Unreference the specified taskprocessor and its reference count will decrement.
- * taskprocessors use astobj2 and will destroy themselves when their reference count reaches zero.
+ *
+ * Taskprocessors use astobj2 and will unlink from the taskprocessor singleton container and destroy
+ * themself when the taskprocessor reference count reaches zero.
  * \param tps taskprocessor to unreference
  * \return NULL */
 void *ast_taskprocessor_unreference(struct ast_taskprocessor *tps);
 
 /*! \brief Push a task into the specified taskprocessor queue and signal the taskprocessor thread
  * \param tps The taskprocessor structure
- * \param t The task to push into the taskprocessor queue
+ * \param task_exe The task handling function to push into the taskprocessor queue
+ * \param datap The data to be used by the task handling function
  * \return zero on success, -1 on failure */
 int ast_taskprocessor_push(struct ast_taskprocessor *tps, int (*task_exe)(void *datap), void *datap);
 
 /*! \brief Return the name of the taskprocessor singleton */
-char * ast_taskprocessor_name(struct ast_taskprocessor *tps);
+const char * ast_taskprocessor_name(struct ast_taskprocessor *tps);
 #endif
 

Modified: team/group/taskprocessors/main/taskprocessor.c
URL: http://svn.digium.com/view/asterisk/team/group/taskprocessors/main/taskprocessor.c?view=diff&rev=115261&r1=115260&r2=115261
==============================================================================
--- team/group/taskprocessors/main/taskprocessor.c (original)
+++ team/group/taskprocessors/main/taskprocessor.c Fri May  2 17:33:23 2008
@@ -17,8 +17,7 @@
  */
 /*! \file
  *
- * \brief Maintain a container of taskprocessor threads that are uniquely named
- * and can be shared across modules.
+ * \brief Maintain a container of uniquely-named taskprocessor threads that can be shared across modules.
  *
  * \author Dwayne Hubbard <dhubbard at digium.com>
  */
@@ -35,10 +34,11 @@
 
 ASTERISK_FILE_VERSION(__FILE__, "$Revision$")
 
-/*! \brief tps_task structure is queued to a taskprocessor and released
- * by the taskprocessing thread.  The callback function that is assigned
- * to the execute() function pointer is responsible for releasing
- * datap resources if necessary. */
+/*! \brief tps_task structure is queued to a taskprocessor
+ *
+ * tps_tasks are processed in FIFO order and freed by the taskprocessing
+ * thread after the task handler returns.  The callback function that is assigned
+ * to the execute() function pointer is responsible for releasing datap resources if necessary. */
 struct tps_task {
 	/*! \brief The execute() task callback function pointer */
 	int (*execute)(void *datap);
@@ -48,8 +48,7 @@
 	AST_LIST_ENTRY(tps_task) list;
 };
 
-/*! \brief tps_taskprocessor_stats are technically optional, but
- * used by default to keep track of statistics for an individual taskprocessor */
+/*! \brief tps_taskprocessor_stats maintain statistics for a taskprocessor. */
 struct tps_taskprocessor_stats {
 	/*! \brief This is the maximum number of tasks queued at any one time */
 	unsigned long max_qsize;
@@ -57,8 +56,7 @@
 	unsigned long _tasks_processed_count;
 };
 
-/*! \brief A ast_taskprocessor structure is used to manage 
- *  a unique taskprocessor thread */
+/*! \brief A ast_taskprocessor structure is a singleton by name */
 struct ast_taskprocessor {
 	/*! \brief Friendly name of the taskprocessor */
 	char *name;
@@ -80,19 +78,32 @@
 	AST_LIST_ENTRY(ast_taskprocessor) list;
 };
 #define TPS_MAX_BUCKETS 7
+/*! \brief tps_singletons is the astobj2 container for taskprocessor singletons */
 static struct ao2_container *tps_singletons;
-ast_mutex_t tps_marshall;
-
-AST_LIST_HEAD_STATIC(taskprocessor_singletons, ast_taskprocessor);
+
+/*! \brief CLI 'taskprocessor ping <blah>' operation requires a ping condition */
 static ast_cond_t cli_ping_cond;
+/*! \brief CLI 'taskprocessor ping <blah>' operation requires a ping condition lock */
 static ast_mutex_t cli_ping_cond_lock;
 
+/*! \brief The astobj2 hash callback for taskprocessors */
 static int tps_hash_cb(const void *obj, const int flags);
+/*! \brief The astobj2 compare callback for taskprocessors */
 static int tps_cmp_cb(void *obj, void *arg, int flags);
-static void *tps_default_processor_function(void *data);
+
+/*! \brief The task processing function executed by a taskprocessor */
+static void *tps_processing_function(void *data);
+
+/*! \brief Destroy the taskprocessor when its refcount reaches zero */
 static void tps_taskprocessor_destroy(void *tps);
+
+/*! \brief CLI 'taskprocessor ping <blah>' handler function */
 static int tps_ping_handler(void *datap);
+
+/*! \brief Remove the front task off the taskprocessor queue */
 static struct tps_task *tps_taskprocessor_pop(struct ast_taskprocessor *tps);
+
+/*! \brief Return the size of the taskprocessor queue */
 static int tps_taskprocessor_depth(struct ast_taskprocessor *tps);
 
 static char *cli_tps_ping(struct ast_cli_entry *e, int cmd, struct ast_cli_args *a);
@@ -103,6 +114,7 @@
 	AST_CLI_DEFINE(cli_tps_report, "List instantiated task processors and statistics"),
 };
 
+/* initialize the taskprocessor container and register CLI operations */
 int ast_tps_init(void)
 {
 	if (!(tps_singletons = ao2_container_alloc(TPS_MAX_BUCKETS, tps_hash_cb, tps_cmp_cb))) {
@@ -113,6 +125,7 @@
 	return 0;
 }
 
+/* allocate resources for the task */
 static struct tps_task *tps_task_alloc(int (*task_exe)(void *datap), void *datap)
 {
 	struct tps_task *t;
@@ -122,7 +135,8 @@
 	}
 	return t;
 }
-	
+
+/* release task resources */	
 static void *tps_task_free(struct tps_task *task)
 {
 	if (task) {
@@ -131,9 +145,7 @@
 	return NULL;
 }
 
-/*
- * Taskprocessor CLI
- */
+/* taskprocessor tab completion */
 static char *tps_taskprocessor_tab_complete(struct ast_taskprocessor *p, struct ast_cli_args *a) 
 {
 	int tklen;
@@ -145,7 +157,6 @@
 		return NULL;
 
 	tklen = strlen(a->word);
-	ast_mutex_lock(&tps_marshall);
 	i = ao2_iterator_init(tps_singletons, 0);
 	while ((p = ao2_iterator_next(&i))) {
 		if (!strncasecmp(a->word, p->name, tklen) && ++wordnum > a->n) {
@@ -155,10 +166,10 @@
 		}
 		ao2_ref(p, -1);
 	}
-	ast_mutex_unlock(&tps_marshall);
 	return name;
 }
 
+/* ping task handling function */
 static int tps_ping_handler(void *datap)
 {
 	ast_mutex_lock(&cli_ping_cond_lock);
@@ -167,6 +178,7 @@
 	return 0;
 }
 
+/* ping the specified taskprocessor and display the ping time on the CLI */
 static char *cli_tps_ping(struct ast_cli_entry *e, int cmd, struct ast_cli_args *a)
 {
 	struct timeval begin, end, delta;
@@ -213,6 +225,7 @@
 	return CLI_SUCCESS;	
 }
 
+/* TPS reports are cool */
 static char *cli_tps_report(struct ast_cli_entry *e, int cmd, struct ast_cli_args *a)
 {
 	char name[256];
@@ -238,7 +251,6 @@
 		return CLI_SHOWUSAGE;
 
 	ast_cli(a->fd, "\n\t+----- Processor -----+--- Processed ---+- In Queue -+- Max Depth -+");
-	ast_mutex_lock(&tps_marshall);
 	i = ao2_iterator_init(tps_singletons, 0);
 	while ((p = ao2_iterator_next(&i))) {
 		ast_copy_string(name, p->name, sizeof(name));
@@ -249,15 +261,12 @@
 		ao2_ref(p, -1);
 	}
 	tcount = ao2_container_count(tps_singletons); 
-	ast_mutex_unlock(&tps_marshall);
 	ast_cli(a->fd, "\n\t+---------------------+-----------------+------------+-------------+\n\t%d taskprocessors\n\n", tcount);
 	return CLI_SUCCESS;	
 }
 
-/*
- * Task processing
- */
-static void *tps_default_processor_function(void *data)
+/* this is the task processing worker function */
+static void *tps_processing_function(void *data)
 {
 	struct ast_taskprocessor *i = data;
 	struct tps_task *t;
@@ -269,51 +278,49 @@
 	}
 
 	while (i->poll_thread_run) {
-		if ((size = tps_taskprocessor_depth(i)) > 0) {
-			/* stuff is in the queue */
-			t = tps_taskprocessor_pop(i);
-			if (!t) {
-				ast_log(LOG_ERROR, "Wtf?? %d tasks in the queue, but we're popping blanks!\n", size);
-				continue;
+ 		ast_mutex_lock(&i->taskprocessor_lock);
+ 		if (!i->poll_thread_run) {
+  			ast_mutex_unlock(&i->taskprocessor_lock);
+ 			break;
+  		}
+ 		if (!(size = tps_taskprocessor_depth(i))) {
+ 			ast_cond_wait(&i->poll_cond, &i->taskprocessor_lock);
+  			if (!i->poll_thread_run) {
+  				ast_mutex_unlock(&i->taskprocessor_lock);
+	  			break;
 			}
-			if (!t->execute) {
-				ast_log(LOG_WARNING, "Task is missing a function to execute!\n");
-				tps_task_free(t);
-				continue;
-			}
-			t->execute(t->datap);
-			ast_mutex_lock(&i->taskprocessor_lock);
-			if (i->stats) {
-				i->stats->_tasks_processed_count++;
-				if (size > i->stats->max_qsize) {
-					i->stats->max_qsize = size;
-				}
-			}
-			ast_mutex_unlock(&i->taskprocessor_lock);
-			tps_task_free(t);
-			if (--size) 
-				continue;
-		}
-		ast_mutex_lock(&i->taskprocessor_lock);
-		if (!i->poll_thread_run) {
-			ast_mutex_unlock(&i->taskprocessor_lock);
-			break;
-		}
-		if (!tps_taskprocessor_depth(i)) 
-			ast_cond_wait(&i->poll_cond, &i->taskprocessor_lock);
-		ast_mutex_unlock(&i->taskprocessor_lock);
-	}
-	while (tps_taskprocessor_depth(i)) {
-		/* stuff is in the queue */
-		t = tps_taskprocessor_pop(i);
-		if (t) {
-			tps_task_free(t);
-			t = NULL;
-		}
+  		}
+  		ast_mutex_unlock(&i->taskprocessor_lock);
+ 		/* stuff is in the queue */
+ 		if (!(t = tps_taskprocessor_pop(i))) {
+ 			ast_log(LOG_ERROR, "Wtf?? %d tasks in the queue, but we're popping blanks!\n", size);
+ 			continue;
+ 		}
+ 		if (!t->execute) {
+ 			ast_log(LOG_WARNING, "Task is missing a function to execute!\n");
+ 			tps_task_free(t);
+ 			continue;
+ 		}
+ 		t->execute(t->datap);
+ 
+ 		ast_mutex_lock(&i->taskprocessor_lock);
+ 		if (i->stats) {
+ 			i->stats->_tasks_processed_count++;
+ 			if (size > i->stats->max_qsize) {
+ 				i->stats->max_qsize = size;
+ 			}
+ 		}
+ 		ast_mutex_unlock(&i->taskprocessor_lock);
+ 
+ 		tps_task_free(t);
+  	}
+	while ((t = tps_taskprocessor_pop(i))) {
+		tps_task_free(t);
 	}
 	return NULL;
 }
 
+/* hash callback for astobj2 */
 static int tps_hash_cb(const void *obj, const int flags)
 {
 	const struct ast_taskprocessor *tps = obj;
@@ -321,6 +328,7 @@
 	return ast_str_hash(tps->name);
 }
 
+/* compare callback for astobj2 */
 static int tps_cmp_cb(void *obj, void *arg, int flags)
 {
 	struct ast_taskprocessor *lhs = obj, *rhs = arg;
@@ -328,6 +336,7 @@
 	return !strcasecmp(lhs->name, rhs->name) ? CMP_MATCH : 0;
 }
 
+/* destroy the taskprocessor */
 static void tps_taskprocessor_destroy(void *tps)
 {
 	struct ast_taskprocessor *t = tps;
@@ -350,9 +359,9 @@
 		t->stats = NULL;
 	}
 	ast_free(t->name);
-	return;
-}
-
+}
+
+/* pop the front task and return it */
 static struct tps_task *tps_taskprocessor_pop(struct ast_taskprocessor *tps)
 {
 	struct tps_task *task;
@@ -371,14 +380,11 @@
 
 static int tps_taskprocessor_depth(struct ast_taskprocessor *tps)
 {
-	return (tps) ? tps->queue_size:-1;
-}
-
-
-/*
- * Taskprocessor service functions
- */
-char *ast_taskprocessor_name(struct ast_taskprocessor *tps)
+	return (tps) ? tps->queue_size : -1;
+}
+
+/* taskprocessor name accessor */
+const char *ast_taskprocessor_name(struct ast_taskprocessor *tps)
 {
 	if (!tps) {
 		ast_log(LOG_ERROR, "no taskprocessor specified!\n");
@@ -387,78 +393,87 @@
 	return tps->name;
 }
 
+/* Provide a reference to a taskprocessor.  Create the taskprocessor if necessary, but don't
+ * create the taskprocessor if we were told via ast_tps_options to return a reference only 
+ * if it already exists */
 struct ast_taskprocessor *ast_taskprocessor_get(char *name, enum ast_tps_options create)
 {
 	struct ast_taskprocessor *p, tmp_tps = {
 		.name = name,
 	};
 		
-	if (!name) {
+	if (ast_strlen_zero(name)) {
 		ast_log(LOG_ERROR, "requesting a nameless taskprocessor!!!\n");
 		return NULL;
 	}
-	ast_mutex_lock(&tps_marshall);
+	ao2_lock(tps_singletons);
 	p = ao2_find(tps_singletons, &tmp_tps, OBJ_POINTER);
 	if (p) {
-		ast_mutex_unlock(&tps_marshall);
+		ao2_unlock(tps_singletons);
 		return p;
 	}
 	if (create & TPS_REF_IF_EXISTS) {
 		/* calling function does not want a new taskprocessor to be created if it doesn't already exist */
-		ast_mutex_unlock(&tps_marshall);
+		ao2_unlock(tps_singletons);
 		return NULL;
 	}
 	/* create a new taskprocessor */
 	if (!(p = ao2_alloc(sizeof(*p), tps_taskprocessor_destroy))) {
-		ast_mutex_unlock(&tps_marshall);
+		ao2_unlock(tps_singletons);
 		ast_log(LOG_WARNING, "failed to create taskprocessor '%s'\n", name);
 		return NULL;
 	}
 	if (!(p->stats = ast_calloc(1, sizeof(*p->stats)))) {
-		ast_mutex_unlock(&tps_marshall);
+		ao2_unlock(tps_singletons);
 		ast_log(LOG_WARNING, "failed to create taskprocessor stats for '%s'\n", name);
 		ao2_ref(p, -1);
 		return NULL;
 	}
-	p->name = ast_strdup(name);
+	if (!(p->name = ast_strdup(name))) {
+		ao2_unlock(tps_singletons);
+		ao2_ref(p, -1);
+		return NULL;
+	}
 	p->poll_thread_run = 1;
 	ast_cond_init(&p->poll_cond, NULL);
 	p->poll_thread = AST_PTHREADT_NULL;
-	if (ast_pthread_create(&p->poll_thread, NULL, tps_default_processor_function, p) < 0) {
-		ast_mutex_unlock(&tps_marshall);
+	if (ast_pthread_create(&p->poll_thread, NULL, tps_processing_function, p) < 0) {
+		ao2_unlock(tps_singletons);
 		ast_log(LOG_ERROR, "Taskprocessor '%s' failed to create the processing thread.\n", p->name);
 		ao2_ref(p, -1);
 		return NULL;
 	}
 	if (!(ao2_link(tps_singletons, p))) {
-		ast_mutex_unlock(&tps_marshall);
+		ao2_unlock(tps_singletons);
 		ast_log(LOG_ERROR, "Failed to add taskprocessor '%s' to container\n", p->name);
 		ao2_ref(p, -1);
 		return NULL;
 	}
-	ast_mutex_unlock(&tps_marshall);
+	ao2_unlock(tps_singletons);
 	return p;
 }
 
+/* decrement the taskprocessor reference count and unlink from the container if necessary */
 void *ast_taskprocessor_unreference(struct ast_taskprocessor *tps)
 {
 	if (tps) {
-		ast_mutex_lock(&tps_marshall);
+		ao2_lock(tps_singletons);
 		ao2_unlink(tps_singletons, tps);
 		if (ao2_ref(tps, -1) > 1) {
 			ao2_link(tps_singletons, tps);
 		}
-		ast_mutex_unlock(&tps_marshall);
+		ao2_unlock(tps_singletons);
 	}
 	return NULL;
 }
-	
+
+/* push the task into the taskprocessor queue */	
 int ast_taskprocessor_push(struct ast_taskprocessor *tps, int (*task_exe)(void *datap), void *datap)
 {
 	struct tps_task *t;
 
 	if (!tps || !task_exe) {
-		ast_log(LOG_ERROR, "%s is missing!!\n", (tps) ? "task callback":"taskprocessor");
+		ast_log(LOG_ERROR, "%s is missing!!\n", (tps) ? "task callback" : "taskprocessor");
 		return -1;
 	}
 	if (!(t = tps_task_alloc(task_exe, datap))) {




More information about the asterisk-commits mailing list