During tick restart, we use last_tick and forward it past now.
Since we are forwarding past now, we can simply use now as a reference
instead of last_tick. This patch removes last_tick and does so.
This patch potentially does more mul/imul than the existing code,
as sometimes forwarding past now need not be done if last_tick > now.
However, the patch is a cleanup which reduces LOC and reduces the size
of struct tick_sched.
Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
---
kernel/time/tick-sched.c | 7 ++-----
kernel/time/tick-sched.h | 1 -
kernel/time/timer_list.c | 1 -
3 files changed, 2 insertions(+), 7 deletions(-)
diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
index 71a792cd8936..52a4eda664cf 100644
--- a/kernel/time/tick-sched.c
+++ b/kernel/time/tick-sched.c
@@ -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);
if (tick_sched_flag_test(ts, TS_FLAG_HIGHRES)) {
hrtimer_start_expires(&ts->sched_timer,
@@ -1043,7 +1041,6 @@ static void tick_nohz_stop_tick(struct tick_sched *ts, int cpu)
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);
}
diff --git a/kernel/time/tick-sched.h b/kernel/time/tick-sched.h
index b4a7822f495d..7210cc473855 100644
--- a/kernel/time/tick-sched.h
+++ b/kernel/time/tick-sched.h
@@ -71,7 +71,6 @@ struct tick_sched {
/* Tick handling */
struct hrtimer sched_timer;
- ktime_t last_tick;
ktime_t next_tick;
unsigned long idle_jiffies;
ktime_t idle_waketime;
diff --git a/kernel/time/timer_list.c b/kernel/time/timer_list.c
index 1c311c46da50..26688a3b8ea8 100644
--- a/kernel/time/timer_list.c
+++ b/kernel/time/timer_list.c
@@ -154,7 +154,6 @@ static void print_cpu(struct seq_file *m, int cpu, u64 now)
struct tick_sched *ts = tick_get_tick_sched(cpu);
P_flag(nohz, TS_FLAG_NOHZ);
P_flag(highres, TS_FLAG_HIGHRES);
- P_ns(last_tick);
P_flag(tick_stopped, TS_FLAG_STOPPED);
P(idle_jiffies);
P(idle_calls);
--
2.47.0.277.g8800431eea-googLe Fri, Nov 08, 2024 at 05:48:34PM +0000, Joel Fernandes (Google) a écrit :
> During tick restart, we use last_tick and forward it past now.
>
> Since we are forwarding past now, we can simply use now as a reference
> instead of last_tick. This patch removes last_tick and does so.
>
> This patch potentially does more mul/imul than the existing code,
> as sometimes forwarding past now need not be done if last_tick > now.
Which is not uncommon if idle exited because of a non-timer interrupt
(remote wake up IPI or hardware interrupt).
It's also cheaper with hrtimer_forward() if now - last_tick < TICK_NSEC
which is not uncommon either if idle exited because of a wake-up from the tick
(schedule_timeout for example).
> However, the patch is a cleanup which reduces LOC and reduces the size
> of struct tick_sched.
Reducing the overhead of idle exit and consolidating its code within existing
forward API is more important than a per-cpu field.
>
> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> ---
> kernel/time/tick-sched.c | 7 ++-----
> kernel/time/tick-sched.h | 1 -
> kernel/time/timer_list.c | 1 -
> 3 files changed, 2 insertions(+), 7 deletions(-)
>
> diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
> index 71a792cd8936..52a4eda664cf 100644
> --- a/kernel/time/tick-sched.c
> +++ b/kernel/time/tick-sched.c
> @@ -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.
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);
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;
/*
Thanks!
On Tue, Nov 12, 2024 at 12:43:58AM +0100, Frederic Weisbecker wrote:
> Le Fri, Nov 08, 2024 at 05:48:34PM +0000, Joel Fernandes (Google) a écrit :
> > During tick restart, we use last_tick and forward it past now.
> >
> > Since we are forwarding past now, we can simply use now as a reference
> > instead of last_tick. This patch removes last_tick and does so.
> >
> > This patch potentially does more mul/imul than the existing code,
> > as sometimes forwarding past now need not be done if last_tick > now.
>
> Which is not uncommon if idle exited because of a non-timer interrupt
> (remote wake up IPI or hardware interrupt).
>
> It's also cheaper with hrtimer_forward() if now - last_tick < TICK_NSEC
> which is not uncommon either if idle exited because of a wake-up from the tick
> (schedule_timeout for example).
>
> > However, the patch is a cleanup which reduces LOC and reduces the size
> > of struct tick_sched.
>
> Reducing the overhead of idle exit and consolidating its code within existing
> forward API is more important than a per-cpu field.
>
> >
> > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> > ---
> > kernel/time/tick-sched.c | 7 ++-----
> > kernel/time/tick-sched.h | 1 -
> > kernel/time/timer_list.c | 1 -
> > 3 files changed, 2 insertions(+), 7 deletions(-)
> >
> > diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
> > index 71a792cd8936..52a4eda664cf 100644
> > --- a/kernel/time/tick-sched.c
> > +++ b/kernel/time/tick-sched.c
> > @@ -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?
> 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.
> 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.
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.
thanks,
- Joel
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
>
On Wed, Nov 13, 2024 at 01:40:17PM +0100, Frederic Weisbecker wrote:
> 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? :-)
I am leaning to dropping it due to the skew issues mentioned which is a
gaping hole. And also the debug usecase you mentioned. At least I appreciate
why this mechanism exists now :-) Thank you both :-)
thanks,
- Joel
On Tue, Nov 12 2024 at 00:43, Frederic Weisbecker wrote:
> Le Fri, Nov 08, 2024 at 05:48:34PM +0000, Joel Fernandes (Google) a écrit :
>> During tick restart, we use last_tick and forward it past now.
>>
>> Since we are forwarding past now, we can simply use now as a reference
>> instead of last_tick. This patch removes last_tick and does so.
>>
>> This patch potentially does more mul/imul than the existing code,
>> as sometimes forwarding past now need not be done if last_tick > now.
>> However, the patch is a cleanup which reduces LOC and reduces the size
>> of struct tick_sched.
May I politely ask you to read and follow the Documentation
vs. changelogs?
https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#changelog
Also
git grep 'This patch' Documentation/process
might give you a hint.
>> - /* 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);
How is a division and multiplication in this hotpath helpful? That's
awfully slow on 32-bit machines and pointless on 64-bit too.
Using now is also wrong as it breaks the sched_skew_tick distribution by
aligning the tick on all CPUs again.
IOW, this "cleanup" is making things worse.
> We don't want to rewrite hrtimer_forward() but, after all, the current expiry is
> enough a relevant information.
>
> 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);
That's just wrong. ts->sched_timer.expires contains a tick in the
future. If tick_nohz_stop_tick() set it to 10 ticks in the future and
the CPU goes out of idle due to a device interrupt before the timer
expires, then hrtimer_forward() will do nothing because expires is ahead
of now.
Which means the CPU is not idle and has no tick until the delayed tick
which was set by tick_nohz_stop_tick() expires. Not really correct.
Thanks,
tglx
On Tue, Nov 12, 2024 at 02:46:23PM +0100, Thomas Gleixner wrote:
> On Tue, Nov 12 2024 at 00:43, Frederic Weisbecker wrote:
> > Le Fri, Nov 08, 2024 at 05:48:34PM +0000, Joel Fernandes (Google) a écrit :
>
> >> During tick restart, we use last_tick and forward it past now.
> >>
> >> Since we are forwarding past now, we can simply use now as a reference
> >> instead of last_tick. This patch removes last_tick and does so.
> >>
> >> This patch potentially does more mul/imul than the existing code,
> >> as sometimes forwarding past now need not be done if last_tick > now.
> >> However, the patch is a cleanup which reduces LOC and reduces the size
> >> of struct tick_sched.
>
> May I politely ask you to read and follow the Documentation
> vs. changelogs?
>
> https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#changelog
>
> Also
>
> git grep 'This patch' Documentation/process
>
> might give you a hint.
Oops, sorry. I will go read that again. My bad.
> >> - /* 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);
>
> How is a division and multiplication in this hotpath helpful? That's
> awfully slow on 32-bit machines and pointless on 64-bit too.
Yes, I was afraid of that but also hrtimer_forward() already does div and
mult:
if (unlikely(delta >= interval)) {
s64 incr = ktime_to_ns(interval);
orun = ktime_divns(delta, incr);
hrtimer_add_expires_ns(timer, incr * orun);
I am not fully sure if I am doing division and multiplication more often than
existing code (I'll go count that), because tick should not be stopped at a
distance of just 1 tick I think (otherwise why stop it in the first place..).
> Using now is also wrong as it breaks the sched_skew_tick distribution by
> aligning the tick on all CPUs again.
I am not very familiar with that so I'll do some research on it, thanks!
> IOW, this "cleanup" is making things worse.
Sorry and thanks for filling me in on the drawbacks of this. One of the goal
of this particular change I posted is to learn "why not" and this really
helped, thanks!
> > We don't want to rewrite hrtimer_forward() but, after all, the current expiry is
> > enough a relevant information.
> >
> > 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);
>
> That's just wrong. ts->sched_timer.expires contains a tick in the
> future. If tick_nohz_stop_tick() set it to 10 ticks in the future and
> the CPU goes out of idle due to a device interrupt before the timer
> expires, then hrtimer_forward() will do nothing because expires is ahead
> of now.
>
> Which means the CPU is not idle and has no tick until the delayed tick
> which was set by tick_nohz_stop_tick() expires. Not really correct.
I agree, Frederic's suggestion will break as we have to reset the hrtimer back
to reality.
thanks,
- Joel
Le Tue, Nov 12, 2024 at 02:46:23PM +0100, Thomas Gleixner a écrit :
> On Tue, Nov 12 2024 at 00:43, Frederic Weisbecker wrote:
> > Le Fri, Nov 08, 2024 at 05:48:34PM +0000, Joel Fernandes (Google) a écrit :
>
> >> During tick restart, we use last_tick and forward it past now.
> >>
> >> Since we are forwarding past now, we can simply use now as a reference
> >> instead of last_tick. This patch removes last_tick and does so.
> >>
> >> This patch potentially does more mul/imul than the existing code,
> >> as sometimes forwarding past now need not be done if last_tick > now.
> >> However, the patch is a cleanup which reduces LOC and reduces the size
> >> of struct tick_sched.
>
> May I politely ask you to read and follow the Documentation
> vs. changelogs?
>
> https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#changelog
>
> Also
>
> git grep 'This patch' Documentation/process
>
> might give you a hint.
>
> >> - /* 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);
>
> How is a division and multiplication in this hotpath helpful? That's
> awfully slow on 32-bit machines and pointless on 64-bit too.
>
> Using now is also wrong as it breaks the sched_skew_tick distribution by
> aligning the tick on all CPUs again.
>
> IOW, this "cleanup" is making things worse.
>
> > We don't want to rewrite hrtimer_forward() but, after all, the current expiry is
> > enough a relevant information.
> >
> > 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);
>
> That's just wrong. ts->sched_timer.expires contains a tick in the
> future. If tick_nohz_stop_tick() set it to 10 ticks in the future and
> the CPU goes out of idle due to a device interrupt before the timer
> expires, then hrtimer_forward() will do nothing because expires is ahead
> of now.
>
> Which means the CPU is not idle and has no tick until the delayed tick
> which was set by tick_nohz_stop_tick() expires. Not really correct.
Bah! Yes of course...
>
> Thanks,
>
> tglx
© 2016 - 2024 Red Hat, Inc.