sleep bug in taskqueue(9)

From: Matthew Fleming <matthew.fleming_at_isilon.com>
Date: Wed, 28 Apr 2010 16:59:58 -0700
It looks to me like taskqueue_drain(taskqueue_thread, foo) will not
correctly detect whether or not a task is currently running.  The check
is against a field in the taskqueue struct, but for the taskqueue_thread
queue with more than one thread, multiple threads can simultaneously be
running a task, thus stomping over the tq_running field.

I have not seen any problem with the code as-is in actual use, so this
is purely an inspection bug.

The following patch should fix the problem.  Because it changes the size
of struct task I'm not sure if it would be suitable for MFC.

Thanks,
matthew

diff --git a/sys/kern/subr_taskqueue.c b/sys/kern/subr_taskqueue.c
index 8405b3d..3b18269 100644
--- a/sys/kern/subr_taskqueue.c
+++ b/sys/kern/subr_taskqueue.c
_at__at_ -51,7 +51,6 _at__at_ __FBSDID("$FreeBSD$");
 	const char		*tq_name;
 	taskqueue_enqueue_fn	tq_enqueue;
 	void			*tq_context;
-	struct task		*tq_running;
 	struct mtx		tq_mutex;
 	struct thread		**tq_threads;
 	int			tq_tcount;
_at__at_ -233,13 +232,13 _at__at_ taskqueue_run(struct taskqueue *queue)
 		STAILQ_REMOVE_HEAD(&queue->tq_queue, ta_link);
 		pending = task->ta_pending;
 		task->ta_pending = 0;
-		queue->tq_running = task;
+		task->ta_flags |= TA_FLAGS_RUNNING;
 		TQ_UNLOCK(queue);
 
 		task->ta_func(task->ta_context, pending);
 
 		TQ_LOCK(queue);
-		queue->tq_running = NULL;
+		task->ta_flags &= ~TA_FLAGS_RUNNING;
 		wakeup(task);
 	}
 

_at__at_ -256,14 +255,16 _at__at_ taskqueue_drain(struct taskqueue *queue, struct
task *task)
 {
 	if (queue->tq_spin) {		/* XXX */
 		mtx_lock_spin(&queue->tq_mutex);
-		while (task->ta_pending != 0 || task ==
queue->tq_running)
+		while (task->ta_pending != 0 ||
+		    (task->ta_flags & TA_FLAGS_RUNNING) != 0)
 			msleep_spin(task, &queue->tq_mutex, "-", 0);
 		mtx_unlock_spin(&queue->tq_mutex);
 	} else {
 		WITNESS_WARN(WARN_GIANTOK | WARN_SLEEPOK, NULL,
__func__);
 
 		mtx_lock(&queue->tq_mutex);
-		while (task->ta_pending != 0 || task ==
queue->tq_running)
+		while (task->ta_pending != 0 ||
+		    (task->ta_flags & TA_FLAGS_RUNNING) != 0)
 			msleep(task, &queue->tq_mutex, PWAIT, "-", 0);
 		mtx_unlock(&queue->tq_mutex);
 	}
diff --git a/sys/sys/_task.h b/sys/sys/_task.h
index 2a51e1b..c57bdd5 100644
--- a/sys/sys/_task.h
+++ b/sys/sys/_task.h
_at__at_ -36,15 +36,21 _at__at_
  * taskqueue_run().  The first argument is taken from the 'ta_context'
  * field of struct task and the second argument is a count of how many
  * times the task was enqueued before the call to taskqueue_run().
+ *
+ * List of locks
+ * (c)	const after init
+ * (q)	taskqueue lock
  */
 typedef void task_fn_t(void *context, int pending);
 
 struct task {
-	STAILQ_ENTRY(task) ta_link;	/* link for queue */
-	u_short	ta_pending;		/* count times queued */
-	u_short	ta_priority;		/* Priority */
-	task_fn_t *ta_func;		/* task handler */
-	void	*ta_context;		/* argument for handler */
+	STAILQ_ENTRY(task) ta_link;	/* (q) link for queue */
+	u_char	ta_flags;		/* (q) state of this task */
+#define	TA_FLAGS_RUNNING	0x01
+	u_short	ta_pending;		/* (q) count times queued */
+	u_short	ta_priority;		/* (c) Priority */
+	task_fn_t *ta_func;		/* (c) task handler */
+	void	*ta_context;		/* (c) argument for handler */
Received on Wed Apr 28 2010 - 22:00:00 UTC

This archive was generated by hypermail 2.4.0 : Wed May 19 2021 - 11:40:03 UTC