Re: [PATCH] timers/nohz: Update nohz load even if tick already stopped

[Scott Wood]

On Tue, 2019-10-29 at 11:05 +0100, Peter Zijlstra wrote:
> On Mon, Oct 28, 2019 at 04:07:16PM +0100, Frederic Weisbecker wrote:
> > From: Scott Wood <swood@xxxxxxxxxx>
> > 
> > The way loadavg is tracked during nohz only pays attention to the load
> > upon entering nohz. This can be particularly noticeable if nohz is
> > entered while non-idle, and then the cpu goes idle and stays that way
> > for
> > a long time. We've had reports of a loadavg near 150 on a mostly idle
> > system.
> > 
> > Calling calc_load_nohz_start() regardless of whether the tick is already
> > stopped addresses the issue when going idle. Tracking load changes when
> > not going idle (e.g. multiple SCHED_FIFO tasks coming and going) is not
> > addressed by this patch.
> 
> Hurph, is that phenomena you describe NOHZ or NOHZ_FULL? Because that
> second thing you talk about, multiple SCHED_FIFO tasks running without a
> tick is definitely NOHZ_FULL.
> 
> I'm thinking all of this is NOHZ_FULL because IIRC we always start the
> tick when there is a runnable task. So your example of going idle in
> NOHZ already cannot happen for regular NOHZ.

Yes, NOHZ_FULL (sorry for not stating that clearly).

> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index eb42b71faab9..209e50d48f80 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -3666,6 +3666,8 @@ static void sched_tick_remote(struct work_struct
> *work)
>  	 * having one too much is no big deal because the scheduler tick
> updates
>  	 * statistics and checks timeslices in a time-independent way,
> regardless
>  	 * of when exactly it is running.
> +	 *
> +	 * XXX should we be checking tick_nohz_tick_stopped_cpu() under rq-
> >lock ?
>  	 */
>  	if (idle_cpu(cpu) || !tick_nohz_tick_stopped_cpu(cpu))
>  		goto out_requeue;
> @@ -3686,6 +3688,7 @@ static void sched_tick_remote(struct work_struct
> *work)
>  	curr->sched_class->task_tick(rq, curr, 0);
>  
>  out_unlock:
> +	calc_load_nohz_remote(cpu);
>  	rq_unlock_irq(rq, &rf);

This gets skipped when the cpu is idle, so it still misses the update.

-Scott