Re: [patch 5/8] ntp: Make the RTC synchronization more reliable
Hi Thomas,
On Mon, Jan 11, 2021 at 11:12 AM Thomas Gleixner wrote:
> On Tue, Dec 29 2020 at 20:41, Geert Uytterhoeven wrote:
> >> Reported-by: Miroslav Lichvar
> >> Signed-off-by: Thomas Gleixner
> >
> > Thanks for your patch, which is now commit c9e6189fb03123a7 ("ntp: Make
> > the RTC synchronization more reliable").
> >
> > Since this commit, the I2C RTC on the R-Car M2-W Koelsch development
> > board is accessed every two seconds. Sticking a WARN() in the I2C
> > activation path gives e.g.
>
> Huch? Every two seconds? The timer is armed with 11 * 60 * NSEC_PER_SEC,
> which is 11 minutes. Confused
Thanks for the hint:
#define SYNC_PERIOD_NS (11UL * 60 * NSEC_PER_SEC)
is truncated on 32-bit platforms.
Patch sent.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Re: [patch 5/8] ntp: Make the RTC synchronization more reliable
On Tue, Dec 29 2020 at 20:41, Geert Uytterhoeven wrote:
> Hi Thomas,
>> Reported-by: Miroslav Lichvar
>> Signed-off-by: Thomas Gleixner
>
> Thanks for your patch, which is now commit c9e6189fb03123a7 ("ntp: Make
> the RTC synchronization more reliable").
>
> Since this commit, the I2C RTC on the R-Car M2-W Koelsch development
> board is accessed every two seconds. Sticking a WARN() in the I2C
> activation path gives e.g.
Huch? Every two seconds? The timer is armed with 11 * 60 * NSEC_PER_SEC,
which is 11 minutes. Confused
Thanks,
tglx
Re: [patch 5/8] ntp: Make the RTC synchronization more reliable
Hi Thomas,
On Sun, Dec 6, 2020 at 11:10 PM Thomas Gleixner wrote:
> Miroslav reported that the periodic RTC synchronization in the NTP code
> fails more often than not to hit the specified update window.
>
> The reason is that the code uses delayed_work to schedule the update which
> needs to be in thread context as the underlying RTC might be connected via
> a slow bus, e.g. I2C. In the update function it verifies whether the
> current time is correct vs. the requirements of the underlying RTC.
>
> But delayed_work is using the timer wheel for scheduling which is
> inaccurate by design. Depending on the distance to the expiry the wheel
> gets less granular to allow batching and to avoid the cascading of the
> original timer wheel. See 500462a9de65 ("timers: Switch to a non-cascading
> wheel") and the code for further details.
>
> The code already deals with this by splitting the 660 seconds period into a
> long 659 seconds timer and then retrying with a smaller delta.
>
> But looking at the actual granularities of the timer wheel (which depend on
> the HZ configuration) the 659 seconds timer ends up in an outer wheel level
> and is affected by a worst case granularity of:
>
> HZ Granularity
> 100032s
> 25016s
> 10040s
>
> So the initial timer can be already off by max 12.5% which is not a big
> issue as the period of the sync is defined as ~11 minutes.
>
> The fine grained second attempt schedules to the desired update point with
> a timer expiring less than a second from now. Depending on the actual delta
> and the HZ setting even the second attempt can end up in outer wheel levels
> which have a large enough granularity to make the correctness check fail.
>
> As this is a fundamental property of the timer wheel there is no way to
> make this more accurate short of iterating in one jiffies steps towards the
> update point.
>
> Switch it to an hrtimer instead which schedules the actual update work. The
> hrtimer will expire precisely (max 1 jiffie delay when high resolution
> timers are not available). The actual scheduling delay of the work is the
> same as before.
>
> The update is triggered from do_adjtimex() which is a bit racy but not much
> more racy than it was before:
>
> if (ntp_synced())
> queue_delayed_work(system_power_efficient_wq, _work, 0);
>
> which is racy when the work is currently executed and has not managed to
> reschedule itself.
>
> This becomes now:
>
> if (ntp_synced() && !hrtimer_is_queued(_hrtimer))
> queue_work(system_power_efficient_wq, _work, 0);
>
> which is racy when the hrtimer has expired and the work is currently
> executed and has not yet managed to rearm the hrtimer.
>
> Not a big problem as it just schedules work for nothing.
>
> The new implementation has a safe guard in place to catch the case where
> the hrtimer is queued on entry to the work function and avoids an extra
> update attempt of the RTC that way.
>
> Reported-by: Miroslav Lichvar
> Signed-off-by: Thomas Gleixner
Thanks for your patch, which is now commit c9e6189fb03123a7 ("ntp: Make
the RTC synchronization more reliable").
Since this commit, the I2C RTC on the R-Car M2-W Koelsch development
board is accessed every two seconds. Sticking a WARN() in the I2C
activation path gives e.g.
Modules linked in:
CPU: 0 PID: 12 Comm: kworker/0:1 Not tainted
5.10.0-rc1-koelsch-00038-gc9e6189fb031-dirty #1021
Hardware name: Generic R-Car Gen2 (Flattened Device Tree)
Workqueue: events rtc_timer_do_work
[] (unwind_backtrace) from [] (show_stack+0x10/0x14)
[] (show_stack) from [] (dump_stack+0xa0/0xc8)
[] (dump_stack) from [] (__warn+0xbc/0xfc)
[] (__warn) from [] (warn_slowpath_fmt+0x78/0xb0)
[] (warn_slowpath_fmt) from []
(cpg_mstp_clock_endisable+0x94/0x1f4)
[] (cpg_mstp_clock_endisable) from []
(clk_core_enable+0x194/0x29c)
[] (clk_core_enable) from []
(clk_core_enable_lock+0x18/0x2c)
[] (clk_core_enable_lock) from []
(pm_clk_resume+0x68/0xa0)
[] (pm_clk_resume) from []
(genpd_runtime_resume+0xc8/0x174)
[] (genpd_runtime_resume) from []
(__rpm_callback+0x30/0xe0)
[] (__rpm_callback) from [] (rpm_callback+0x70/0x80)
[] (rpm_callback) from [] (rpm_resume+0x438/0x4dc)
[] (rpm_resume) from [] (__pm_runtime_resume+0x64/0x80)
[] (__pm_runtime_resume) from []
(sh_mobile_i2c_xfer+0x38/0x554)
[] (sh_mobile_i2c_xfer) from []
(__i2c_transfer+0x4e4/0x680)
[] (__i2c_transfer) from [] (i2c_transfer+0x58/0xf8)
[] (i2c_transfer) from [] (regmap_i2c_read+0x58/0x94)
[] (regmap_i2c_read) from []
(_regmap_raw_read+0x19c/0x2f4)
[] (_regmap_raw_read) from []
(_regmap_bus_read+0x44/0x68)
[] (_regmap_bus_read) from [] (_regmap_read+0x84/0x1a4)
[] (_regmap_read) from []
(_regmap_update_bits+0xa8/0xf4)
[] (_regmap_update_bits) from []
(regmap_update_bits_base+0x4c/0x70)
[] (regmap_update_bits_base) from []
(regmap_update_bits+0x18/0x20)
[] (regmap_update_bits)
Re: [patch 5/8] ntp: Make the RTC synchronization more reliable
On Sun, Dec 06, 2020 at 10:46:18PM +0100, Thomas Gleixner wrote:
> Miroslav reported that the periodic RTC synchronization in the NTP code
> fails more often than not to hit the specified update window.
>
> The reason is that the code uses delayed_work to schedule the update which
> needs to be in thread context as the underlying RTC might be connected via
> a slow bus, e.g. I2C. In the update function it verifies whether the
> current time is correct vs. the requirements of the underlying RTC.
>
> But delayed_work is using the timer wheel for scheduling which is
> inaccurate by design. Depending on the distance to the expiry the wheel
> gets less granular to allow batching and to avoid the cascading of the
> original timer wheel. See 500462a9de65 ("timers: Switch to a non-cascading
> wheel") and the code for further details.
>
> The code already deals with this by splitting the 660 seconds period into a
> long 659 seconds timer and then retrying with a smaller delta.
>
> But looking at the actual granularities of the timer wheel (which depend on
> the HZ configuration) the 659 seconds timer ends up in an outer wheel level
> and is affected by a worst case granularity of:
>
> HZ Granularity
> 100032s
> 25016s
> 10040s
>
> So the initial timer can be already off by max 12.5% which is not a big
> issue as the period of the sync is defined as ~11 minutes.
>
> The fine grained second attempt schedules to the desired update point with
> a timer expiring less than a second from now. Depending on the actual delta
> and the HZ setting even the second attempt can end up in outer wheel levels
> which have a large enough granularity to make the correctness check fail.
>
> As this is a fundamental property of the timer wheel there is no way to
> make this more accurate short of iterating in one jiffies steps towards the
> update point.
>
> Switch it to an hrtimer instead which schedules the actual update work. The
> hrtimer will expire precisely (max 1 jiffie delay when high resolution
> timers are not available). The actual scheduling delay of the work is the
> same as before.
>
> The update is triggered from do_adjtimex() which is a bit racy but not much
> more racy than it was before:
>
> if (ntp_synced())
> queue_delayed_work(system_power_efficient_wq, _work, 0);
>
> which is racy when the work is currently executed and has not managed to
> reschedule itself.
>
> This becomes now:
>
> if (ntp_synced() && !hrtimer_is_queued(_hrtimer))
> queue_work(system_power_efficient_wq, _work, 0);
>
> which is racy when the hrtimer has expired and the work is currently
> executed and has not yet managed to rearm the hrtimer.
>
> Not a big problem as it just schedules work for nothing.
>
> The new implementation has a safe guard in place to catch the case where
> the hrtimer is queued on entry to the work function and avoids an extra
> update attempt of the RTC that way.
>
> Reported-by: Miroslav Lichvar
> Signed-off-by: Thomas Gleixner
> Cc: John Stultz
> Cc: Prarit Bhargava
> Cc: Jason Gunthorpe
> ---
> include/linux/timex.h |1
> kernel/time/ntp.c | 90
> -
> kernel/time/ntp_internal.h |7 +++
> 3 files changed, 55 insertions(+), 43 deletions(-)
Reviewed-by: Jason Gunthorpe
Jason
Re: [patch 5/8] ntp: Make the RTC synchronization more reliable
On Sun, Dec 06, 2020 at 10:46:18PM +0100, Thomas Gleixner wrote: > Switch it to an hrtimer instead which schedules the actual update work. The > hrtimer will expire precisely (max 1 jiffie delay when high resolution > timers are not available). The actual scheduling delay of the work is the > same as before. It works well in my tests. > This becomes now: > > if (ntp_synced() && !hrtimer_is_queued(_hrtimer)) > queue_work(system_power_efficient_wq, _work, 0); > > which is racy when the hrtimer has expired and the work is currently > executed and has not yet managed to rearm the hrtimer. > > Not a big problem as it just schedules work for nothing. No more unexpected updates of the RTC observed. Tested-by: Miroslav Lichvar Thanks, -- Miroslav Lichvar
[patch 5/8] ntp: Make the RTC synchronization more reliable
Miroslav reported that the periodic RTC synchronization in the NTP code
fails more often than not to hit the specified update window.
The reason is that the code uses delayed_work to schedule the update which
needs to be in thread context as the underlying RTC might be connected via
a slow bus, e.g. I2C. In the update function it verifies whether the
current time is correct vs. the requirements of the underlying RTC.
But delayed_work is using the timer wheel for scheduling which is
inaccurate by design. Depending on the distance to the expiry the wheel
gets less granular to allow batching and to avoid the cascading of the
original timer wheel. See 500462a9de65 ("timers: Switch to a non-cascading
wheel") and the code for further details.
The code already deals with this by splitting the 660 seconds period into a
long 659 seconds timer and then retrying with a smaller delta.
But looking at the actual granularities of the timer wheel (which depend on
the HZ configuration) the 659 seconds timer ends up in an outer wheel level
and is affected by a worst case granularity of:
HZ Granularity
100032s
25016s
10040s
So the initial timer can be already off by max 12.5% which is not a big
issue as the period of the sync is defined as ~11 minutes.
The fine grained second attempt schedules to the desired update point with
a timer expiring less than a second from now. Depending on the actual delta
and the HZ setting even the second attempt can end up in outer wheel levels
which have a large enough granularity to make the correctness check fail.
As this is a fundamental property of the timer wheel there is no way to
make this more accurate short of iterating in one jiffies steps towards the
update point.
Switch it to an hrtimer instead which schedules the actual update work. The
hrtimer will expire precisely (max 1 jiffie delay when high resolution
timers are not available). The actual scheduling delay of the work is the
same as before.
The update is triggered from do_adjtimex() which is a bit racy but not much
more racy than it was before:
if (ntp_synced())
queue_delayed_work(system_power_efficient_wq, _work, 0);
which is racy when the work is currently executed and has not managed to
reschedule itself.
This becomes now:
if (ntp_synced() && !hrtimer_is_queued(_hrtimer))
queue_work(system_power_efficient_wq, _work, 0);
which is racy when the hrtimer has expired and the work is currently
executed and has not yet managed to rearm the hrtimer.
Not a big problem as it just schedules work for nothing.
The new implementation has a safe guard in place to catch the case where
the hrtimer is queued on entry to the work function and avoids an extra
update attempt of the RTC that way.
Reported-by: Miroslav Lichvar
Signed-off-by: Thomas Gleixner
Cc: John Stultz
Cc: Prarit Bhargava
Cc: Jason Gunthorpe
---
include/linux/timex.h |1
kernel/time/ntp.c | 90 -
kernel/time/ntp_internal.h |7 +++
3 files changed, 55 insertions(+), 43 deletions(-)
--- a/include/linux/timex.h
+++ b/include/linux/timex.h
@@ -157,7 +157,6 @@ extern int do_clock_adjtime(const clocki
extern void hardpps(const struct timespec64 *, const struct timespec64 *);
int read_current_timer(unsigned long *timer_val);
-void ntp_notify_cmos_timer(void);
/* The clock frequency of the i8253/i8254 PIT */
#define PIT_TICK_RATE 1193182ul
--- a/kernel/time/ntp.c
+++ b/kernel/time/ntp.c
@@ -494,65 +494,55 @@ int second_overflow(time64_t secs)
return leap;
}
+#if defined(CONFIG_GENERIC_CMOS_UPDATE) || defined(CONFIG_RTC_SYSTOHC)
static void sync_hw_clock(struct work_struct *work);
-static DECLARE_DELAYED_WORK(sync_work, sync_hw_clock);
-
-static void sched_sync_hw_clock(struct timespec64 now,
- unsigned long target_nsec, bool fail)
+static DECLARE_WORK(sync_work, sync_hw_clock);
+static struct hrtimer sync_hrtimer;
+#define SYNC_PERIOD_NS (11UL * 60 * NSEC_PER_SEC)
+static enum hrtimer_restart sync_timer_callback(struct hrtimer *timer)
{
- struct timespec64 next;
+ queue_work(system_power_efficient_wq, _work);
- ktime_get_real_ts64();
- if (!fail)
- next.tv_sec = 659;
- else {
- /*
-* Try again as soon as possible. Delaying long periods
-* decreases the accuracy of the work queue timer. Due to this
-* the algorithm is very likely to require a short-sleep retry
-* after the above long sleep to synchronize ts_nsec.
-*/
- next.tv_sec = 0;
- }
+ return HRTIMER_NORESTART;
+}
- /* Compute the needed delay that will get to tv_nsec == target_nsec */
- next.tv_nsec = target_nsec - next.tv_nsec;
- if (next.tv_nsec <= 0)
- next.tv_nsec += NSEC_PER_SEC;
- if (next.tv_nsec >= NSEC_PER_SEC)
