History

workqueue: use worker_set/clr_flags() only from worker itself

worker_set/clr_flags() assume that if none of NOT_RUNNING flags is set the worker must be contributing to nr_running which is only true if the worker is actually running. As when called from self, it is guaranteed that the worker is running, those functions can be safely used from the worker itself and they aren't necessary from other places anyway. Make the following changes to fix the bug. * Make worker_set/clr_flags() whine if not called from self. * Convert all places which called those functions from other tasks to manipulate flags directly. * Make trustee_thread() directly clear nr_running after setting WORKER_ROGUE on all workers. This is the only place where nr_running manipulation is necessary outside of workers themselves. * While at it, add sanity check for nr_running in worker_enter_idle().

Tejun Heo committed 16 years ago

Commit cb444761 parent fb0e7be1 file · +28 −19Browse files

Showing the changes to kernel/workqueue.c (1 of 1 file). Show all

Unified · Split

kernel/workqueue.c+28−19
@@ -601,7 +601,7 @@ struct task_struct *wq_worker_sleeping(struct task_struct *task, · 600 unmodified lines
601601
602602 /**
603603 * worker_set_flags - set worker flags and adjust nr_running accordingly
604− * @worker: worker to set flags for
604+ * @worker: self
605605 * @flags: flags to set
606606 * @wakeup: wakeup an idle worker if necessary
607607 *
@@ -609,14 +609,16 @@ struct task_struct *wq_worker_sleeping(struct task_struct *task, · 1 unmodified line
609609 * nr_running becomes zero and @wakeup is %true, an idle worker is
610610 * woken up.
611611 *
612− * LOCKING:
613− * spin_lock_irq(gcwq->lock).
612+ * CONTEXT:
613+ * spin_lock_irq(gcwq->lock)
614614 */
615615 static inline void worker_set_flags(struct worker *worker, unsigned int flags,
616616 bool wakeup)
617617 {
618618 struct global_cwq *gcwq = worker->gcwq;
619619
620+ WARN_ON_ONCE(worker->task != current);
621+
620622 /*
621623 * If transitioning into NOT_RUNNING, adjust nr_running and
622624 * wake up an idle worker as necessary if requested by
@@ -639,19 +641,21 @@ static inline void worker_set_flags(struct worker *worker, unsigned int flags, · 16 unmodified lines
639641
640642 /**
641643 * worker_clr_flags - clear worker flags and adjust nr_running accordingly
642− * @worker: worker to set flags for
644+ * @worker: self
643645 * @flags: flags to clear
644646 *
645647 * Clear @flags in @worker->flags and adjust nr_running accordingly.
646648 *
647− * LOCKING:
648− * spin_lock_irq(gcwq->lock).
649+ * CONTEXT:
650+ * spin_lock_irq(gcwq->lock)
649651 */
650652 static inline void worker_clr_flags(struct worker *worker, unsigned int flags)
651653 {
652654 struct global_cwq *gcwq = worker->gcwq;
653655 unsigned int oflags = worker->flags;
654656
657+ WARN_ON_ONCE(worker->task != current);
658+
655659 worker->flags &= ~flags;
656660
657661 /* if transitioning out of NOT_RUNNING, increment nr_running */
@@ -1073,7 +1077,8 @@ static void worker_enter_idle(struct worker *worker) · 415 unmodified lines
10731077 BUG_ON(!list_empty(&worker->entry) &&
10741078 (worker->hentry.next || worker->hentry.pprev));
10751079
1076− worker_set_flags(worker, WORKER_IDLE, false);
1080+ /* can't use worker_set_flags(), also called from start_worker() */
1081+ worker->flags |= WORKER_IDLE;
10771082 gcwq->nr_idle++;
10781083 worker->last_active = jiffies;
10791084
@@ -1086,6 +1091,10 @@ static void worker_enter_idle(struct worker *worker) · 6 unmodified lines
10861091 jiffies + IDLE_WORKER_TIMEOUT);
10871092 } else
10881093 wake_up_all(&gcwq->trustee_wait);
1094+
1095+ /* sanity check nr_running */
1096+ WARN_ON_ONCE(gcwq->nr_workers == gcwq->nr_idle &&
1097+ atomic_read(get_gcwq_nr_running(gcwq->cpu)));
10891098 }
10901099
10911100 /**
@@ -1270,7 +1279,7 @@ fail: · 178 unmodified lines
12701279 */
12711280 static void start_worker(struct worker *worker)
12721281 {
1273− worker_set_flags(worker, WORKER_STARTED, false);
1282+ worker->flags |= WORKER_STARTED;
12741283 worker->gcwq->nr_workers++;
12751284 worker_enter_idle(worker);
12761285 wake_up_process(worker->task);
@@ -1300,7 +1309,7 @@ static void destroy_worker(struct worker *worker) · 23 unmodified lines
13001309 gcwq->nr_idle--;
13011310
13021311 list_del_init(&worker->entry);
1303− worker_set_flags(worker, WORKER_DIE, false);
1312+ worker->flags |= WORKER_DIE;
13041313
13051314 spin_unlock_irq(&gcwq->lock);
13061315
@@ -2979,10 +2988,10 @@ static int __cpuinit trustee_thread(void *__gcwq) · 1672 unmodified lines
29792988 gcwq->flags |= GCWQ_MANAGING_WORKERS;
29802989
29812990 list_for_each_entry(worker, &gcwq->idle_list, entry)
2982− worker_set_flags(worker, WORKER_ROGUE, false);
2991+ worker->flags |= WORKER_ROGUE;
29832992
29842993 for_each_busy_worker(worker, i, pos, gcwq)
2985− worker_set_flags(worker, WORKER_ROGUE, false);
2994+ worker->flags |= WORKER_ROGUE;
29862995
29872996 /*
29882997 * Call schedule() so that we cross rq->lock and thus can
@@ -2995,12 +3004,12 @@ static int __cpuinit trustee_thread(void *__gcwq) · 6 unmodified lines
29953004 spin_lock_irq(&gcwq->lock);
29963005
29973006 /*
2998− * Sched callbacks are disabled now. gcwq->nr_running should
2999− * be zero and will stay that way, making need_more_worker()
3000− * and keep_working() always return true as long as the
3001− * worklist is not empty.
3007+ * Sched callbacks are disabled now. Zap nr_running. After
3008+ * this, nr_running stays zero and need_more_worker() and
3009+ * keep_working() are always true as long as the worklist is
3010+ * not empty.
30023011 */
3003− WARN_ON_ONCE(atomic_read(get_gcwq_nr_running(gcwq->cpu)) != 0);
3012+ atomic_set(get_gcwq_nr_running(gcwq->cpu), 0);
30043013
30053014 spin_unlock_irq(&gcwq->lock);
30063015 del_timer_sync(&gcwq->idle_timer);
@@ -3046,7 +3055,7 @@ static int __cpuinit trustee_thread(void *__gcwq) · 39 unmodified lines
30463055 worker = create_worker(gcwq, false);
30473056 spin_lock_irq(&gcwq->lock);
30483057 if (worker) {
3049− worker_set_flags(worker, WORKER_ROGUE, false);
3058+ worker->flags |= WORKER_ROGUE;
30503059 start_worker(worker);
30513060 }
30523061 }
@@ -3085,8 +3094,8 @@ static int __cpuinit trustee_thread(void *__gcwq) · 32 unmodified lines
30853094 * operations. Use a separate flag to mark that
30863095 * rebinding is scheduled.
30873096 */
3088− worker_set_flags(worker, WORKER_REBIND, false);
3089− worker_clr_flags(worker, WORKER_ROGUE);
3097+ worker->flags |= WORKER_REBIND;
3098+ worker->flags &= ~WORKER_ROGUE;
30903099
30913100 /* queue rebind_work, wq doesn't matter, use the default one */
30923101 if (test_and_set_bit(WORK_STRUCT_PENDING_BIT,