Move WQ,unplug scheduler hooks

Thomas found that we're doing a horrendous amount of work in that scheduler unplug hook while having preempt and IRQs disabled.

Move it to the head of schedule() where both preemption and IRQs are enabled such that we don't get these silly long IRQ/preempt disable times.

This allows us to remove a lot of special magic in the unplug path, simplifying that code as a bonus.

Quoted 6 lines · Peter Zijlstra

Thomas found that we're doing a horrendous amount of work in that scheduler unplug hook while having preempt and IRQs disabled.

Move it to the head of schedule() where both preemption and IRQs are enabled such that we don't get these silly long IRQ/preempt disable times.

Quoted 2 lines

This allows us to remove a lot of special magic in the unplug path, simplifying that code as a bonus.

The major change here is moving the queue running inline, instead of punting to a thread. The worry is/was that we risk blowing the stack if something ends up blocking inadvertently further down the call path.

Since it's the unlikely way to unplug, a bit of latency was acceptable to prevent this problem.

I'm curious why you made that change? It seems orthogonal to the change you are actually describing in the commit message.

Quoted 7 lines · Jens Axboe

Quoted 6 lines · Peter Zijlstra

Thomas found that we're doing a horrendous amount of work in that scheduler unplug hook while having preempt and IRQs disabled.

Move it to the head of schedule() where both preemption and IRQs are enabled such that we don't get these silly long IRQ/preempt disable times.

Quoted 6 lines

Quoted 2 lines

This allows us to remove a lot of special magic in the unplug path, simplifying that code as a bonus.

The major change here is moving the queue running inline, instead of punting to a thread. The worry is/was that we risk blowing the stack if something ends up blocking inadvertently further down the call path.

Is that a real problem or just a "we have no clue what might happen" countermeasure? The plug list should not be magically refilled once it's split off so this should not recurse endlessly, right? If it does then we better fix it at the root cause of the problem and not by adding some last resort band aid into the scheduler code.

If the stack usage of that whole block code is the real issue, then we probably need to keep that "delegate to async" workaround [sigh!], but definitely outside of the scheduler core code.

Quoted 2 lines

Since it's the unlikely way to unplug, a bit of latency was acceptable to prevent this problem.

It's not at all acceptable. There is no reason to hook stuff which runs perfectly fine in preemptible code into the irq disabled region of the scheduler internals.

Quoted 2 lines

I'm curious why you made that change? It seems orthogonal to the change you are actually describing in the commit message.

Right, it should be split into two separate commits, one moving the stuff out from the irq disabled region and the other removing that from_schedule hackery. The latter can be dropped.

Quoted 6 lines · Thomas Gleixner

On Wed, 22 Jun 2011, Jens Axboe wrote:
Is that a real problem or just a "we have no clue what might happen" countermeasure? The plug list should not be magically refilled once it's split off so this should not recurse endlessly, right? If it does then we better fix it at the root cause of the problem and not by adding some last resort band aid into the scheduler code.

It is supposedly a real problem, not just an inkling. It's not about recursing indefinitely, the plug is fairly bounded. But the IO dispatch path can be pretty deep, and if you hit that deep inside the reclaim or file system write path, then you get dangerously close. Dave Chinner posted some numbers in the 2.6.39-rc1 time frame showing how close we got.

We are talking past each other again. Flushing on going to sleep is needed. Placement of that call was pretty much left in the hands of the scheduler people. I personally don't care where it's put, as long as it does what is needed.

Quoted 6 lines

It is supposedly a real problem, not just an inkling. It's not about recursing indefinitely, the plug is fairly bounded. But the IO dispatch path can be pretty deep, and if you hit that deep inside the reclaim or file system write path, then you get dangerously close. Dave Chinner posted some numbers in the 2.6.39-rc1 time frame showing how close we got.

Fair enough.

Ok. So we move it out and keep the from_scheduler flag so that code does not go down the IO path from there.

Quoted 7 lines · Thomas Gleixner

Quoted 6 lines

It is supposedly a real problem, not just an inkling. It's not about recursing indefinitely, the plug is fairly bounded. But the IO dispatch path can be pretty deep, and if you hit that deep inside the reclaim or file system write path, then you get dangerously close. Dave Chinner posted some numbers in the 2.6.39-rc1 time frame showing how close we got.

Quoted 4 lines

Fair enough.

Ok. So we move it out and keep the from_scheduler flag so that code does not go down the IO path from there.

Won't punting the plug to a worker thread wreck all kinds of io accounting due to the wrong task doing the actual io submission?

Quoted 13 lines · Peter Zijlstra

Quoted 11 lines · Thomas Gleixner

Quoted 6 lines

It is supposedly a real problem, not just an inkling. It's not about recursing indefinitely, the plug is fairly bounded. But the IO dispatch path can be pretty deep, and if you hit that deep inside the reclaim or file system write path, then you get dangerously close. Dave Chinner posted some numbers in the 2.6.39-rc1 time frame showing how close we got.

Fair enough.

Ok. So we move it out and keep the from_scheduler flag so that code does not go down the IO path from there.

Quoted 2 lines

Won't punting the plug to a worker thread wreck all kinds of io accounting due to the wrong task doing the actual io submission?

I think all the accounting will the done in IO submission path and while IO is added to plug. This is just plug flush so should not have effect on accounting.

kernel/sched.c +3 -1

-/**
  * wake_up_process - Wake up a specific process
  * @p: The process to be woken up.
  *

@@ -4215,6 +4181,19 @@ static inline void sched_submit_work(voi
 
 	if (tsk->state && !(preempt_count() & PREEMPT_ACTIVE)) {
 		/*
+		 * If a worker went to sleep, notify and ask workqueue
+		 * whether it wants to wake up a task to maintain
+		 * concurrency.

kernel/sched.c +0 -3

 		 * If we are going to sleep and we have plugged IO
 		 * queued, make sure to submit it to avoid deadlocks.
 		 */

@@ -4256,19 +4235,6 @@ asmlinkage void __sched schedule(void)
 		} else {
 			deactivate_task(rq, prev, DEQUEUE_SLEEP);
 			prev->on_rq = 0;
-
-			/*
-			 * If a worker went to sleep, notify and ask workqueue

Hello, Peter.

Quoted 17 lines · Peter Zijlstra

kernel/sched.c +9 -1

-/**
  * wake_up_process - Wake up a specific process
  * @p: The process to be woken up.
  *

@@ -4215,6 +4181,19 @@ static inline void sched_submit_work(voi
 
 	if (tsk->state && !(preempt_count() & PREEMPT_ACTIVE)) {
 		/*
+		 * If a worker went to sleep, notify and ask workqueue
+		 * whether it wants to wake up a task to maintain
+		 * concurrency.
+		 */
+		if (tsk->flags & PF_WQ_WORKER) {
+			struct task_struct *to_wakeup;
+
+			to_wakeup = wq_worker_sleeping(tsk, smp_processor_id());
+			if (to_wakeup)

kernel/sched.c static inline void sched_submit_work(voi · line 4196

Quoted 4 lines

kernel/sched.c +4 -0

+				wake_up_process(to_wakeup);
+		}
+
+		/*

Preemption could still be enabled here, right? What prevents preemtion kicking after wq_worker_sleeping() and do it again thus breaking nr_running tracking.

Quoted 15 lines

kernel/sched.c +0 -8

 		 * If we are going to sleep and we have plugged IO
 		 * queued, make sure to submit it to avoid deadlocks.
 		 */

@@ -4256,19 +4235,6 @@ asmlinkage void __sched schedule(void)
 		} else {
 			deactivate_task(rq, prev, DEQUEUE_SLEEP);
 			prev->on_rq = 0;
-
-			/*
-			 * If a worker went to sleep, notify and ask workqueue
-			 * whether it wants to wake up a task to maintain
-			 * concurrency.
-			 */
-			if (prev->flags & PF_WQ_WORKER) {
-				struct task_struct *to_wakeup;

kernel/sched.c asmlinkage void __sched schedule(void) · line 4271

Quoted 5 lines

kernel/sched.c +0 -5

-
-				to_wakeup = wq_worker_sleeping(prev, cpu);
-				if (to_wakeup)
-					try_to_wake_up_local(to_wakeup);
-			}

Similarly, the if the 'if {}' part of the above if/else is taken, the task never goes to sleep and nr_running will again be broken.

Quoted 3 lines · Tejun Heo

Preemption could still be enabled here, right? What prevents preemtion kicking after wq_worker_sleeping() and do it again thus breaking nr_running tracking.

Aren't all PF_WQ_WORKER threads cpu-bound?

On Wed, Jun 22, 2011 at 01:34:46AM +0200, Peter Zijlstra wrote:

Preemption could still be enabled here, right? What prevents preemtion kicking after wq_worker_sleeping() and do it again thus breaking nr_running tracking.

Quoted 15 lines

kernel/sched.c +0 -8

 		 * If we are going to sleep and we have plugged IO
 		 * queued, make sure to submit it to avoid deadlocks.
 		 */

@@ -4256,19 +4235,6 @@ asmlinkage void __sched schedule(void)
 		} else {
 			deactivate_task(rq, prev, DEQUEUE_SLEEP);
 			prev->on_rq = 0;
-
-			/*
-			 * If a worker went to sleep, notify and ask workqueue
-			 * whether it wants to wake up a task to maintain
-			 * concurrency.
-			 */
-			if (prev->flags & PF_WQ_WORKER) {
-				struct task_struct *to_wakeup;

kernel/sched.c asmlinkage void __sched schedule(void) · line 4271

Quoted 5 lines

kernel/sched.c +0 -5

-
-				to_wakeup = wq_worker_sleeping(prev, cpu);
-				if (to_wakeup)
-					try_to_wake_up_local(to_wakeup);
-			}

Similarly, the if the 'if {}' part of the above if/else is taken, the task never goes to sleep and nr_running will again be broken.

Keeping IRQs disabled for long is not a feature.