Re: Scheduler bug related to rq->skip_clock_update?

[Yong Zhang]

On Mon, Dec 6, 2010 at 1:33 PM, Mike Galbraith <efault@xxxxxx> wrote:
> On Sun, 2010-12-05 at 13:28 +0800, Yong Zhang wrote:
>
>> when we init idle task, we doesn't mark it on_rq.
>> My test show the concern is smoothed by below patch.
>
> Close :)
>
> The skip_clock_update flag should only be set if rq->curr is on_rq,
> because it it _that_ clock update during dequeue, and subsequent
> microscopic vruntime update it causes that we're trying to avoid.
>
> I think the below fixes it up properly.

Yep. Now it's running well on my machine.

If you want, you can add my tested-by. :)

Thanks,
Yong

>
> Sched: fix skip_clock_update optimization
>
> idle_balance() drops/retakes rq->lock, leaving the previous task
> vulnerable to set_tsk_need_resched(). ÂClear it after we return
> from balancing instead, and in setup_thread_stack() as well, so
> no successfully descheduled or never scheduled task has it set.
>
> Need resched confused the skip_clock_update logic, which assumes
> that the next call to update_rq_clock() will come nearly immediately
> after being set. ÂMake the optimization robust against the waking
> a sleeper before it sucessfully deschedules case by checking that
> the current task has not been dequeued before setting the flag,
> since it is that useless clock update we're trying to save, and
> clear in update_rq_clock() to ensure that ONE call may be skipped.
>
> Signed-off-by: Mike Galbraith <efault@xxxxxx>
> Cc: Ingo Molnar <mingo@xxxxxxx>
> Cc: Peter Zijlstra <a.p.zijlstra@xxxxxxxxx>
> Cc: Bjoern B. Brandenburg <bbb.lst@xxxxxxxxx>
> Reported-by: Bjoern B. Brandenburg <bbb.lst@xxxxxxxxx>
>
> ---
> Âkernel/fork.c Â| Â Â1 +
> Âkernel/sched.c | Â Â6 +++---
> Â2 files changed, 4 insertions(+), 3 deletions(-)
>
> Index: linux-2.6.37.git/kernel/sched.c
> ===================================================================
> --- linux-2.6.37.git.orig/kernel/sched.c
> +++ linux-2.6.37.git/kernel/sched.c
> @@ -660,6 +660,7 @@ inline void update_rq_clock(struct rq *r
>
> Â Â Â Â Â Â Â Âsched_irq_time_avg_update(rq, irq_time);
> Â Â Â Â}
> + Â Â Â rq->skip_clock_update = 0;
> Â}
>
> Â/*
> @@ -2138,7 +2139,7 @@ static void check_preempt_curr(struct rq
> Â Â Â Â * A queue event has occurred, and we're going to schedule. ÂIn
> Â Â Â Â * this case, we can save a useless back to back clock update.
> Â Â Â Â */
> - Â Â Â if (test_tsk_need_resched(rq->curr))
> + Â Â Â if (rq->curr->se.on_rq && test_tsk_need_resched(rq->curr))
> Â Â Â Â Â Â Â Ârq->skip_clock_update = 1;
> Â}
>
> @@ -3854,7 +3855,6 @@ static void put_prev_task(struct rq *rq,
> Â{
> Â Â Â Âif (prev->se.on_rq)
> Â Â Â Â Â Â Â Âupdate_rq_clock(rq);
> - Â Â Â rq->skip_clock_update = 0;
> Â Â Â Âprev->sched_class->put_prev_task(rq, prev);
> Â}
>
> @@ -3912,7 +3912,6 @@ need_resched_nonpreemptible:
> Â Â Â Â Â Â Â Âhrtick_clear(rq);
>
> Â Â Â Âraw_spin_lock_irq(&rq->lock);
> - Â Â Â clear_tsk_need_resched(prev);
>
> Â Â Â Âswitch_count = &prev->nivcsw;
> Â Â Â Âif (prev->state && !(preempt_count() & PREEMPT_ACTIVE)) {
> @@ -3942,6 +3941,7 @@ need_resched_nonpreemptible:
> Â Â Â Âif (unlikely(!rq->nr_running))
> Â Â Â Â Â Â Â Âidle_balance(cpu, rq);
>
> + Â Â Â clear_tsk_need_resched(prev);
> Â Â Â Âput_prev_task(rq, prev);
> Â Â Â Ânext = pick_next_task(rq);
>
> Index: linux-2.6.37.git/kernel/fork.c
> ===================================================================
> --- linux-2.6.37.git.orig/kernel/fork.c
> +++ linux-2.6.37.git/kernel/fork.c
> @@ -275,6 +275,7 @@ static struct task_struct *dup_task_stru
>
> Â Â Â Âsetup_thread_stack(tsk, orig);
> Â Â Â Âclear_user_return_notifier(tsk);
> + Â Â Â clear_tsk_need_resched(tsk);
> Â Â Â Âstackend = end_of_stack(tsk);
> Â Â Â Â*stackend = STACK_END_MAGIC; Â Â/* for overflow detection */
>
>
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/