Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20181130211750.5571-1-roland@purestorage.com> <alpine.DEB.2.21.1812041237460.8181@nanos.tec.linutronix.de>
In-Reply-To: <alpine.DEB.2.21.1812041237460.8181@nanos.tec.linutronix.de>
From:   Roland Dreier <roland@purestorage.com>
Date:   Tue, 4 Dec 2018 11:33:43 -0800
Message-ID: <CAL1RGDV_f8_sDMrK6rUq1P-xh-=E0xwphPo0bQ6tHSnJk6JQmw@mail.gmail.com>
Subject: Re: [PATCH] clocksource: Add heuristics to avoid switching away from
 TSC due to timer delay
To:     Thomas Gleixner <tglx@linutronix.de>
Cc:     john.stultz@linaro.org, sboyd@kernel.org,
        LKML <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

>                  /*
>                   * Proper multiline comments look like this not like
>                   * the above.
>                   */

Got it, will fix next time around.

> That aside. Why are you trying to do heuristics on the delta?
>
> We have way better information than that. The watchdog timer expiry time is
> known and we can determine the exact delay of the timer.
>
> The watchdog clocksource provides the maximum 'idle' time, i.e. the time
> between two reads, in clocksource::max_idle_ns. That value is filled in
> when the clocksource is configured.
>
> So without doing speculation we can make an informed decision:
>
>     elapsed = jiffies_to_nsec(jiffies - watchdog_timer->expires) +
>               WATCHDOG_INTERVAL_NS;
>
>     if (elapsed > wdcs->max_idle_ns) {
>             Skip ......
>     }

Yes, that makes more sense than what I was doing, although I'm not
sure on the details.  Just missed that idea.

Why are you adding the watchdog interval to the calculated elapsed
time?  It seems we have an issue exactly if jiffies -
watchdog_timer->expires is too big, without adding the interval we
tried to wait in on top.  Also I think we might want to be careful
that jiffies is >= the expires time - or is it not possible that a
timer fires one jiffy early?

Also for full generality it seems we should check against the
clocksource max_idle_ns as well - for x86 TSC is wider than HPET but
there may be other architectures that could hit the same problem, just
with the clocksource being checked wrapping around instead of the
watchdog clocksource.  Right?

Thanks!
  Roland