Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:6 as permitted sender) client-ip=2620:137:e000::3:6;
MIME-Version: 1.0
References: <20230912104406.312185-1-frederic@kernel.org> <20230912104406.312185-4-frederic@kernel.org>
In-Reply-To: <20230912104406.312185-4-frederic@kernel.org>
From:   Joel Fernandes <joel@joelfernandes.org>
Date:   Wed, 13 Sep 2023 21:17:21 -0400
Message-ID: <CAEXW_YTwzK9jkUdMUPY0t++yYvwSHx1EZiPX6NiSt7hPhHZbpA@mail.gmail.com>
Subject: Re: [PATCH 3/5] tick/nohz: Don't shutdown the lowres tick from itself
To:     Frederic Weisbecker <frederic@kernel.org>
Cc:     LKML <linux-kernel@vger.kernel.org>,
        Thomas Gleixner <tglx@linutronix.de>, vineethrp@gmail.com,
        Nicholas Piggin <npiggin@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Tue, Sep 12, 2023 at 6:44=E2=80=AFAM Frederic Weisbecker <frederic@kerne=
l.org> 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-r=
es handler when the tick is stopped")
> Signed-off-by: Frederic Weisbecker <frederic@kernel.org>

If on some weird hardware, say  ts->next_tick =3D KTIME_MAX but a
spurious timer interrupt went off and tick_nohz_handler() did get
called (yeah weird hypothetical situation), then in
tick_nohz_stop_tick() we might early return from:

/* Skip reprogram of event if its not changed */
if (ts->tick_stopped && (expires =3D=3D ts->next_tick))

without no "eventual" reprogramming.

Maybe we should also reprogram with KTIME_MAX in such a situation?
Then we can get rid of it from tick_nohz_handler() for the common case
as you are doing.

So for weird hardware, with this patch we are not doing an extra
tick_program_event(KTIME_MAX, 1); like Nick was doing. That makes me a
tad bit nervous.

Otherwise your patch looks correct to me (for hardware that tends not
to misbehave).

thanks,

 - Joel


> ---
>  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 95a8d1d118a2..8e9a9dcf60d5 100644
> --- a/kernel/time/tick-sched.c
> +++ b/kernel/time/tick-sched.c
> @@ -1403,18 +1403,16 @@ static void tick_nohz_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 t=
he
> -                * clock event device to ONESHOT_STOPPED to avoid spuriou=
s
> -                * interrupts on devices which might not be truly one sho=
t.
> -                */
> -               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)
> @@ -1519,7 +1517,11 @@ static enum hrtimer_restart tick_nohz_highres_hand=
ler(struct hrtimer *timer)
>         else
>                 ts->next_tick =3D 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
>