Messages in this thread Patch in this message | | | Subject | Re: [PATCH 05/10] block: remove per-queue plugging | From | Peter Zijlstra <> | Date | Mon, 07 Mar 2011 11:23:47 +0100 |
| |
On Sat, 2011-03-05 at 21:54 +0100, Jens Axboe wrote: > > Apparently so. Peter/Ingo, please shoot this one down in flames. > Summary: > > - Need a way to trigger this flushing when a task is going to sleep > - It's currently done right before calling deactivate_task(). We know > the task is going to sleep here, but it's also under the runqueue > lock. Not good. > - In the new location, it's not completely clear to me whether we can > safely deref 'prev' or not. The usage of prev_state would seem to > indicate that we cannot, and as far as I can tell, prev could at this > point already potentially be running on another CPU. > > Help? Peter, we talked about this in Tokyo in September. Initial > suggestion was to use preempt notifiers, which we can't because: > > - runqueue lock is also held > - It's not unconditionally available, depends on config. > > diff --git a/kernel/sched.c b/kernel/sched.c > index e806446..8581ad3 100644 > --- a/kernel/sched.c > +++ b/kernel/sched.c > @@ -2826,6 +2826,14 @@ static void finish_task_switch(struct rq *rq, struct task_struct *prev) > #endif /* __ARCH_WANT_INTERRUPTS_ON_CTXSW */ > finish_lock_switch(rq, prev); > > + /* > + * If this task has IO plugged, make sure it > + * gets flushed out to the devices before we go > + * to sleep > + */ > + if (prev_state != TASK_RUNNING) > + blk_flush_plug(prev); > + > fire_sched_in_preempt_notifiers(current); > if (mm) > mmdrop(mm); > @@ -3973,14 +3981,6 @@ need_resched_nonpreemptible: > if (to_wakeup) > try_to_wake_up_local(to_wakeup); > } > - /* > - * If this task has IO plugged, make sure it > - * gets flushed out to the devices before we go > - * to sleep > - */ > - blk_flush_plug(prev); > - BUG_ON(prev->plug && !list_empty(&prev->plug->list)); > - > deactivate_task(rq, prev, DEQUEUE_SLEEP); > } > switch_count = &prev->nvcsw; >
Right, so your new location is still under rq->lock for a number of architectures (including x86). finish_lock_switch() doesn't actually release the lock unless __ARCH_WANT_INTERRUPTS_ON_CTXSW || __ARCH_WANT_UNLOCKED_CTXSW (the former implies the latter since rq->lock is IRQ-safe).
If you want a safe place to drop rq->lock (but keep in mind to keep IRQs disabled there) and use prev, do something like the below. Both pre_schedule() and idle_balance() can already drop the rq->lock do doing it once more is quite all-right ;-)
Note that once you drop rq->lock prev->state can change to TASK_RUNNING again so don't re-check that.
--- kernel/sched.c | 6 +++++- 1 files changed, 5 insertions(+), 1 deletions(-)
diff --git a/kernel/sched.c b/kernel/sched.c index 655164e..99c5637 100644 --- a/kernel/sched.c +++ b/kernel/sched.c @@ -4120,8 +4120,12 @@ need_resched_nonpreemptible: switch_count = &prev->nvcsw; } + if (prev->state != TASK_RUNNING) { + raw_spin_unlock(&rq->lock); + blk_flush_plug(prev); + raw_spin_lock(&rq->lock); + } pre_schedule(rq, prev); - if (unlikely(!rq->nr_running)) idle_balance(cpu, rq);
| |