On 02/03/21 18:35, Vincent Guittot wrote: > > > 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? > > The end goal is to keep running on this cpu that is about to become idle. > > This patch is mainly there to check that Joel's problem disappears if > the update of the blocked load of the cpus is not done in the > newidle_balance context. I'm preparing few other patches on top to > clean up the full path
+1 > > > > 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 :-) > > The goal is to schedule the update only if we are about to be idle and > nothing else has been queued in the meantime I see. This short coming already existed and not *strictly* related to moving update of blocked load out of newidle balance. Thanks -- Qais Yousef
