2021-02-05 11:55:59

by Vincent Guittot

Subject: [PATCH 4/6] sched/fair: reorder newidle_balance pulled_task test

Reorder the tests and skip prevent useless test when no load balance has
been performed.

Signed-off-by: Vincent Guittot <[email protected]>
---
kernel/sched/fair.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index c587af230010..935594cd5430 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -10592,7 +10592,6 @@ static int newidle_balance(struct rq *this_rq, struct rq_flags *rf)
if (curr_cost > this_rq->max_idle_balance_cost)
this_rq->max_idle_balance_cost = curr_cost;

-out:
/*
* While browsing the domains, we released the rq lock, a task could
* have been enqueued in the meantime. Since we're not going idle,
@@ -10601,14 +10600,15 @@ static int newidle_balance(struct rq *this_rq, struct rq_flags *rf)
if (this_rq->cfs.h_nr_running && !pulled_task)
pulled_task = 1;

- /* Move the next balance forward */
- if (time_after(this_rq->next_balance, next_balance))
- this_rq->next_balance = next_balance;
-
/* Is there a task of a high priority class? */
if (this_rq->nr_running != this_rq->cfs.h_nr_running)
pulled_task = -1;

+out:
+ /* Move the next balance forward */
+ if (time_after(this_rq->next_balance, next_balance))
+ this_rq->next_balance = next_balance;
+
if (pulled_task)
this_rq->idle_stamp = 0;
else
--
2.17.1

2021-02-09 13:50:22

by Dietmar Eggemann

Subject: Re: [PATCH 4/6] sched/fair: reorder newidle_balance pulled_task test

On 05/02/2021 12:48, Vincent Guittot wrote:
> Reorder the tests and skip prevent useless test when no load balance has
> been performed.

LGTM.

But IMHO the reason why those two if conditions can be skipped for the
'goto out' path is that we don't release the rq lock rather the actual
lb. Might be worth saying this in the patch header? It's already
mentioned on top of the first if condition though.

> Signed-off-by: Vincent Guittot <[email protected]>
> ---
> kernel/sched/fair.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index c587af230010..935594cd5430 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -10592,7 +10592,6 @@ static int newidle_balance(struct rq *this_rq, struct rq_flags *rf)
> if (curr_cost > this_rq->max_idle_balance_cost)
> this_rq->max_idle_balance_cost = curr_cost;
>
> -out:
> /*
> * While browsing the domains, we released the rq lock, a task could
> * have been enqueued in the meantime. Since we're not going idle,
> @@ -10601,14 +10600,15 @@ static int newidle_balance(struct rq *this_rq, struct rq_flags *rf)
> if (this_rq->cfs.h_nr_running && !pulled_task)
> pulled_task = 1;
>
> - /* Move the next balance forward */
> - if (time_after(this_rq->next_balance, next_balance))
> - this_rq->next_balance = next_balance;
> -
> /* Is there a task of a high priority class? */
> if (this_rq->nr_running != this_rq->cfs.h_nr_running)
> pulled_task = -1;
>
> +out:
> + /* Move the next balance forward */
> + if (time_after(this_rq->next_balance, next_balance))
> + this_rq->next_balance = next_balance;
> +
> if (pulled_task)
> this_rq->idle_stamp = 0;
> else
>