Messages in this thread | | | Date | Wed, 13 May 2015 17:01:32 -0700 | Subject | Re: [PATCH] timekeeping: always make sure wall_to_monotonic is negative | From | John Stultz <> |
| |
On Mon, Mar 16, 2015 at 12:28 PM, Wang YanQing <udknight@gmail.com> wrote: > > When wall_to_monotonic become positive, then I get below two > errors on an IMX6 development board without enable RTC device: > > 1:execute exportfs -a generate: > "exportfs: /opt/nfs/arm does not support NFS export" > 2:cat /proc/stat: > "btime 4294967236" > > I can product the same issues on x86 with newest kernel in next > tree with below c code: > " > int main(void) > { > struct timeval val; > int ret; > > val.tv_sec = 0; > val.tv_usec = 0; > ret = settimeofday(&val, NULL); > printf("ret:%d\n", ret); > return 0; > } > " > The reason is getboottime[64] return negative value, cause below > codes don't work: > > nfs error:cache.h:get_expiry return negative value cause > cache_flush always clear entries just added in > ip_map_parse. > proc/stat error: > seq_printf use "unsigned long" to show > a negative number return by getboottime. > > This patch fix it by validate new value of wall_to_monotonic > before assign it.
Sorry for the slow response, and thanks for the problem report. Some questions below...
So to restate the problem: setting the CLOCK_REALTIME back to a timevalue that is less then the value of CLOCK_MONOTONIC causes problems with various interfaces such as nfs and proc/stat.
Is that right?
> Signed-off-by: Wang YanQing <udknight@gmail.com> > --- > kernel/time/timekeeping.c | 71 ++++++++++++++++++++++++++++++++++++----------- > 1 file changed, 55 insertions(+), 16 deletions(-) > > diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c > index 91db941..799e323 100644 > --- a/kernel/time/timekeeping.c > +++ b/kernel/time/timekeeping.c > @@ -96,11 +96,29 @@ static void tk_xtime_add(struct timekeeper *tk, const struct timespec64 *ts) > tk_normalize_xtime(tk); > } > > -static void tk_set_wall_to_mono(struct timekeeper *tk, struct timespec64 wtm) > +static inline bool timespec64_negative(struct timespec64 *ts) > +{ > + if (ts->tv_sec > 0) > + return false; > + if (ts->tv_sec == 0 && ts->tv_nsec > 0) > + return false; > + return true; > +} > + > +static bool tk_set_wall_to_mono(struct timekeeper *tk, struct timespec64 wtm) > { > struct timespec64 tmp; > > /* > + * The current time > + * wall_to_monotonic is what we need to add to xtime (or xtime corrected > + * for sub jiffie times) to get to monotonic time. Monotonic is pegged > + * at zero at system boot time, so wall_to_monotonic will be negative. > + */ > + if (!timespec64_negative(&wtm)) > + return false; > + > + /* > * Verify consistency of: offset_real = -wall_to_monotonic > * before modifying anything > */ > @@ -111,6 +129,7 @@ static void tk_set_wall_to_mono(struct timekeeper *tk, struct timespec64 wtm) > set_normalized_timespec64(&tmp, -wtm.tv_sec, -wtm.tv_nsec); > tk->offs_real = timespec64_to_ktime(tmp); > tk->offs_tai = ktime_add(tk->offs_real, ktime_set(tk->tai_offset, 0)); > + return true; > } > > static inline void tk_update_sleep_time(struct timekeeper *tk, ktime_t delta) > @@ -796,8 +815,9 @@ EXPORT_SYMBOL(do_gettimeofday); > int do_settimeofday64(const struct timespec64 *ts) > { > struct timekeeper *tk = &tk_core.timekeeper; > - struct timespec64 ts_delta, xt; > + struct timespec64 ts_delta, tmp; > unsigned long flags; > + int ret = 0; > > if (!timespec64_valid_strict(ts)) > return -EINVAL; > @@ -807,23 +827,27 @@ int do_settimeofday64(const struct timespec64 *ts) > > timekeeping_forward_now(tk); > > - xt = tk_xtime(tk); > - ts_delta.tv_sec = ts->tv_sec - xt.tv_sec; > - ts_delta.tv_nsec = ts->tv_nsec - xt.tv_nsec; > + tmp = tk_xtime(tk); > + ts_delta.tv_sec = ts->tv_sec - tmp.tv_sec; > + ts_delta.tv_nsec = ts->tv_nsec - tmp.tv_nsec; > > - tk_set_wall_to_mono(tk, timespec64_sub(tk->wall_to_monotonic, ts_delta)); > + tmp = timespec64_sub(tk->wall_to_monotonic, ts_delta); > + if (!tk_set_wall_to_mono(tk, tmp)) { > + ret = -EINVAL; > + goto out; > + }
It seems like a simpler check at the top of do_settimeofday64() would be easier then embedding the check in set_wall_to_mono and trying to deal with it later in the process of adjusting time.
> > tk_set_xtime(tk, ts); > - > timekeeping_update(tk, TK_CLEAR_NTP | TK_MIRROR | TK_CLOCK_WAS_SET); > - > +out: > write_seqcount_end(&tk_core.seq); > raw_spin_unlock_irqrestore(&timekeeper_lock, flags); > > /* signal hrtimers about time change */ > - clock_was_set(); > + if (!ret) > + clock_was_set(); > > - return 0; > + return ret; > } > EXPORT_SYMBOL(do_settimeofday64); > > @@ -857,8 +881,13 @@ int timekeeping_inject_offset(struct timespec *ts) > goto error; > } > > + tmp = timespec64_sub(tk->wall_to_monotonic, ts64); > + if (!tk_set_wall_to_mono(tk, tmp)) { > + ret = -EINVAL; > + goto error; > + } > +
Similarly here. A simple check at the top before we make any adjustments would make more sense to me.
> tk_xtime_add(tk, &ts64); > - tk_set_wall_to_mono(tk, timespec64_sub(tk->wall_to_monotonic, ts64)); > > error: /* even if we error out, we forwarded the time, so call update */ > timekeeping_update(tk, TK_CLEAR_NTP | TK_MIRROR | TK_CLOCK_WAS_SET); > @@ -1140,14 +1169,21 @@ static struct timespec64 timekeeping_suspend_time; > static void __timekeeping_inject_sleeptime(struct timekeeper *tk, > struct timespec64 *delta) > { > + struct timespec64 tmp; > + > if (!timespec64_valid_strict(delta)) { > printk_deferred(KERN_WARNING > "__timekeeping_inject_sleeptime: Invalid " > "sleep delta value!\n"); > return; > } > + > + tmp = timespec64_sub(tk->wall_to_monotonic, *delta); > + if (!tk_set_wall_to_mono(tk, tmp)) { > + WARN_ON(1); > + return; > + }
Is this really possible? _inject_sleeptime should never be negative. Or am I missing something?
> tk_xtime_add(tk, delta); > - tk_set_wall_to_mono(tk, timespec64_sub(tk->wall_to_monotonic, *delta)); > tk_update_sleep_time(tk, timespec64_to_ktime(*delta)); > tk_debug_account_sleep_time(delta); > } > @@ -1539,14 +1575,17 @@ static inline unsigned int accumulate_nsecs_to_secs(struct timekeeper *tk) > leap = second_overflow(tk->xtime_sec); > if (unlikely(leap)) { > struct timespec64 ts; > - > - tk->xtime_sec += leap; > + bool ret; > > ts.tv_sec = leap; > ts.tv_nsec = 0; > - tk_set_wall_to_mono(tk, > + ret = tk_set_wall_to_mono(tk, > timespec64_sub(tk->wall_to_monotonic, ts));
How would adding time in accumulate_nsecs_to_secs ever cause wall_to_monotonic to not be negative?
> - > + if (!ret) { > + WARN_ON_ONCE(1); > + break; > + } > + tk->xtime_sec += leap; > __timekeeping_set_tai_offset(tk, tk->tai_offset - leap); > > clock_set = TK_CLOCK_WAS_SET; > -- > 2.2.2.dirty
| |