On Wed, May 29, 2019 at 08:36:44PM +0000, Vineeth Remanan Pillai wrote:
> From: Peter Zijlstra <pet...@infradead.org>
>
> Avoid the RETRY_TASK case in the pick_next_task() slow path.
>
> By doing the put_prev_task() early, we get the rt/deadline pull done,
> and by testing rq->nr_running we know if we need newidle_balance().
>
> This then gives a stable state to pick a task from.
>
> Since the fast-path is fair only; it means the other classes will
> always have pick_next_task(.prev=NULL, .rf=NULL) and we can simplify.
>
> Signed-off-by: Peter Zijlstra (Intel) <pet...@infradead.org>
> ---
> kernel/sched/core.c | 19 ++++++++++++-------
> kernel/sched/deadline.c | 30 ++----------------------------
> kernel/sched/fair.c | 9 ++++++---
> kernel/sched/idle.c | 4 +++-
> kernel/sched/rt.c | 29 +----------------------------
> kernel/sched/sched.h | 13 ++++++++-----
> kernel/sched/stop_task.c | 3 ++-
> 7 files changed, 34 insertions(+), 73 deletions(-)
>
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index 9dfa0c53deb3..b883c70674ba 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -3363,7 +3363,7 @@ pick_next_task(struct rq *rq, struct task_struct *prev,
> struct rq_flags *rf)
>
> p = fair_sched_class.pick_next_task(rq, prev, rf);
> if (unlikely(p == RETRY_TASK))
> - goto again;
> + goto restart;
>
> /* Assumes fair_sched_class->next == idle_sched_class */
> if (unlikely(!p))
> @@ -3372,14 +3372,19 @@ pick_next_task(struct rq *rq, struct task_struct
> *prev, struct rq_flags *rf)
> return p;
> }
>
> -again:
> +restart:
> + /*
> + * Ensure that we put DL/RT tasks before the pick loop, such that they
> + * can PULL higher prio tasks when we lower the RQ 'priority'.
> + */
> + prev->sched_class->put_prev_task(rq, prev, rf);
> + if (!rq->nr_running)
> + newidle_balance(rq, rf);
> +
> for_each_class(class) {
> - p = class->pick_next_task(rq, prev, rf);
> - if (p) {
> - if (unlikely(p == RETRY_TASK))
> - goto again;
> + p = class->pick_next_task(rq, NULL, NULL);
> + if (p)
> return p;
> - }
> }
>
> /* The idle class should always have a runnable task: */
> diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
> index 45425f971eec..d3904168857a 100644
> --- a/kernel/sched/deadline.c
> +++ b/kernel/sched/deadline.c
> @@ -1729,39 +1729,13 @@ pick_next_task_dl(struct rq *rq, struct task_struct
> *prev, struct rq_flags *rf)
> struct task_struct *p;
> struct dl_rq *dl_rq;
>
> - dl_rq = &rq->dl;
> -
> - if (need_pull_dl_task(rq, prev)) {
> - /*
> - * This is OK, because current is on_cpu, which avoids it being
> - * picked for load-balance and preemption/IRQs are still
> - * disabled avoiding further scheduler activity on it and we're
> - * being very careful to re-start the picking loop.
> - */
> - rq_unpin_lock(rq, rf);
> - pull_dl_task(rq);
> - rq_repin_lock(rq, rf);
> - /*
> - * pull_dl_task() can drop (and re-acquire) rq->lock; this
> - * means a stop task can slip in, in which case we need to
> - * re-start task selection.
> - */
> - if (rq->stop && task_on_rq_queued(rq->stop))
> - return RETRY_TASK;
> - }
> + WARN_ON_ONCE(prev || rf);
should there be a helpful message to go with this warning?
>
> - /*
> - * When prev is DL, we may throttle it in put_prev_task().
> - * So, we update time before we check for dl_nr_running.
> - */
> - if (prev->sched_class == &dl_sched_class)
> - update_curr_dl(rq);
> + dl_rq = &rq->dl;
>
> if (unlikely(!dl_rq->dl_nr_running))
> return NULL;
>
> - put_prev_task(rq, prev);
> -
> dl_se = pick_next_dl_entity(rq, dl_rq);
> BUG_ON(!dl_se);
>
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 8e3eb243fd9f..e65f2dfda77a 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -6979,7 +6979,7 @@ pick_next_task_fair(struct rq *rq, struct task_struct
> *prev, struct rq_flags *rf
> goto idle;
>
> #ifdef CONFIG_FAIR_GROUP_SCHED
> - if (prev->sched_class != &fair_sched_class)
> + if (!prev || prev->sched_class != &fair_sched_class)
> goto simple;
>
> /*
> @@ -7056,8 +7056,8 @@ pick_next_task_fair(struct rq *rq, struct task_struct
> *prev, struct rq_flags *rf
> goto done;
> simple:
> #endif
> -
> - put_prev_task(rq, prev);
> + if (prev)
> + put_prev_task(rq, prev);
>
> do {
> se = pick_next_entity(cfs_rq, NULL);
> @@ -7085,6 +7085,9 @@ done: __maybe_unused;
> return p;
>
> idle:
> + if (!rf)
> + return NULL;
> +
> new_tasks = newidle_balance(rq, rf);
>
> /*
> diff --git a/kernel/sched/idle.c b/kernel/sched/idle.c
> index 1b65a4c3683e..7ece8e820b5d 100644
> --- a/kernel/sched/idle.c
> +++ b/kernel/sched/idle.c
> @@ -388,7 +388,9 @@ pick_next_task_idle(struct rq *rq, struct task_struct
> *prev, struct rq_flags *rf
> {
> struct task_struct *next = rq->idle;
>
> - put_prev_task(rq, prev);
> + if (prev)
> + put_prev_task(rq, prev);
> +
> set_next_task_idle(rq, next);
>
> return next;
> diff --git a/kernel/sched/rt.c b/kernel/sched/rt.c
> index 51ee87c5a28a..79f2e60516ef 100644
> --- a/kernel/sched/rt.c
> +++ b/kernel/sched/rt.c
> @@ -1554,38 +1554,11 @@ pick_next_task_rt(struct rq *rq, struct task_struct
> *prev, struct rq_flags *rf)
> struct task_struct *p;
> struct rt_rq *rt_rq = &rq->rt;
>
> - if (need_pull_rt_task(rq, prev)) {
> - /*
> - * This is OK, because current is on_cpu, which avoids it being
> - * picked for load-balance and preemption/IRQs are still
> - * disabled avoiding further scheduler activity on it and we're
> - * being very careful to re-start the picking loop.
> - */
> - rq_unpin_lock(rq, rf);
> - pull_rt_task(rq);
> - rq_repin_lock(rq, rf);
> - /*
> - * pull_rt_task() can drop (and re-acquire) rq->lock; this
> - * means a dl or stop task can slip in, in which case we need
> - * to re-start task selection.
> - */
> - if (unlikely((rq->stop && task_on_rq_queued(rq->stop)) ||
> - rq->dl.dl_nr_running))
> - return RETRY_TASK;
> - }
> -
> - /*
> - * We may dequeue prev's rt_rq in put_prev_task().
> - * So, we update time before rt_queued check.
> - */
> - if (prev->sched_class == &rt_sched_class)
> - update_curr_rt(rq);
> + WARN_ON_ONCE(prev || rf);
>
> if (!rt_rq->rt_queued)
> return NULL;
>
> - put_prev_task(rq, prev);
> -
> p = _pick_next_task_rt(rq);
>
> set_next_task_rt(rq, p);
> diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
> index 4cbe2bef92e4..460dd04e76af 100644
> --- a/kernel/sched/sched.h
> +++ b/kernel/sched/sched.h
> @@ -1665,12 +1665,15 @@ struct sched_class {
> void (*check_preempt_curr)(struct rq *rq, struct task_struct *p, int
> flags);
>
> /*
> - * It is the responsibility of the pick_next_task() method that will
> - * return the next task to call put_prev_task() on the @prev task or
> - * something equivalent.
> + * Both @prev and @rf are optional and may be NULL, in which case the
> + * caller must already have invoked put_prev_task(rq, prev, rf).
> *
> - * May return RETRY_TASK when it finds a higher prio class has runnable
> - * tasks.
> + * Otherwise it is the responsibility of the pick_next_task() to call
> + * put_prev_task() on the @prev task or something equivalent, IFF it
> + * returns a next task.
> + *
> + * In that case (@rf != NULL) it may return RETRY_TASK when it finds a
> + * higher prio class has runnable tasks.
> */
> struct task_struct * (*pick_next_task)(struct rq *rq,
> struct task_struct *prev,
> diff --git a/kernel/sched/stop_task.c b/kernel/sched/stop_task.c
> index 8f414018d5e0..7e1cee4e65b2 100644
> --- a/kernel/sched/stop_task.c
> +++ b/kernel/sched/stop_task.c
> @@ -33,10 +33,11 @@ pick_next_task_stop(struct rq *rq, struct task_struct
> *prev, struct rq_flags *rf
> {
> struct task_struct *stop = rq->stop;
>
> + WARN_ON_ONCE(prev || rf);
should there be a helpful message to go with this warning?
--mark
> +
> if (!stop || !task_on_rq_queued(stop))
> return NULL;
>
> - put_prev_task(rq, prev);
> set_next_task_stop(rq, stop);
>
> return stop;
> --
> 2.17.1
>
