Date: Thu, 29 Apr 2010 07:44:35 -0400 From: John Baldwin <jhb@freebsd.org> To: freebsd-current@freebsd.org Cc: Matthew Fleming <matthew.fleming@isilon.com> Subject: Re: sleep bug in taskqueue(9) Message-ID: <201004290744.35090.jhb@freebsd.org> In-Reply-To: <06D5F9F6F655AD4C92E28B662F7F853E039E389A@seaxch09.desktop.isilon.com> References: <06D5F9F6F655AD4C92E28B662F7F853E039E389A@seaxch09.desktop.isilon.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Wednesday 28 April 2010 7:59:58 pm Matthew Fleming wrote:
> 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.
That sounds right.
> 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.
For HEAD I would maybe pad the flags field out to an int? The compiler is
going to make it an int anyway. For the purposes of MFC'ing there are a
couple of options. The simplest might be to reuse the high bit of the
'pending' count as a running flag. Breaking the ABI of struct task would
prohibit an MFC otherwise.
> 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 = 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);
> }
>
>
> @@ -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 != 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
> @@ -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);
>
> 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 */
> _______________________________________________
> freebsd-current@freebsd.org mailing list
> http://lists.freebsd.org/mailman/listinfo/freebsd-current
> To unsubscribe, send any mail to "freebsd-current-unsubscribe@freebsd.org"
>
--
John Baldwin
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201004290744.35090.jhb>