Re: [RFC 1/3] tick-sched: Remove last_tick and calculate next tick from now

[Frederic Weisbecker <frederic@kernel.org>]

Le Tue, Nov 12, 2024 at 06:33:30PM +0000, Joel Fernandes a écrit :
> On Tue, Nov 12, 2024 at 12:43:58AM +0100, Frederic Weisbecker wrote:
> > > @@ -837,11 +837,9 @@ EXPORT_SYMBOL_GPL(get_cpu_iowait_time_us);
> > >  
> > >  static void tick_nohz_restart(struct tick_sched *ts, ktime_t now)
> > >  {
> > > +	/* Set the time to expire on the next tick and not some far away future. */
> > >  	hrtimer_cancel(&ts->sched_timer);
> > > -	hrtimer_set_expires(&ts->sched_timer, ts->last_tick);
> > > -
> > > -	/* Forward the time to expire in the future */
> > > -	hrtimer_forward(&ts->sched_timer, now, TICK_NSEC);
> > > +	hrtimer_set_expires(&ts->sched_timer, DIV_ROUND_UP_ULL(now, TICK_NSEC) * TICK_NSEC);
> > 
> > We don't want to rewrite hrtimer_forward() but, after all, the current expiry is
> > enough a relevant information.
> 
> Thanks, do you envision any way we can get past the sched_skew_tick issue
> Thomas mentioned, if we still want to do something like this patch?

First, do we still want to do something like this patch? :-)

> 
> > How about just this? It's worth it as it now forwards after the real last programmed
> > tick, which should be close enough from @now with a delta below TICK_NSEC, or even
> > better @now is below the expiry. Therefore it should resume as just a no-op
> > or at worst an addition within hrtimer_forward():
> > 
> > diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
> > index 753a184c7090..ffd0c026a248 100644
> > --- a/kernel/time/tick-sched.c
> > +++ b/kernel/time/tick-sched.c
> > @@ -838,7 +838,6 @@ EXPORT_SYMBOL_GPL(get_cpu_iowait_time_us);
> >  static void tick_nohz_restart(struct tick_sched *ts, ktime_t now)
> >  {
> >  	hrtimer_cancel(&ts->sched_timer);
> > -	hrtimer_set_expires(&ts->sched_timer, ts->last_tick);
> >  
> >  	/* Forward the time to expire in the future */
> >  	hrtimer_forward(&ts->sched_timer, now, TICK_NSEC);
> 
> For completeness, as we discussed on other thread and Thomas mentioned, we
> break code if doing this.

Right!

> 
> > As for removing last_tick, I think it's a precious debugging information. But
> > it's lagging behind the record of the first time only the tick got stopped within
> > the last trip to idle. So it could become this instead:
> > 
> > diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
> > index 753a184c7090..af013f7733b2 100644
> > --- a/kernel/time/tick-sched.c
> > +++ b/kernel/time/tick-sched.c
> > @@ -1042,12 +1041,11 @@ static void tick_nohz_stop_tick(struct tick_sched *ts, int cpu)
> >  	if (!tick_sched_flag_test(ts, TS_FLAG_STOPPED)) {
> >  		calc_load_nohz_start();
> >  		quiet_vmstat();
> > -
> > -		ts->last_tick = hrtimer_get_expires(&ts->sched_timer);
> >  		tick_sched_flag_set(ts, TS_FLAG_STOPPED);
> >  		trace_tick_stop(1, TICK_DEP_MASK_NONE);
> >  	}
> >  
> > +	ts->last_tick = hrtimer_get_expires(&ts->sched_timer);
> >  	ts->next_tick = expires;
> 
> Are you suggesting we roll this part of your diff into a new patch (to
> improve debug)? I could do that with attribution to you. But I guess I don't
> understand this particular part of your diff.

No there is no point in doing this after all. I was trying to find a point for
this ->last_tick existence but there was one I overlooked like Thomas explained.

> If the tick was already stopped, how does
> hrtimer_get_expires(&ts->sched_timer) change since the last time the tick was
> stopped? ->last_tick should be set only when the tick was last running and a
> stop was attempted? Otherwise your diff might set ->last_tick well into the
> future after the tick was already stopped, AFAICS.

Right.

Thanks.

> 
> thanks,
> 
>  - Joel
>