Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20211008080305.13401-1-yanghui.def@bytedance.com>
 <CALAqxLWUNFozhfhuVFAPo9xGgO+xsXPQ=i5w1Y0E9-w-PdHXgw@mail.gmail.com>
 <c70a418d-4748-6876-ac8a-c9d1b7e94e78@gmail.com> <CALAqxLVgQ6QEThWaN65nOW9F_XCh7885n9RigAQDU+OgDntS5g@mail.gmail.com>
 <6b715fb7-9850-04f3-4ab8-1a2a8a2cdfbf@gmail.com>
In-Reply-To: <6b715fb7-9850-04f3-4ab8-1a2a8a2cdfbf@gmail.com>
From:   John Stultz <john.stultz@linaro.org>
Date:   Mon, 11 Oct 2021 22:29:42 -0700
Message-ID: <CALAqxLWgw8tA1Lrg27JUUFrGWCQqPQXmhjHyjsTRA5a4qingkg@mail.gmail.com>
Subject: Re: [PATCH] Clocksource: Avoid misjudgment of clocksource
To:     brookxu <brookxu.cn@gmail.com>
Cc:     yanghui <yanghui.def@bytedance.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        Stephen Boyd <sboyd@kernel.org>,
        lkml <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Mon, Oct 11, 2021 at 10:23 PM brookxu <brookxu.cn@gmail.com> wrote:
> John Stultz wrote on 2021/10/12 12:52 =E4=B8=8B=E5=8D=88:
> > On Sat, Oct 9, 2021 at 7:04 AM brookxu <brookxu.cn@gmail.com> wrote:
> >>
> >> hello
> >>
> >> John Stultz wrote on 2021/10/9 7:45:
> >>> On Fri, Oct 8, 2021 at 1:03 AM yanghui <yanghui.def@bytedance.com> wr=
ote:
> >>>>
> >>>> clocksource_watchdog is executed every WATCHDOG_INTERVAL(0.5s) by
> >>>> Timer. But sometimes system is very busy and the Timer cannot be
> >>>> executed in 0.5sec. For example,if clocksource_watchdog be executed
> >>>> after 10sec, the calculated value of abs(cs_nsec - wd_nsec) will
> >>>> be enlarged. Then the current clocksource will be misjudged as
> >>>> unstable. So we add conditions to prevent the clocksource from
> >>>> being misjudged.
> >>>>
> >>>> Signed-off-by: yanghui <yanghui.def@bytedance.com>
> >>>> ---
> >>>>  kernel/time/clocksource.c | 6 +++++-
> >>>>  1 file changed, 5 insertions(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/kernel/time/clocksource.c b/kernel/time/clocksource.c
> >>>> index b8a14d2fb5ba..d535beadcbc8 100644
> >>>> --- a/kernel/time/clocksource.c
> >>>> +++ b/kernel/time/clocksource.c
> >>>> @@ -136,8 +136,10 @@ static void __clocksource_change_rating(struct =
clocksource *cs, int rating);
> >>>>
> >>>>  /*
> >>>>   * Interval: 0.5sec.
> >>>> + * MaxInterval: 1s.
> >>>>   */
> >>>>  #define WATCHDOG_INTERVAL (HZ >> 1)
> >>>> +#define WATCHDOG_MAX_INTERVAL_NS (NSEC_PER_SEC)
> >>>>
> >>>>  static void clocksource_watchdog_work(struct work_struct *work)
> >>>>  {
> >>>> @@ -404,7 +406,9 @@ static void clocksource_watchdog(struct timer_li=
st *unused)
> >>>>
> >>>>                 /* Check the deviation from the watchdog clocksource=
. */
> >>>>                 md =3D cs->uncertainty_margin + watchdog->uncertaint=
y_margin;
> >>>> -               if (abs(cs_nsec - wd_nsec) > md) {
> >>>> +               if ((abs(cs_nsec - wd_nsec) > md) &&
> >>>> +                       cs_nsec < WATCHDOG_MAX_INTERVAL_NS &&
> >>>
> >>> Sorry, it's been awhile since I looked at this code, but why are you
> >>> bounding the clocksource delta here?
> >>> It seems like if the clocksource being watched was very wrong (with a
> >>> delta larger than the MAX_INTERVAL_NS), we'd want to throw it out.
> >>>
> >>>> +                       wd_nsec < WATCHDOG_MAX_INTERVAL_NS) {
> >>>
> >>> Bounding the watchdog interval on the check does seem reasonable.
> >>> Though one may want to keep track that if we are seeing too many of
> >>> these delayed watchdog checks we provide some feedback via dmesg.
> >>
> >> For some fast timeout timers, such as acpi-timer, checking wd_nsec sho=
uld not
> >> make much sense, because when wacthdog is called, the timer may overfl=
ow many
> >> times.
> >
> > Indeed. But in that case we can't tell which way is up. This is what I
> > was fretting about when I said:
> >> So I do worry these watchdog robustness fixes are papering over a
> >> problem, pushing expectations closer to the edge of how far the system
> >> should tolerate bad behavior. Because at some point we'll fall off. :)
> >
> > If the timer is delayed long enough for the watchdog to wrap, we're
> > way out of tolerable behavior. There's not much we can do because we
> > can't even tell what happened.
> >
> > But in the case where the watchdog has not wrapped, I don't see a
> > major issue with trying to be a bit more robust in the face of just
> > slightly delayed timers.
> > (And yes, we can't really distinguish between slightly delayed and
> > watchdog-wrap-interval + slight delay, but in either case we can
> > probably skip disqualifying the clocksource as we know something seems
> > off)
>
> If we record the watchdog's start_time in clocksource_start_watchdog(), a=
nd then
> when we verify cycles in clocksource_watchdog(), check whether the clocks=
ource
> watchdog is blocked. Due to MSB verification, if the blocked time is grea=
ter than
> half of the watchdog timer max_cycles, then we can safely ignore the curr=
ent
> verification? Do you think this idea is okay?

I can't say I totally understand the idea. Maybe could you clarify with a p=
atch?

thanks
-john