Date: Wed, 28 Apr 2010 16:59:58 -0700 From: "Matthew Fleming" <matthew.fleming@isilon.com> To: <freebsd-current@freebsd.org> Subject: sleep bug in taskqueue(9) Message-ID: <06D5F9F6F655AD4C92E28B662F7F853E039E389A@seaxch09.desktop.isilon.com>
next in thread | raw e-mail | index | archive | help
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
@@ -51,7 +51,6 @@ __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;
@@ -233,13 +232,13 @@ taskqueue_run(struct taskqueue *queue)
STAILQ_REMOVE_HEAD(&queue->tq_queue, ta_link);
pending =3D task->ta_pending;
task->ta_pending =3D 0;
- queue->tq_running =3D task;
+ task->ta_flags |=3D TA_FLAGS_RUNNING;
TQ_UNLOCK(queue);
=20
task->ta_func(task->ta_context, pending);
=20
TQ_LOCK(queue);
- queue->tq_running =3D NULL;
+ task->ta_flags &=3D ~TA_FLAGS_RUNNING;
wakeup(task);
}
=20
@@ -256,14 +255,16 @@ taskqueue_drain(struct taskqueue *queue, struct
task *task)
{
if (queue->tq_spin) { /* XXX */
mtx_lock_spin(&queue->tq_mutex);
- while (task->ta_pending !=3D 0 || task =3D=3D
queue->tq_running)
+ while (task->ta_pending !=3D 0 ||
+ (task->ta_flags & TA_FLAGS_RUNNING) !=3D 0)
msleep_spin(task, &queue->tq_mutex, "-", 0);
mtx_unlock_spin(&queue->tq_mutex);
} else {
WITNESS_WARN(WARN_GIANTOK | WARN_SLEEPOK, NULL,
__func__);
=20
mtx_lock(&queue->tq_mutex);
- while (task->ta_pending !=3D 0 || task =3D=3D
queue->tq_running)
+ while (task->ta_pending !=3D 0 ||
+ (task->ta_flags & TA_FLAGS_RUNNING) !=3D 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
@@ -36,15 +36,21 @@
* 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);
=20
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 */
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?06D5F9F6F655AD4C92E28B662F7F853E039E389A>