From owner-freebsd-current@FreeBSD.ORG Thu Apr 29 00:00:00 2010 Return-Path: Delivered-To: freebsd-current@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id D19A71065670 for ; Thu, 29 Apr 2010 00:00:00 +0000 (UTC) (envelope-from matthew.fleming@isilon.com) Received: from seaxch09.isilon.com (seaxch09.isilon.com [74.85.160.25]) by mx1.freebsd.org (Postfix) with ESMTP id AF3C28FC17 for ; Wed, 28 Apr 2010 23:59:58 +0000 (UTC) x-mimeole: Produced By Microsoft Exchange V6.5 MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable x-cr-hashedpuzzle: Ayrt A/c0 BFP3 BH1e Bq0o BrnT Dexf FN40 GltG G5mJ HP5F ImHR Iqoh It+9 IvGQ Iwby; 1; ZgByAGUAZQBiAHMAZAAtAGMAdQByAHIAZQBuAHQAQABmAHIAZQBlAGIAcwBkAC4AbwByAGcA; Sosha1_v1; 7; {9DF9F10B-1901-4C9D-A53C-7D50EB92009B}; bQBhAHQAdABoAGUAdwAuAGYAbABlAG0AaQBuAGcAQABpAHMAaQBsAG8AbgAuAGMAbwBtAA==; Wed, 28 Apr 2010 23:59:58 GMT; cwBsAGUAZQBwACAAYgB1AGcAIABpAG4AIAB0AGEAcwBrAHEAdQBlAHUAZQAoADkAKQA= x-cr-puzzleid: {9DF9F10B-1901-4C9D-A53C-7D50EB92009B} Content-class: urn:content-classes:message Date: Wed, 28 Apr 2010 16:59:58 -0700 Message-ID: <06D5F9F6F655AD4C92E28B662F7F853E039E389A@seaxch09.desktop.isilon.com> X-MS-Has-Attach: X-MS-TNEF-Correlator: Thread-Topic: sleep bug in taskqueue(9) Thread-Index: AcrnLugjIb9k3BJXRcO0WnukadoAGg== From: "Matthew Fleming" To: X-Mailman-Approved-At: Thu, 29 Apr 2010 02:04:52 +0000 Subject: sleep bug in taskqueue(9) X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 29 Apr 2010 00:00:01 -0000 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 */