Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230714120852.23573-1-frederic@kernel.org> <20230714120852.23573-4-frederic@kernel.org>
 <9347e3d4-e774-f75f-22c4-6c2dba294423@joelfernandes.org> <ZLHh71KIIioR85aa@lothringen>
In-Reply-To: <ZLHh71KIIioR85aa@lothringen>
From:   Joel Fernandes <joel@joelfernandes.org>
Date:   Fri, 14 Jul 2023 21:02:43 -0400
Message-ID: <CAEXW_YRTtvq0_YZiN=V9DZi2QxrC4hQFeUC9=JrgAKkg8KAnmw@mail.gmail.com>
Subject: Re: [PATCH 3/3] tick/nohz: Don't shutdown the lowres tick from itself
To:     Frederic Weisbecker <frederic@kernel.org>
Cc:     Thomas Gleixner <tglx@linutronix.de>,
        LKML <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Fri, Jul 14, 2023 at 8:01=E2=80=AFPM Frederic Weisbecker <frederic@kerne=
l.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(), do=
n'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 =3D=3D ts->next_tick)) {
> >                 /* Sanity check: make sure clockevent is actually progr=
ammed */
> >                 if (tick =3D=3D KTIME_MAX || ts->next_tick =3D=3D  [...=
]
> >                         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_even=
t().
>
> 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