DF-2870 / fix.diff
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57 58 59 60 61 62 63 64 65 66 67 68 69 70 71 72 73 74 75 76 77 78 79 80 81 82 83 84 85 86 87 88 89 90 91 92 93 94 95 96 97 98 99 100 101 102 103 104 105 106 107 108 109 110 111 112 113 114 115 116 117 118 119 120 121 122 123 124 125 126 127 128 129 130 131 132 133 134 135 136 137 138 139 140 141 142 143 144 145 146 147 148 149 150 151 152 153 154 155 | --- a/sys/sys/taskqueue.h +++ b/sys/sys/taskqueue.h @@ -55,11 +55,14 @@ */ typedef void (*taskqueue_enqueue_fn)(void *context); +#define TASKQ_RUNNING 0x0001 /* ta_func currently executing */ + struct task { STAILQ_ENTRY(task) ta_link; /* link for queue */ struct taskqueue *ta_queue; /* taskqueue enqueued on */ int ta_pending; /* count times queued */ int ta_priority; /* priority of task in queue */ + int ta_flags; /* TASK_* flags, see above */ task_fn_t *ta_func; /* task handler */ void *ta_context; /* argument for handler */ }; @@ -99,6 +102,7 @@ { .ta_queue = NULL, \ .ta_pending = 0, \ .ta_priority = (priority), \ + .ta_flags = 0, \ .ta_func = (func), \ .ta_context = (context) } @@ -115,6 +119,7 @@ (task)->ta_queue = NULL; \ (task)->ta_pending = 0; \ (task)->ta_priority = (priority); \ + (task)->ta_flags = 0; \ (task)->ta_func = (func); \ (task)->ta_context = (context); \ } while (0) --- a/sys/kern/subr_taskqueue.c +++ b/sys/kern/subr_taskqueue.c @@ -54,7 +54,6 @@ taskqueue_enqueue_fn tq_enqueue; void *tq_context; - struct task *tq_running; struct spinlock tq_lock; struct thread **tq_threads; int tq_tcount; @@ -152,6 +151,14 @@ { TQ_LOCK(queue); queue->tq_flags &= ~TQ_FLAGS_ACTIVE; + /* + * Wait for any armed timeout-task callouts to complete so they + * cannot fire into the queue after it is freed. Completing + * callouts try to enqueue their task, which is rejected with + * EPIPE because TQ_FLAGS_ACTIVE is already clear. + */ + while (queue->tq_callouts != 0) + TQ_SLEEP(queue, &queue->tq_callouts, "tqfree"); taskqueue_run(queue, 1); taskqueue_terminate(queue->tq_threads, queue); TQ_UNLOCK(queue); @@ -325,6 +332,7 @@ { struct taskqueue *queue; struct timeout_task *timeout_task; + int wake_freer; timeout_task = arg; queue = timeout_task->t.ta_queue; @@ -333,8 +341,11 @@ KASSERT((timeout_task->f & DT_CALLOUT_ARMED) != 0, ("Stray timeout")); timeout_task->f &= ~DT_CALLOUT_ARMED; queue->tq_callouts--; + wake_freer = (queue->tq_callouts == 0); taskqueue_enqueue_locked(queue, &timeout_task->t); TQ_UNLOCK(queue); + if (wake_freer) + wakeup(&queue->tq_callouts); } int @@ -398,17 +409,28 @@ while (STAILQ_FIRST(&queue->tq_queue)) { /* * Carefully remove the first task from the queue and - * zero its pending count. + * zero its pending count. Mark it TASKQ_RUNNING under the + * queue spinlock so that concurrent runners (multi-thread + * queues, taskqueue_free() racing the service thread) each + * track their own task. */ task = STAILQ_FIRST(&queue->tq_queue); STAILQ_REMOVE_HEAD(&queue->tq_queue, ta_link); pending = task->ta_pending; task->ta_pending = 0; - queue->tq_running = task; + atomic_set_int(&task->ta_flags, TASKQ_RUNNING); TQ_UNLOCK(queue); task->ta_func(task->ta_context, pending); - queue->tq_running = NULL; + /* + * Clear TASKQ_RUNNING *before* wakeup() and while holding + * the queue spinlock: sleepers check the flag under the + * same spinlock and interlock their sleep with it + * (ssleep), so the wakeup cannot be lost. + */ + TQ_LOCK(queue); + atomic_clear_int(&task->ta_flags, TASKQ_RUNNING); + TQ_UNLOCK(queue); wakeup(task); TQ_LOCK(queue); } @@ -426,7 +448,7 @@ if (pendp != NULL) *pendp = task->ta_pending; task->ta_pending = 0; - return (task == queue->tq_running ? EBUSY : 0); + return ((task->ta_flags & TASKQ_RUNNING) ? EBUSY : 0); } int @@ -490,7 +512,8 @@ taskqueue_drain(struct taskqueue *queue, struct task *task) { TQ_LOCK(queue); - while (task->ta_pending != 0 || task == queue->tq_running) + while (task->ta_pending != 0 || + (task->ta_flags & TASKQ_RUNNING) != 0) TQ_SLEEP(queue, task, "-"); TQ_UNLOCK(queue); } @@ -509,7 +532,8 @@ if (queue == NULL) return; TQ_LOCK(queue); - if (task->ta_pending == 0 && task != queue->tq_running) { + if (task->ta_pending == 0 && + (task->ta_flags & TASKQ_RUNNING) == 0) { TQ_UNLOCK(queue); return; } @@ -616,6 +640,15 @@ TQ_LOCK(tq); while ((tq->tq_flags & TQ_FLAGS_ACTIVE) != 0) { taskqueue_run(tq, 1); + /* + * Re-check TQ_FLAGS_ACTIVE (under the spinlock we still + * hold from taskqueue_run) before parking. Otherwise a + * wakeup() delivered by taskqueue_terminate() while we + * were inside ta_func is lost and both threads sleep + * forever. + */ + if ((tq->tq_flags & TQ_FLAGS_ACTIVE) == 0) + break; TQ_SLEEP(tq, tq, "tqthr"); } |