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

[Qais Yousef]

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