Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932252AbbFBQ11 (ORCPT ); Tue, 2 Jun 2015 12:27:27 -0400 Received: from forward18m.cmail.yandex.net ([5.255.216.149]:50554 "EHLO forward18m.cmail.yandex.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1759459AbbFBQ10 (ORCPT ); Tue, 2 Jun 2015 12:27:26 -0400 From: Kirill Tkhai To: Peter Zijlstra Cc: "umgwanakikbuti@gmail.com" , "mingo@elte.hu" , "ktkhai@parallels.com" , "rostedt@goodmis.org" , "juri.lelli@gmail.com" , "pang.xunlei@linaro.org" , "oleg@redhat.com" , "linux-kernel@vger.kernel.org" , "luca.abeni@unitn.it" In-Reply-To: <20150602160724.GS18673@twins.programming.kicks-ass.net> References: <20150601135818.506080835@infradead.org> <20150601140839.852669281@infradead.org> <449191433253514@web6g.yandex.ru> <1433255230.1495.2.camel@twins> <20150602160724.GS18673@twins.programming.kicks-ass.net> Subject: Re: [RFC][PATCH 3/7] sched: Allow balance callbacks for check_class_changed() MIME-Version: 1.0 Message-Id: <249151433262439@web6m.yandex.ru> X-Mailer: Yamail [ http://yandex.ru ] 5.0 Date: Tue, 02 Jun 2015 19:27:19 +0300 Content-Transfer-Encoding: 8bit Content-Type: text/plain; charset=koi8-r Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5413 Lines: 150 ? ??, 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" : > > > > > > @@ -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 > Cc: Juri Lelli > Fixes: 40767b0dc768 ("sched/deadline: Fix deadline parameter modification handling") > Signed-off-by: Peter Zijlstra (Intel) > --- > 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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/