Date:	Tue, 02 Jun 2015 19:27:19 +0300
From:	Kirill Tkhai <tkhai@...dex.ru>
To:	Peter Zijlstra <peterz@...radead.org>
Cc:	"umgwanakikbuti@...il.com" <umgwanakikbuti@...il.com>,
	"mingo@...e.hu" <mingo@...e.hu>,
	"ktkhai@...allels.com" <ktkhai@...allels.com>,
	"rostedt@...dmis.org" <rostedt@...dmis.org>,
	"juri.lelli@...il.com" <juri.lelli@...il.com>,
	"pang.xunlei@...aro.org" <pang.xunlei@...aro.org>,
	"oleg@...hat.com" <oleg@...hat.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"luca.abeni@...tn.it" <luca.abeni@...tn.it>
Subject: Re: [RFC][PATCH 3/7] sched: Allow balance callbacks for check_class_changed()

В Вт, 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" <peterz@...radead.org>:
> > 
> > > > @@ -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@...tn.it>
> Cc: Juri Lelli <juri.lelli@....com>
> Fixes: 40767b0dc768 ("sched/deadline: Fix deadline parameter modification handling")
> Signed-off-by: Peter Zijlstra (Intel) <peterz@...radead.org>
> ---
>  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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists