В Вт, 02/06/2015 в 18:07 +0200, Peter Zijlstra пишет: On Tue, Jun 02, 2015 at 04:27:10PM +0200, Peter Zijlstra wrote: > > On Tue, 2015-06-02 at 16:58 +0300, Kirill Tkhai wrote: > > > 01.06.2015, 17:13, "Peter Zijlstra" <[email protected]>: > > > > > > @@ -1010,7 +1014,7 @@ static inline void check_class_changed(s > > > > if (prev_class != p->sched_class) { > > > > if (prev_class->switched_from) > > > > prev_class->switched_from(rq, p); > > > > - /* Possble rq->lock 'hole'. */ > > > > + > > > > > > But switched_from_dl()->cancel_dl_timer() still unlocks rq->lock. > > > > > > It seems we should drop it (cancel_dl_timer) and move hrtimer_cancel() > > > from switched_from_dl() to finish_task_switch(). It will be executed > > > for all classes and completely take the functionality we implement > > > cancel_dl_timer() for. > > > > *groan* yes.. I don't like moving it into generic code though. > > > > more thinking required. > > How about something like so then? > > --- > Subject: sched,deadline: Fix sched class hopping CBS hole > > We still have a few pending issues with the deadline code, one of which > is that switching between scheduling classes can 'leak' CBS state. > > Close the hole by retaining the current CBS state when leaving > SCHED_DEADLINE and unconditionally programming the deadline timer. > > The timer will then reset the CBS state if the task is still > !SCHED_DEADLINE by the time it hits. > > If the task were to die, task_dead_dl() will cancel the timer, so no > lingering state. > > Cc: Luca Abeni <[email protected]> > Cc: Juri Lelli <[email protected]> > Fixes: 40767b0dc768 ("sched/deadline: Fix deadline parameter modification > handling") > Signed-off-by: Peter Zijlstra (Intel) <[email protected]> > --- > kernel/sched/deadline.c | 71 > ++++++++++++++++++++----------------------------- > 1 file changed, 29 insertions(+), 42 deletions(-) > > diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c > index 7336fe5fea30..625ed74f1467 100644 > --- a/kernel/sched/deadline.c > +++ b/kernel/sched/deadline.c > @@ -572,20 +572,26 @@ static enum hrtimer_restart dl_task_timer(struct > hrtimer *timer) > rq = task_rq_lock(p, &flags); > > /* > - * We need to take care of several possible races here: > - * > - * - the task might have changed its scheduling policy > - * to something different than SCHED_DEADLINE > - * - the task might have changed its reservation parameters > - * (through sched_setattr()) > - * - the task might have been boosted by someone else and > - * might be in the boosting/deboosting path > - * > - * In all this cases we bail out, as the task is already > - * in the runqueue or is going to be enqueued back anyway. > + * The task might have changed its scheduling policy to something > + * different than SCHED_DEADLINE (through switched_fromd_dl()). > */ > - if (!dl_task(p) || dl_se->dl_new || > - dl_se->dl_boosted || !dl_se->dl_throttled) > + if (!dl_task(p)) { > + __dl_clear_params(p); > + goto unlock; > + } > + > + /* > + * There's only two ways to have dl_new set; 1) __sched_fork(), and a > + * fresh task should not have a pending timer, and 2) the above, which > + * does not get here. > + */ > + WARN_ON_ONCE(dl_se->dl_new); > + > + /* > + * The task might have been boosted by someone else and might be in the > + * boosting/deboosting path, its not throttled. > + */ > + if (dl_se->dl_boosted || !dl_se->dl_throttled) > goto unlock; > > sched_clock_tick(); > @@ -1683,37 +1689,18 @@ void __init init_sched_dl_class(void) > > #endif /* CONFIG_SMP */ > > -/* > - * Ensure p's dl_timer is cancelled. May drop rq->lock for a while. > - */ > -static void cancel_dl_timer(struct rq *rq, struct task_struct *p) > -{ > - struct hrtimer *dl_timer = &p->dl.dl_timer; > - > - /* Nobody will change task's class if pi_lock is held */ > - lockdep_assert_held(&p->pi_lock); > - > - if (hrtimer_active(dl_timer)) { > - int ret = hrtimer_try_to_cancel(dl_timer); > - > - if (unlikely(ret == -1)) { > - /* > - * Note, p may migrate OR new deadline tasks > - * may appear in rq when we are unlocking it. > - * A caller of us must be fine with that. > - */ > - raw_spin_unlock(&rq->lock); > - hrtimer_cancel(dl_timer); > - raw_spin_lock(&rq->lock); > - } > - } > -} > - > static void switched_from_dl(struct rq *rq, struct task_struct *p) > { > - /* XXX we should retain the bw until 0-lag */ > - cancel_dl_timer(rq, p); > - __dl_clear_params(p); > + /* > + * Start the deadline timer; if we switch back to dl before this we'll > + * continue consuming our current CBS slice. If we stay outside of > + * SCHED_DEADLINE until the deadline passes, the timer will reset the > + * task. > + * > + * task_dead_dl() will cancel our timer if we happen to die while > + * its still pending.
task_dead_dl() is called for tasks of deadline class only. So if we do that, the timer may be executed after final task's dead. > + */ > + start_dl_timer(&p->dl, false); > > /* > * Since this might be the only -deadline task on the rq, > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
