| Message ID | 504E8372.20904@linaro.org (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
On 09/11/2012 02:18 AM, John Stultz wrote: > On 09/10/2012 12:45 PM, Daniel Lezcano wrote: >> On 09/10/2012 07:14 PM, John Stultz wrote: >>> In the meantime, I'll try to reproduce on my T61. If you could send me >>> your .config, I'd appreciate it. >> http://pastebin.com/qSxqfdDK >> >> The header of the config file shows for a v3.5-rc7 because it is the >> result of the git-bisect. If you keep this config file for the latest >> kernel that should reproduce the problem. >> >> Let me know if you were able to reproduce the problem. > Great! With this I was able to quickly reproduce the problem and I think > I have a fix. Cool ! > Would you mind testing the following patch? It seems to resolve the > issue, but I've not yet run it through my test suite to make sure it > didn't break anything else. No problem, I will try it this evening. Is this problem related to all 32bits arch ? Thanks ! -- Daniel > If both your and my testing comes back ok, I'll submit it to Thomas. > > thanks > -john > > From f10a285a5b532a14d3330f6e60e4d7bd5627932a Mon Sep 17 00:00:00 2001 > From: John Stultz <john.stultz@linaro.org> > Date: Mon, 10 Sep 2012 20:00:15 -0400 > Subject: [PATCH] time: Fix timeekeping_get_ns overflow on 32bit systems > > Daniel Lezcano reported seeing multi-second stalls from > keyboard input on his T61 laptop when NOHZ and CPU_IDLE > were enabled on a 32bit kernel. > > He bisected the problem down to > 1e75fa8be9fb61e1af46b5b3b176347a4c958ca1 (time: Condense > timekeeper.xtime into xtime_sec). > > After reproducing this issue, I narrowed the problem down > to the fact that timekeeping_get_ns() returns a 64bit > nsec value that hasn't been accumulated. In some cases > this value was being then stored in timespec.tv_nsec > (which is a long). > > On 32bit systems, With idle times larger then 4 seconds > (or less, depending on the value of xtime_nsec), the > returned nsec value would overflow 32bits. This limited > kept time from increasing, causing timers to not expire. > > The fix is to make sure we don't directly store the > result of timekeeping_get_ns() into a tv_nsec field, > instead using a 64bit nsec value which can then be > added into the timespec via timespec_add_ns(). > > With this patch I cannot reproduce the issue. > > Cc: Ingo Molnar <mingo@kernel.org> > Cc: Richard Cochran <richardcochran@gmail.com> > Cc: Prarit Bhargava <prarit@redhat.com> > Cc: Thomas Gleixner <tglx@linutronix.de> > Cc: Daniel Lezcano <daniel.lezcano@linaro.org> > Reported-and-bisected-by: Daniel Lezcano <daniel.lezcano@linaro.org> > Signed-off-by: John Stultz <john.stultz@linaro.org> > --- > kernel/time/timekeeping.c | 19 ++++++++++++------- > 1 file changed, 12 insertions(+), 7 deletions(-) > > diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c > index 34e5eac..d3b91e7 100644 > --- a/kernel/time/timekeeping.c > +++ b/kernel/time/timekeeping.c > @@ -303,10 +303,11 @@ void getnstimeofday(struct timespec *ts) > seq = read_seqbegin(&tk->lock); > > ts->tv_sec = tk->xtime_sec; > - ts->tv_nsec = timekeeping_get_ns(tk); > + nsecs = timekeeping_get_ns(tk); > > } while (read_seqretry(&tk->lock, seq)); > > + ts->tv_nsec = 0; > timespec_add_ns(ts, nsecs); > } > EXPORT_SYMBOL(getnstimeofday); > @@ -345,6 +346,7 @@ void ktime_get_ts(struct timespec *ts) > { > struct timekeeper *tk = &timekeeper; > struct timespec tomono; > + s64 nsec; > unsigned int seq; > > WARN_ON(timekeeping_suspended); > @@ -352,13 +354,14 @@ void ktime_get_ts(struct timespec *ts) > do { > seq = read_seqbegin(&tk->lock); > ts->tv_sec = tk->xtime_sec; > - ts->tv_nsec = timekeeping_get_ns(tk); > + nsec = timekeeping_get_ns(tk); > tomono = tk->wall_to_monotonic; > > } while (read_seqretry(&tk->lock, seq)); > > - set_normalized_timespec(ts, ts->tv_sec + tomono.tv_sec, > - ts->tv_nsec + tomono.tv_nsec); > + ts->tv_sec += tomono.tv_sec; > + ts->tv_nsec = 0; > + timespec_add_ns(ts, nsec + tomono.tv_nsec); > } > EXPORT_SYMBOL_GPL(ktime_get_ts); > > @@ -1244,6 +1247,7 @@ void get_monotonic_boottime(struct timespec *ts) > { > struct timekeeper *tk = &timekeeper; > struct timespec tomono, sleep; > + s64 nsec; > unsigned int seq; > > WARN_ON(timekeeping_suspended); > @@ -1251,14 +1255,15 @@ void get_monotonic_boottime(struct timespec *ts) > do { > seq = read_seqbegin(&tk->lock); > ts->tv_sec = tk->xtime_sec; > - ts->tv_nsec = timekeeping_get_ns(tk); > + nsec = timekeeping_get_ns(tk); > tomono = tk->wall_to_monotonic; > sleep = tk->total_sleep_time; > > } while (read_seqretry(&tk->lock, seq)); > > - set_normalized_timespec(ts, ts->tv_sec + tomono.tv_sec + sleep.tv_sec, > - ts->tv_nsec + tomono.tv_nsec + sleep.tv_nsec); > + ts->tv_sec += tomono.tv_sec + sleep.tv_sec; > + ts->tv_nsec = 0; > + timespec_add_ns(ts, nsec + tomono.tv_nsec + sleep.tv_nsec); > } > EXPORT_SYMBOL_GPL(get_monotonic_boottime); >
On 09/10/2012 11:58 PM, Daniel Lezcano wrote: >> Would you mind testing the following patch? It seems to resolve the >> issue, but I've not yet run it through my test suite to make sure it >> didn't break anything else. > No problem, I will try it this evening. > > Is this problem related to all 32bits arch ? I believe so. Although it didn't appear in my 32bit testing w/ kvm, but I suspect that is due to my distro userland setting lots of timers so that we don't hit those multi-second idle times, which could overflow 32bit nanoseconds, or maybe some other kvm quirk. Anyway, let me know if your testing goes well. Thanks so much again for noticing and bisecting this down. -john -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 09/11/2012 02:18 AM, John Stultz wrote: > On 09/10/2012 12:45 PM, Daniel Lezcano wrote: >> On 09/10/2012 07:14 PM, John Stultz wrote: >>> In the meantime, I'll try to reproduce on my T61. If you could send me >>> your .config, I'd appreciate it. >> http://pastebin.com/qSxqfdDK >> >> The header of the config file shows for a v3.5-rc7 because it is the >> result of the git-bisect. If you keep this config file for the latest >> kernel that should reproduce the problem. >> >> Let me know if you were able to reproduce the problem. > Great! With this I was able to quickly reproduce the problem and I think > I have a fix. > > Would you mind testing the following patch? It seems to resolve the > issue, but I've not yet run it through my test suite to make sure it > didn't break anything else. > > If both your and my testing comes back ok, I'll submit it to Thomas. Sounds like this solves the problem. Without enough background on timers in general, I don't have an opinion about the patch itself but I can confirm the issue is no longer occurring. Thanks -- Daniel
diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c index 34e5eac..d3b91e7 100644 --- a/kernel/time/timekeeping.c +++ b/kernel/time/timekeeping.c @@ -303,10 +303,11 @@ void getnstimeofday(struct timespec *ts) seq = read_seqbegin(&tk->lock); ts->tv_sec = tk->xtime_sec; - ts->tv_nsec = timekeeping_get_ns(tk); + nsecs = timekeeping_get_ns(tk); } while (read_seqretry(&tk->lock, seq)); + ts->tv_nsec = 0; timespec_add_ns(ts, nsecs); } EXPORT_SYMBOL(getnstimeofday); @@ -345,6 +346,7 @@ void ktime_get_ts(struct timespec *ts) { struct timekeeper *tk = &timekeeper; struct timespec tomono; + s64 nsec; unsigned int seq; WARN_ON(timekeeping_suspended); @@ -352,13 +354,14 @@ void ktime_get_ts(struct timespec *ts) do { seq = read_seqbegin(&tk->lock); ts->tv_sec = tk->xtime_sec; - ts->tv_nsec = timekeeping_get_ns(tk); + nsec = timekeeping_get_ns(tk); tomono = tk->wall_to_monotonic; } while (read_seqretry(&tk->lock, seq)); - set_normalized_timespec(ts, ts->tv_sec + tomono.tv_sec, - ts->tv_nsec + tomono.tv_nsec); + ts->tv_sec += tomono.tv_sec; + ts->tv_nsec = 0; + timespec_add_ns(ts, nsec + tomono.tv_nsec); } EXPORT_SYMBOL_GPL(ktime_get_ts); @@ -1244,6 +1247,7 @@ void get_monotonic_boottime(struct timespec *ts) { struct timekeeper *tk = &timekeeper; struct timespec tomono, sleep; + s64 nsec; unsigned int seq; WARN_ON(timekeeping_suspended); @@ -1251,14 +1255,15 @@ void get_monotonic_boottime(struct timespec *ts) do { seq = read_seqbegin(&tk->lock); ts->tv_sec = tk->xtime_sec; - ts->tv_nsec = timekeeping_get_ns(tk); + nsec = timekeeping_get_ns(tk); tomono = tk->wall_to_monotonic; sleep = tk->total_sleep_time; } while (read_seqretry(&tk->lock, seq)); - set_normalized_timespec(ts, ts->tv_sec + tomono.tv_sec + sleep.tv_sec, - ts->tv_nsec + tomono.tv_nsec + sleep.tv_nsec); + ts->tv_sec += tomono.tv_sec + sleep.tv_sec; + ts->tv_nsec = 0; + timespec_add_ns(ts, nsec + tomono.tv_nsec + sleep.tv_nsec); } EXPORT_SYMBOL_GPL(get_monotonic_boottime);