[PATCH 0/3] tick/nohz cleanups

[PATCH 0/3] tick/nohz cleanups

[Frederic Weisbecker]

After a few exchanges with Joel I realized that some things in tick/nohz
are really not welcoming reviewers. Here's just a few cleanups.

Frederic Weisbecker (3):
  tick/nohz: Rename the tick handlers to more self-explanatory names
  tick/nohz: Update obsolete comments
  tick/nohz: Don't shutdown the lowres tick from itself

 kernel/time/tick-sched.c | 78 +++++++++++++++++++++++++++-------------
 1 file changed, 53 insertions(+), 25 deletions(-)

-- 
2.34.1

[PATCH 1/3] tick/nohz: Rename the tick handlers to more self-explanatory names

[Frederic Weisbecker]

The current name of the tick handlers don't tell much about what differ
between them. Use names that better reflect their role and resolution.

Signed-off-by: Frederic Weisbecker <frederic@kernel.org>
---
 kernel/time/tick-sched.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
index 89055050d1ac..a7b5960258f2 100644
--- a/kernel/time/tick-sched.c
+++ b/kernel/time/tick-sched.c
@@ -1355,7 +1355,7 @@ void tick_nohz_idle_exit(void)
 /*
  * The nohz low res interrupt handler
  */
-static void tick_nohz_handler(struct clock_event_device *dev)
+static void tick_lowres_handler(struct clock_event_device *dev)
 {
 	struct tick_sched *ts = this_cpu_ptr(&tick_cpu_sched);
 	struct pt_regs *regs = get_irq_regs();
@@ -1401,7 +1401,7 @@ static void tick_nohz_switch_to_nohz(void)
 	if (!tick_nohz_enabled)
 		return;
 
-	if (tick_switch_to_oneshot(tick_nohz_handler))
+	if (tick_switch_to_oneshot(tick_lowres_handler))
 		return;
 
 	/*
@@ -1464,7 +1464,7 @@ void tick_irq_enter(void)
  * We rearm the timer until we get disabled by the idle code.
  * Called with interrupts disabled.
  */
-static enum hrtimer_restart tick_sched_timer(struct hrtimer *timer)
+static enum hrtimer_restart tick_highres_handler(struct hrtimer *timer)
 {
 	struct tick_sched *ts =
 		container_of(timer, struct tick_sched, sched_timer);
@@ -1513,7 +1513,7 @@ void tick_setup_sched_timer(void)
 	 * Emulate tick processing via per-CPU hrtimers:
 	 */
 	hrtimer_init(&ts->sched_timer, CLOCK_MONOTONIC, HRTIMER_MODE_ABS_HARD);
-	ts->sched_timer.function = tick_sched_timer;
+	ts->sched_timer.function = tick_highres_handler;
 
 	/* Get the next period (per-CPU) */
 	hrtimer_set_expires(&ts->sched_timer, tick_init_jiffy_update());
-- 
2.34.1

[PATCH 2/3] tick/nohz: Update obsolete comments

[Frederic Weisbecker]

Some comments are obsolete enough to assume that IRQ exit restarts the
tick in idle or RCU is turned on at the same time as the tick, among
other details.

Update them and add more.

Signed-off-by: Frederic Weisbecker <frederic@kernel.org>
---
 kernel/time/tick-sched.c | 46 +++++++++++++++++++++++++++++++---------
 1 file changed, 36 insertions(+), 10 deletions(-)

diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
index a7b5960258f2..dd25da8531f4 100644
--- a/kernel/time/tick-sched.c
+++ b/kernel/time/tick-sched.c
@@ -1164,12 +1164,23 @@ void tick_nohz_idle_enter(void)
 }
 
 /**
- * tick_nohz_irq_exit - update next tick event from interrupt exit
+ * tick_nohz_irq_exit - Notify the tick about IRQ exit
  *
- * When an interrupt fires while we are idle and it doesn't cause
- * a reschedule, it may still add, modify or delete a timer, enqueue
- * an RCU callback, etc...
- * So we need to re-calculate and reprogram the next tick event.
+ * A timer may have been added/modified/deleted either by the current IRQ,
+ * or by another place using this IRQ as a notification. This IRQ may have
+ * also updated the RCU callback list. These events may require a
+ * re-evaluation of the next tick. Depending on the context:
+ *
+ * 1) If the CPU is idle and no resched is pending, just proceed with idle
+ *    time accounting. The next tick will be re-evaluated on the next idle
+ *    loop iteration.
+ *
+ * 2) If the CPU is nohz_full:
+ *
+ *    2.1) If there is any tick dependency, restart the tick if stopped.
+ *
+ *    2.2) If there is no tick dependency, (re-)evaluate the next tick and
+ *         stop/update it accordingly.
  */
 void tick_nohz_irq_exit(void)
 {
@@ -1319,11 +1330,20 @@ static void tick_nohz_idle_update_tick(struct tick_sched *ts, ktime_t now)
 }
 
 /**
- * tick_nohz_idle_exit - restart the idle tick from the idle task
+ * tick_nohz_idle_exit - Update the tick upon idle task exit
+ *
+ * When the idle task exits, update the tick depending on the
+ * following situations:
+ *
+ * 1) If the CPU is not in nohz_full mode (most cases), then
+ *    restart the tick.
+ *
+ * 2) If the CPU is in nohz_full mode (corner case):
+ *   2.1) If the tick can be kept stopped (no tick dependencies)
+ *        then re-eavaluate the next tick and try to keep it stopped
+ *        as long as possible.
+ *   2.2) If the tick has dependencies, restart the tick.
  *
- * Restart the idle tick when the CPU is woken up from idle
- * This also exit the RCU extended quiescent state. The CPU
- * can use RCU again after this function is called.
  */
 void tick_nohz_idle_exit(void)
 {
@@ -1353,7 +1373,13 @@ void tick_nohz_idle_exit(void)
 }
 
 /*
- * The nohz low res interrupt handler
+ * In low-resolution mode, the tick handler must be implemented directly
+ * at the clockevent level. hrtimer can't be used instead because its
+ * infrastructure actually relies on the tick itself as a backend in
+ * low-resolution mode (see hrtimer_run_queues()).
+ *
+ * This low-resolution handler still makes use of some hrtimer APIs meanwhile
+ * for commodity with expiration calculation and forwarding.
  */
 static void tick_lowres_handler(struct clock_event_device *dev)
 {
-- 
2.34.1

[PATCH 3/3] tick/nohz: Don't shutdown the lowres tick from itself

[Frederic Weisbecker]

In lowres dynticks mode, just like in highres dynticks mode, when there
is no tick to program in the future, the tick eventually gets
deactivated either:

* From the idle loop if in idle mode.
* From the IRQ exit if in full dynticks mode.

Therefore there is no need to deactivate it from the tick itself. This
just just brings more overhead in the idle tick path for no reason.

Fixes: 62c1256d5447 ("timers/nohz: Switch to ONESHOT_STOPPED in the low-res handler when the tick is stopped")
Signed-off-by: Frederic Weisbecker <frederic@kernel.org>
---
 kernel/time/tick-sched.c | 24 +++++++++++++-----------
 1 file changed, 13 insertions(+), 11 deletions(-)

diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
index dd25da8531f4..101251e103be 100644
--- a/kernel/time/tick-sched.c
+++ b/kernel/time/tick-sched.c
@@ -1392,18 +1392,16 @@ static void tick_lowres_handler(struct clock_event_device *dev)
 	tick_sched_do_timer(ts, now);
 	tick_sched_handle(ts, regs);
 
-	if (unlikely(ts->tick_stopped)) {
-		/*
-		 * The clockevent device is not reprogrammed, so change the
-		 * clock event device to ONESHOT_STOPPED to avoid spurious
-		 * interrupts on devices which might not be truly one shot.
-		 */
-		tick_program_event(KTIME_MAX, 1);
-		return;
+	/*
+	 * In dynticks mode, tick reprogram is deferred:
+	 * - to the idle task if in dynticks-idle
+	 * - to IRQ exit if in full-dynticks.
+	 */
+	if (likely(!ts->tick_stopped)) {
+		hrtimer_forward(&ts->sched_timer, now, TICK_NSEC);
+		tick_program_event(hrtimer_get_expires(&ts->sched_timer), 1);
 	}
 
-	hrtimer_forward(&ts->sched_timer, now, TICK_NSEC);
-	tick_program_event(hrtimer_get_expires(&ts->sched_timer), 1);
 }
 
 static inline void tick_nohz_activate(struct tick_sched *ts, int mode)
@@ -1508,7 +1506,11 @@ static enum hrtimer_restart tick_highres_handler(struct hrtimer *timer)
 	else
 		ts->next_tick = 0;
 
-	/* No need to reprogram if we are in idle or full dynticks mode */
+	/*
+	 * In dynticks mode, tick reprogram is deferred:
+	 * - to the idle task if in dynticks-idle
+	 * - to IRQ exit if in full-dynticks.
+	 */
 	if (unlikely(ts->tick_stopped))
 		return HRTIMER_NORESTART;
 
-- 
2.34.1

Re: [PATCH 1/3] tick/nohz: Rename the tick handlers to more self-explanatory names

[Joel Fernandes]

On 7/14/23 08:08, Frederic Weisbecker wrote:
> The current name of the tick handlers don't tell much about what differ
> between them. Use names that better reflect their role and resolution.
> 
> Signed-off-by: Frederic Weisbecker <frederic@kernel.org>
> ---
>   kernel/time/tick-sched.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
> index 89055050d1ac..a7b5960258f2 100644
> --- a/kernel/time/tick-sched.c
> +++ b/kernel/time/tick-sched.c
> @@ -1355,7 +1355,7 @@ void tick_nohz_idle_exit(void)
>   /*
>    * The nohz low res interrupt handler
>    */
> -static void tick_nohz_handler(struct clock_event_device *dev)
> +static void tick_lowres_handler(struct clock_event_device *dev)

Maybe tick_nohz_lowres_handler() is better?

You also have tick_periodic which is also 'lowres'.

That would also keep the "tick_nohz" prefix which is prevalent in this code.

>   {
>   	struct tick_sched *ts = this_cpu_ptr(&tick_cpu_sched);
>   	struct pt_regs *regs = get_irq_regs();
> @@ -1401,7 +1401,7 @@ static void tick_nohz_switch_to_nohz(void)
>   	if (!tick_nohz_enabled)
>   		return;
>   
> -	if (tick_switch_to_oneshot(tick_nohz_handler))
> +	if (tick_switch_to_oneshot(tick_lowres_handler))
>   		return;
>   
>   	/*
> @@ -1464,7 +1464,7 @@ void tick_irq_enter(void)
>    * We rearm the timer until we get disabled by the idle code.
>    * Called with interrupts disabled.
>    */
> -static enum hrtimer_restart tick_sched_timer(struct hrtimer *timer)
> +static enum hrtimer_restart tick_highres_handler(struct hrtimer *timer)

This one looks good.

thanks,

  - Joel


>   {
>   	struct tick_sched *ts =
>   		container_of(timer, struct tick_sched, sched_timer);
> @@ -1513,7 +1513,7 @@ void tick_setup_sched_timer(void)
>   	 * Emulate tick processing via per-CPU hrtimers:
>   	 */
>   	hrtimer_init(&ts->sched_timer, CLOCK_MONOTONIC, HRTIMER_MODE_ABS_HARD);
> -	ts->sched_timer.function = tick_sched_timer;
> +	ts->sched_timer.function = tick_highres_handler;
>   
>   	/* Get the next period (per-CPU) */
>   	hrtimer_set_expires(&ts->sched_timer, tick_init_jiffy_update());

Re: [PATCH 3/3] tick/nohz: Don't shutdown the lowres tick from itself

[Joel Fernandes]

On 7/14/23 08:08, Frederic Weisbecker wrote:
> In lowres dynticks mode, just like in highres dynticks mode, when there
> is no tick to program in the future, the tick eventually gets
> deactivated either:
> 
> * From the idle loop if in idle mode.
> * From the IRQ exit if in full dynticks mode.
> 
> Therefore there is no need to deactivate it from the tick itself. This
> just just brings more overhead in the idle tick path for no reason.
> 
> Fixes: 62c1256d5447 ("timers/nohz: Switch to ONESHOT_STOPPED in the low-res handler when the tick is stopped")
> Signed-off-by: Frederic Weisbecker <frederic@kernel.org>
> ---
>   kernel/time/tick-sched.c | 24 +++++++++++++-----------
>   1 file changed, 13 insertions(+), 11 deletions(-)
> 
> diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
> index dd25da8531f4..101251e103be 100644
> --- a/kernel/time/tick-sched.c
> +++ b/kernel/time/tick-sched.c
> @@ -1392,18 +1392,16 @@ static void tick_lowres_handler(struct clock_event_device *dev)
>   	tick_sched_do_timer(ts, now);
>   	tick_sched_handle(ts, regs);
>   
> -	if (unlikely(ts->tick_stopped)) {
> -		/*
> -		 * The clockevent device is not reprogrammed, so change the
> -		 * clock event device to ONESHOT_STOPPED to avoid spurious
> -		 * interrupts on devices which might not be truly one shot.
> -		 */
> -		tick_program_event(KTIME_MAX, 1);
> -		return;
> +	/*
> +	 * In dynticks mode, tick reprogram is deferred:
> +	 * - to the idle task if in dynticks-idle
> +	 * - to IRQ exit if in full-dynticks.
> +	 */
> +	if (likely(!ts->tick_stopped)) {
> +		hrtimer_forward(&ts->sched_timer, now, TICK_NSEC);
> +		tick_program_event(hrtimer_get_expires(&ts->sched_timer), 1);
>   	}
>   
> -	hrtimer_forward(&ts->sched_timer, now, TICK_NSEC);
> -	tick_program_event(hrtimer_get_expires(&ts->sched_timer), 1);

One slight concern here though, where in the idle loop is the removed statement 
"tick_program_event(KTIME_MAX, 1);" happening if the tick was already stopped 
before? If it is happening in tick_nohz_stop_tick(), don't we early return from 
there and avoid doing that "tick_program_event(KTIME_MAX, 1);" altogether, if 
the tick was already stopped and the next event has not changed?

         /* Skip reprogram of event if its not changed */
         if (ts->tick_stopped && (expires == ts->next_tick)) {
                 /* Sanity check: make sure clockevent is actually programmed */
                 if (tick == KTIME_MAX || ts->next_tick ==  [...]
                         return;
		[...]
	}

Also just a nit, here you can remove indent by doing:

if (unlikely(ts->tick_stopped))
     return;
hrtimer_forward(&ts->sched_timer, now, TICK_NSEC);
tick_program_event(hrtimer_get_expires(&ts->sched_timer), 1);

Which is pretty much the original code except for the tick_program_event().

thanks,

  - Joel


>   }
>   
>   static inline void tick_nohz_activate(struct tick_sched *ts, int mode)
> @@ -1508,7 +1506,11 @@ static enum hrtimer_restart tick_highres_handler(struct hrtimer *timer)
>   	else
>   		ts->next_tick = 0;
>   
> -	/* No need to reprogram if we are in idle or full dynticks mode */
> +	/*
> +	 * In dynticks mode, tick reprogram is deferred:
> +	 * - to the idle task if in dynticks-idle
> +	 * - to IRQ exit if in full-dynticks.
> +	 */
>   	if (unlikely(ts->tick_stopped))
>   		return HRTIMER_NORESTART;
>   

Re: [PATCH 3/3] tick/nohz: Don't shutdown the lowres tick from itself

[Frederic Weisbecker]

On Fri, Jul 14, 2023 at 02:44:49PM -0400, Joel Fernandes wrote:
> On 7/14/23 08:08, Frederic Weisbecker wrote:
> One slight concern here though, where in the idle loop is the removed
> statement "tick_program_event(KTIME_MAX, 1);" happening if the tick was
> already stopped before? If it is happening in tick_nohz_stop_tick(), don't
> we early return from there and avoid doing that
> "tick_program_event(KTIME_MAX, 1);" altogether, if the tick was already
> stopped and the next event has not changed?
> 
>         /* Skip reprogram of event if its not changed */
>         if (ts->tick_stopped && (expires == ts->next_tick)) {
>                 /* Sanity check: make sure clockevent is actually programmed */
>                 if (tick == KTIME_MAX || ts->next_tick ==  [...]
>                         return;
> 		[...]
> 	}

Sure, if tick_program_event(KTIME_MAX, 1) was already called in the
previous idle loop iteration, then there is no need to call that again.

Or am I missing something else?

> 
> Also just a nit, here you can remove indent by doing:
> 
> if (unlikely(ts->tick_stopped))
>     return;
> hrtimer_forward(&ts->sched_timer, now, TICK_NSEC);
> tick_program_event(hrtimer_get_expires(&ts->sched_timer), 1);
> 
> Which is pretty much the original code except for the tick_program_event().

Either I remove an indent or I remove a statement. I guess it's a matter of
personal taste. I don't mind either way :-)

Thanks.

Re: [PATCH 3/3] tick/nohz: Don't shutdown the lowres tick from itself

[Joel Fernandes]

On Fri, Jul 14, 2023 at 8:01 PM Frederic Weisbecker <frederic@kernel.org> wrote:
>
> On Fri, Jul 14, 2023 at 02:44:49PM -0400, Joel Fernandes wrote:
> > On 7/14/23 08:08, Frederic Weisbecker wrote:
> > One slight concern here though, where in the idle loop is the removed
> > statement "tick_program_event(KTIME_MAX, 1);" happening if the tick was
> > already stopped before? If it is happening in tick_nohz_stop_tick(), don't
> > we early return from there and avoid doing that
> > "tick_program_event(KTIME_MAX, 1);" altogether, if the tick was already
> > stopped and the next event has not changed?
> >
> >         /* Skip reprogram of event if its not changed */
> >         if (ts->tick_stopped && (expires == ts->next_tick)) {
> >                 /* Sanity check: make sure clockevent is actually programmed */
> >                 if (tick == KTIME_MAX || ts->next_tick ==  [...]
> >                         return;
> >               [...]
> >       }
>
> Sure, if tick_program_event(KTIME_MAX, 1) was already called in the
> previous idle loop iteration, then there is no need to call that again.
>
> Or am I missing something else?

Just take it with a grain of salt but I think you need to still call
tick_program_event(KTIME_MAX, 1) here for the case where the tick was
previously stopped, and then when the next tick fires (say after a
long time T), but that tick is a one-off and does not result in
restarting the tick -- then there is no one to call
"tick_program_event(KTIME_MAX, 1)".

I think that's the concern Nick was addressing in [1] so that it
resets the tick device correctly?

[1] https://lore.kernel.org/lkml/165089105607.4207.3022534114716811208.tip-bot2@tip-bot2/

> >
> > Also just a nit, here you can remove indent by doing:
> >
> > if (unlikely(ts->tick_stopped))
> >     return;
> > hrtimer_forward(&ts->sched_timer, now, TICK_NSEC);
> > tick_program_event(hrtimer_get_expires(&ts->sched_timer), 1);
> >
> > Which is pretty much the original code except for the tick_program_event().
>
> Either I remove an indent or I remove a statement. I guess it's a matter of
> personal taste. I don't mind either way :-)

Ah true, in defense of the "less indent" way, the original code was
also using that style. ;-) But I am also Ok with either way. :-)

thanks,

 - Joel

Re: [PATCH 3/3] tick/nohz: Don't shutdown the lowres tick from itself

[Frederic Weisbecker]

On Fri, Jul 14, 2023 at 09:02:43PM -0400, Joel Fernandes wrote:
> On Fri, Jul 14, 2023 at 8:01 PM Frederic Weisbecker <frederic@kernel.org> wrote:
> >
> > On Fri, Jul 14, 2023 at 02:44:49PM -0400, Joel Fernandes wrote:
> > > On 7/14/23 08:08, Frederic Weisbecker wrote:
> > > One slight concern here though, where in the idle loop is the removed
> > > statement "tick_program_event(KTIME_MAX, 1);" happening if the tick was
> > > already stopped before? If it is happening in tick_nohz_stop_tick(), don't
> > > we early return from there and avoid doing that
> > > "tick_program_event(KTIME_MAX, 1);" altogether, if the tick was already
> > > stopped and the next event has not changed?
> > >
> > >         /* Skip reprogram of event if its not changed */
> > >         if (ts->tick_stopped && (expires == ts->next_tick)) {
> > >                 /* Sanity check: make sure clockevent is actually programmed */
> > >                 if (tick == KTIME_MAX || ts->next_tick ==  [...]
> > >                         return;
> > >               [...]
> > >       }
> >
> > Sure, if tick_program_event(KTIME_MAX, 1) was already called in the
> > previous idle loop iteration, then there is no need to call that again.
> >
> > Or am I missing something else?
> 
> Just take it with a grain of salt but I think you need to still call
> tick_program_event(KTIME_MAX, 1) here for the case where the tick was
> previously stopped, and then when the next tick fires (say after a
> long time T), but that tick is a one-off and does not result in
> restarting the tick -- then there is no one to call
> "tick_program_event(KTIME_MAX, 1)".

I'm a bit confused about that one-off thing. What can trigger that timer
interrupt if it has been stopped?

One thing can happen though: a pending timer IRQ while we are stopping the
tick (IRQs are disabled in that idle loop portion). But then that pending timer
interrupt is not going to reprogram another one. So it remains stopped.

Thanks.

Re: [PATCH 3/3] tick/nohz: Don't shutdown the lowres tick from itself

[Joel Fernandes]

> On Jul 15, 2023, at 2:19 PM, Frederic Weisbecker <frederic@kernel.org> wrote:
> 
> On Fri, Jul 14, 2023 at 09:02:43PM -0400, Joel Fernandes wrote:
>>> On Fri, Jul 14, 2023 at 8:01 PM Frederic Weisbecker <frederic@kernel.org> wrote:
>>> 
>>> On Fri, Jul 14, 2023 at 02:44:49PM -0400, Joel Fernandes wrote:
>>>> On 7/14/23 08:08, Frederic Weisbecker wrote:
>>>> One slight concern here though, where in the idle loop is the removed
>>>> statement "tick_program_event(KTIME_MAX, 1);" happening if the tick was
>>>> already stopped before? If it is happening in tick_nohz_stop_tick(), don't
>>>> we early return from there and avoid doing that
>>>> "tick_program_event(KTIME_MAX, 1);" altogether, if the tick was already
>>>> stopped and the next event has not changed?
>>>> 
>>>>        /* Skip reprogram of event if its not changed */
>>>>        if (ts->tick_stopped && (expires == ts->next_tick)) {
>>>>                /* Sanity check: make sure clockevent is actually programmed */
>>>>                if (tick == KTIME_MAX || ts->next_tick ==  [...]
>>>>                        return;
>>>>              [...]
>>>>      }
>>> 
>>> Sure, if tick_program_event(KTIME_MAX, 1) was already called in the
>>> previous idle loop iteration, then there is no need to call that again.
>>> 
>>> Or am I missing something else?
>> 
>> Just take it with a grain of salt but I think you need to still call
>> tick_program_event(KTIME_MAX, 1) here for the case where the tick was
>> previously stopped, and then when the next tick fires (say after a
>> long time T), but that tick is a one-off and does not result in
>> restarting the tick -- then there is no one to call
>> "tick_program_event(KTIME_MAX, 1)".
> 
> I'm a bit confused about that one-off thing. What can trigger that timer
> interrupt if it has been stopped?

It is the tick that has been stopped in this scenario.
The timer could still be armed to serve a future hrtimer?

I think the naming in the code for is a bit confusing for tick vs timer event,
so I could be the confused one.

Thanks,

 - Joel


> One thing can happen though: a pending timer IRQ while we are stopping the
> tick (IRQs are disabled in that idle loop portion). But then that pending timer
> interrupt is not going to reprogram another one. So it remains stopped.
> 
> Thanks.

Re: [PATCH 3/3] tick/nohz: Don't shutdown the lowres tick from itself

[Joel Fernandes]

On Sat, Jul 15, 2023 at 08:18:57PM +0200, Frederic Weisbecker wrote:
> On Fri, Jul 14, 2023 at 09:02:43PM -0400, Joel Fernandes wrote:
> > On Fri, Jul 14, 2023 at 8:01 PM Frederic Weisbecker <frederic@kernel.org> wrote:
> > >
> > > On Fri, Jul 14, 2023 at 02:44:49PM -0400, Joel Fernandes wrote:
> > > > On 7/14/23 08:08, Frederic Weisbecker wrote:
> > > > One slight concern here though, where in the idle loop is the removed
> > > > statement "tick_program_event(KTIME_MAX, 1);" happening if the tick was
> > > > already stopped before? If it is happening in tick_nohz_stop_tick(), don't
> > > > we early return from there and avoid doing that
> > > > "tick_program_event(KTIME_MAX, 1);" altogether, if the tick was already
> > > > stopped and the next event has not changed?
> > > >
> > > >         /* Skip reprogram of event if its not changed */
> > > >         if (ts->tick_stopped && (expires == ts->next_tick)) {
> > > >                 /* Sanity check: make sure clockevent is actually programmed */
> > > >                 if (tick == KTIME_MAX || ts->next_tick ==  [...]
> > > >                         return;
> > > >               [...]
> > > >       }
> > >
> > > Sure, if tick_program_event(KTIME_MAX, 1) was already called in the
> > > previous idle loop iteration, then there is no need to call that again.
> > >
> > > Or am I missing something else?
> > 
> > Just take it with a grain of salt but I think you need to still call
> > tick_program_event(KTIME_MAX, 1) here for the case where the tick was
> > previously stopped, and then when the next tick fires (say after a
> > long time T), but that tick is a one-off and does not result in
> > restarting the tick -- then there is no one to call
> > "tick_program_event(KTIME_MAX, 1)".
> 
> I'm a bit confused about that one-off thing. What can trigger that timer
> interrupt if it has been stopped?
> 
> One thing can happen though: a pending timer IRQ while we are stopping the
> tick (IRQs are disabled in that idle loop portion). But then that pending timer
> interrupt is not going to reprogram another one. So it remains stopped.

I think I see what you mean now. Maybe I wrongly assumed the above 'Skip
reprogram of event' code could early return and skip over
"tick_program_event(KTIME_MAX, 1);", but I think it cannot because of the
"expires != ts->next_tick" check.

 Maybe the "tick_program_event(KTIME_MAX, 1)" bit in tick_nohz_handler() is
 supposed to handle buggy hardware where an unexpected timer event came
 through? In such a situation, the idle loop will not write
 "tick_program_event(KTIME_MAX, 1);" again because it already did so the
 previous time, as you pointed.

Adding Vineeth who is also looking into this code.

thanks,

 - Joel

Re: [PATCH 3/3] tick/nohz: Don't shutdown the lowres tick from itself

[Frederic Weisbecker]

On Mon, Jul 17, 2023 at 05:30:49PM +0000, Joel Fernandes wrote:
> I think I see what you mean now. Maybe I wrongly assumed the above 'Skip
> reprogram of event' code could early return and skip over
> "tick_program_event(KTIME_MAX, 1);", but I think it cannot because of the
> "expires != ts->next_tick" check.
> 
>  Maybe the "tick_program_event(KTIME_MAX, 1)" bit in tick_nohz_handler() is
>  supposed to handle buggy hardware where an unexpected timer event came
>  through? In such a situation, the idle loop will not write
>  "tick_program_event(KTIME_MAX, 1);" again because it already did so the
>  previous time, as you pointed.

Well at least if the double write was put there intentionally in order to
fix buggy hardware, this was neither mentionned nor commented anywhere AFAICT.

Thanks.

> 
> Adding Vineeth who is also looking into this code.
> 
> thanks,
> 
>  - Joel
>