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

From: Scott Wood <swood@redhat.com>
To: Thomas Gleixner <tglx@linutronix.de>
Cc: Peter Zijlstra <peterz@infradead.org>,
	Frederic Weisbecker <frederic@kernel.org>,
	Ingo Molnar <mingo@kernel.org>,
	LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] timers/nohz: Update nohz load even if tick already stopped
Date: Fri, 08 Nov 2019 02:16:03 -0600	[thread overview]
Message-ID: <37e46985b60839190f65c764da7c2eb2877d53b9.camel@redhat.com> (raw)
In-Reply-To: <alpine.DEB.2.21.1911051048190.17054@nanos.tec.linutronix.de>

On Tue, 2019-11-05 at 10:53 +0100, Thomas Gleixner wrote:
> On Tue, 5 Nov 2019, Scott Wood wrote:
> > On Tue, 2019-11-05 at 00:43 +0100, Thomas Gleixner wrote:
> > > As Peter pointed out to me privately we should rather go and analyze
> > > the
> > > real thing instead of just applying duct tape.
> > > 
> > > /me drops the patch again.
> > 
> > The warning is due to kernel/sched/idle.c not updating curr-
> > >se.exec_start.
> > 
> > While debugging I noticed an issue with a particular load pattern.  The
> > CPU
> > goes non-nohz for a brief time at an interval very close to twice 
> > tick_period.  When the tick is started, the timer expiration is more
> > than
> > tick_period in the past, so hrtimer_forward() tries to catch up by
> > adding
> > 2*tick_period to the expiration.  Then the tick is stopped before that
> > new
> > expiration, and when the tick is woken up the expiry is again advanced
> > by
> > 2*tick_period with the timer never actually
> > running.  sched_tick_remote()
> > does fire every second, but there are streaks of several seconds where
> > it
> > keeps catching the CPU in a non-nohz state, so neither the normal nor
> > remote
> > ticks are calling calc_load_nohz_remote().
> > 
> > Is there a reason to not just remove the hrtimer_forward() from
> > tick_nohz_restart(), letting the timer fire if it's in the past, which
> > will
> > take care of doing hrtimer_forward()?
> 
> Well, no. tick_nohz_restart() can be invoked in a situation where the
> timer
> is armed for something in the far future (or completelt disabled) due to
> previously entering an estimated long idle (or user space execution on
> NOHZ_FULL) period.
> 
> That means if the timer is not canceled, realigned to the current tick and
> forwarded to the next due tick, the tick will not fire on time causing
> another sort of trouble.

That might be true of the expiry on entering tick_nohz_restart(), but it
shouldn't be true of ts->last_tick which the expiry is set to before calling
hrtimer_forward() -- and if it were, hrtimer_forward() is a no-op when the
expiry is in the future.

BTW, the name "last_tick" seems misleading as it's actually the saved
expiry, not the last time the tick ran.

-Scott