Re: [PATCH] sched/fair: Rate limit calls to update_blocked_averages() for NOHZ

Qais Yousef
2021-02-04T10:46:26Z

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