From: Qais Yousef <qais.yousef@arm.com>
To: Vincent Guittot <vincent.guittot@linaro.org>
Cc: Joel Fernandes <joel@joelfernandes.org>,
linux-kernel <linux-kernel@vger.kernel.org>,
Paul McKenney <paulmck@kernel.org>,
Frederic Weisbecker <fweisbec@gmail.com>,
Dietmar Eggeman <dietmar.eggemann@arm.com>,
Ben Segall <bsegall@google.com>,
Daniel Bristot de Oliveira <bristot@redhat.com>,
Ingo Molnar <mingo@redhat.com>,
Juri Lelli <juri.lelli@redhat.com>, Mel Gorman <mgorman@suse.de>,
Peter Zijlstra <peterz@infradead.org>,
Steven Rostedt <rostedt@goodmis.org>,
"Uladzislau Rezki (Sony)" <urezki@gmail.com>,
Neeraj upadhyay <neeraj.iitr10@gmail.com>
Subject: Re: [PATCH] sched/fair: Rate limit calls to update_blocked_averages() for NOHZ
Date: Wed, 3 Feb 2021 17:09:16 +0000 [thread overview]
Message-ID: <20210203170916.ows7d2b56t34i2w4@e107158-lin> (raw)
In-Reply-To: <20210129172727.GA30719@vingu-book>
On 01/29/21 18:27, Vincent Guittot wrote:
> The patch below moves the update of the blocked load of CPUs outside newidle_balance().
>
> Instead, the update is done with the usual idle load balance update. I'm working on an
> additonnal patch that will select this cpu that is about to become idle, instead of a
> random idle cpu but this 1st step fixe the problem of lot of update in newly idle.
>
> Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
> ---
> kernel/sched/fair.c | 32 +++-----------------------------
> 1 file changed, 3 insertions(+), 29 deletions(-)
>
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 197a51473e0c..8200b1d4df3d 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -7421,8 +7421,6 @@ enum migration_type {
> #define LBF_NEED_BREAK 0x02
> #define LBF_DST_PINNED 0x04
> #define LBF_SOME_PINNED 0x08
> -#define LBF_NOHZ_STATS 0x10
> -#define LBF_NOHZ_AGAIN 0x20
>
> struct lb_env {
> struct sched_domain *sd;
> @@ -8426,9 +8424,6 @@ static inline void update_sg_lb_stats(struct lb_env *env,
> for_each_cpu_and(i, sched_group_span(group), env->cpus) {
> struct rq *rq = cpu_rq(i);
>
> - if ((env->flags & LBF_NOHZ_STATS) && update_nohz_stats(rq, false))
> - env->flags |= LBF_NOHZ_AGAIN;
> -
> sgs->group_load += cpu_load(rq);
> sgs->group_util += cpu_util(i);
> sgs->group_runnable += cpu_runnable(rq);
> @@ -8969,11 +8964,6 @@ static inline void update_sd_lb_stats(struct lb_env *env, struct sd_lb_stats *sd
> struct sg_lb_stats tmp_sgs;
> int sg_status = 0;
>
> -#ifdef CONFIG_NO_HZ_COMMON
> - if (env->idle == CPU_NEWLY_IDLE && READ_ONCE(nohz.has_blocked))
> - env->flags |= LBF_NOHZ_STATS;
> -#endif
> -
> do {
> struct sg_lb_stats *sgs = &tmp_sgs;
> int local_group;
> @@ -9010,15 +9000,6 @@ static inline void update_sd_lb_stats(struct lb_env *env, struct sd_lb_stats *sd
> /* Tag domain that child domain prefers tasks go to siblings first */
> sds->prefer_sibling = child && child->flags & SD_PREFER_SIBLING;
>
> -#ifdef CONFIG_NO_HZ_COMMON
> - if ((env->flags & LBF_NOHZ_AGAIN) &&
> - cpumask_subset(nohz.idle_cpus_mask, sched_domain_span(env->sd))) {
> -
> - WRITE_ONCE(nohz.next_blocked,
> - jiffies + msecs_to_jiffies(LOAD_AVG_PERIOD));
> - }
> -#endif
> -
> if (env->sd->flags & SD_NUMA)
> env->fbq_type = fbq_classify_group(&sds->busiest_stat);
>
> @@ -10547,14 +10528,7 @@ static void nohz_newidle_balance(struct rq *this_rq)
> return;
>
> raw_spin_unlock(&this_rq->lock);
> - /*
> - * This CPU is going to be idle and blocked load of idle CPUs
> - * need to be updated. Run the ilb locally as it is a good
> - * candidate for ilb instead of waking up another idle CPU.
> - * Kick an normal ilb if we failed to do the update.
> - */
> - if (!_nohz_idle_balance(this_rq, NOHZ_STATS_KICK, CPU_NEWLY_IDLE))
Since we removed the call to this function (which uses this_rq)
> - kick_ilb(NOHZ_STATS_KICK);
> + kick_ilb(NOHZ_STATS_KICK);
And unconditionally call kick_ilb() which will find a suitable cpu to run the
lb at regardless what this_rq is.
Doesn't the below become unnecessary now?
10494 /*
10495 * This CPU doesn't want to be disturbed by scheduler
10496 * housekeeping
10497 */
10498 if (!housekeeping_cpu(this_cpu, HK_FLAG_SCHED))
10499 return;
10500
10501 /* Will wake up very soon. No time for doing anything else*/
10502 if (this_rq->avg_idle < sysctl_sched_migration_cost)
10503 return;
And we can drop this_rq arg altogether?
> raw_spin_lock(&this_rq->lock);
> }
>
> @@ -10616,8 +10590,6 @@ static int newidle_balance(struct rq *this_rq, struct rq_flags *rf)
> update_next_balance(sd, &next_balance);
> rcu_read_unlock();
>
> - nohz_newidle_balance(this_rq);
> -
> goto out;
> }
>
> @@ -10683,6 +10655,8 @@ static int newidle_balance(struct rq *this_rq, struct rq_flags *rf)
>
> if (pulled_task)
> this_rq->idle_stamp = 0;
> + else
> + nohz_newidle_balance(this_rq);
Since nohz_newidle_balance() will not do any real work now, I couldn't figure
out what moving this here achieves. Fault from my end to parse the change most
likely :-)
Joel can still test this patch as is of course. This is just an early review
since I already spent the time trying to understand it.
Thanks
--
Qais Yousef
>
> rq_repin_lock(this_rq, rf);
>
> --
> 2.17.1
next prev parent reply other threads:[~2021-02-03 17:10 UTC|newest]
Thread overview: 44+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-01-22 15:46 [PATCH] sched/fair: Rate limit calls to update_blocked_averages() for NOHZ Joel Fernandes (Google)
2021-01-22 16:56 ` Vincent Guittot
2021-01-22 18:39 ` Qais Yousef
2021-01-22 19:14 ` Joel Fernandes
2021-01-25 13:23 ` Vincent Guittot
2021-01-26 16:36 ` Qais Yousef
2021-01-22 19:10 ` Joel Fernandes
2021-01-25 10:44 ` Dietmar Eggemann
2021-01-25 17:30 ` Vincent Guittot
2021-01-25 17:53 ` Dietmar Eggemann
2021-01-25 14:42 ` Vincent Guittot
2021-01-27 18:43 ` Joel Fernandes
2021-01-28 13:57 ` Vincent Guittot
2021-01-28 15:09 ` Joel Fernandes
2021-01-28 16:57 ` Qais Yousef
[not found] ` <CAKfTPtBvwm9vZb5C=2oTF6N-Ht6Rvip4Lv18yi7O3G8e-_ZWdg@mail.gmail.com>
2021-01-29 17:27 ` Vincent Guittot
2021-02-03 11:54 ` Dietmar Eggemann
2021-02-03 13:12 ` Vincent Guittot
2021-02-04 9:47 ` Dietmar Eggemann
2021-02-03 17:09 ` Qais Yousef [this message]
2021-02-03 17:35 ` Vincent Guittot
2021-02-04 10:45 ` Qais Yousef
2021-02-03 19:56 ` Joel Fernandes
2021-03-23 21:37 ` Tim Chen
2021-03-24 13:44 ` Vincent Guittot
2021-03-24 16:05 ` Tim Chen
2021-04-07 14:02 ` Vincent Guittot
2021-04-07 17:19 ` Tim Chen
2021-04-08 14:51 ` Vincent Guittot
2021-04-08 23:05 ` Tim Chen
2021-04-09 15:26 ` Vincent Guittot
2021-04-09 17:59 ` Tim Chen
2021-05-10 21:59 ` Tim Chen
2021-05-11 15:25 ` Vincent Guittot
2021-05-11 17:25 ` Tim Chen
2021-05-11 17:56 ` Vincent Guittot
2021-05-12 13:59 ` Qais Yousef
2021-05-13 18:45 ` Tim Chen
2021-05-17 16:14 ` Qais Yousef
2021-06-11 20:00 ` Tim Chen
2021-06-18 10:28 ` Vincent Guittot
2021-06-18 16:14 ` Tim Chen
2021-06-25 8:50 ` Vincent Guittot
2021-02-01 15:13 ` Joel Fernandes
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20210203170916.ows7d2b56t34i2w4@e107158-lin \
--to=qais.yousef@arm.com \
--cc=bristot@redhat.com \
--cc=bsegall@google.com \
--cc=dietmar.eggemann@arm.com \
--cc=fweisbec@gmail.com \
--cc=joel@joelfernandes.org \
--cc=juri.lelli@redhat.com \
--cc=linux-kernel@vger.kernel.org \
--cc=mgorman@suse.de \
--cc=mingo@redhat.com \
--cc=neeraj.iitr10@gmail.com \
--cc=paulmck@kernel.org \
--cc=peterz@infradead.org \
--cc=rostedt@goodmis.org \
--cc=urezki@gmail.com \
--cc=vincent.guittot@linaro.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).