Re: [RFC][PATCH 3/7] sched: Allow balance callbacks for check_class_changed()

[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" <peterz@xxxxxxxxxxxxx>:
> 
> > > @@ -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 <luca.abeni@xxxxxxxx>
Cc: Juri Lelli <juri.lelli@xxxxxxx>
Fixes: 40767b0dc768 ("sched/deadline: Fix deadline parameter modification handling")
Signed-off-by: Peter Zijlstra (Intel) <peterz@xxxxxxxxxxxxx>
---
 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.
+	 */
+	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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/