| Message ID | 1406349439-11785-1-git-send-email-john.stultz@linaro.org |
|---|---|
| State | Accepted |
| Commit | 953dec21aed4038464fec02f96a2f1b8701a5bce |
| Headers | show |
On Wed, 2014-07-30 at 00:31 -0700, tip-bot for John Stultz wrote: > Commit-ID: 953dec21aed4038464fec02f96a2f1b8701a5bce > Gitweb: http://git.kernel.org/tip/953dec21aed4038464fec02f96a2f1b8701a5bce > Author: John Stultz <john.stultz@linaro.org> > AuthorDate: Fri, 25 Jul 2014 21:37:19 -0700 > Committer: Thomas Gleixner <tglx@linutronix.de> > CommitDate: Wed, 30 Jul 2014 09:26:25 +0200 > > timekeeping: Fixup typo in update_vsyscall_old definition > > In commit 4a0e637738f0 ("clocksource: Get rid of cycle_last"), > currently in the -tip tree, there was a small typo where cycles_t > was used intstead of cycle_t. This broke ppc64 builds. There's another bug in there... You fix timespec vs. timespec64 for the first argument of update_vsyscall_old but not the second one ... (wall_to_monotonic). Also, in e2dff1ec0 you claim this is "minor", you seem to forget that arch/powerpc also deals with 32-bit kernels which use the same time keeping code, so we have a pretty serious regressions here... BTW. Is there some documentation you can point me to to figure out what replace that "_OLD" stuff so we can update to whatever is "new" ? Cheers, Ben. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
On 08/10/2014 09:19 PM, Benjamin Herrenschmidt wrote: > On Wed, 2014-07-30 at 00:31 -0700, tip-bot for John Stultz wrote: >> Commit-ID: 953dec21aed4038464fec02f96a2f1b8701a5bce >> Gitweb: http://git.kernel.org/tip/953dec21aed4038464fec02f96a2f1b8701a5bce >> Author: John Stultz <john.stultz@linaro.org> >> AuthorDate: Fri, 25 Jul 2014 21:37:19 -0700 >> Committer: Thomas Gleixner <tglx@linutronix.de> >> CommitDate: Wed, 30 Jul 2014 09:26:25 +0200 >> >> timekeeping: Fixup typo in update_vsyscall_old definition >> >> In commit 4a0e637738f0 ("clocksource: Get rid of cycle_last"), >> currently in the -tip tree, there was a small typo where cycles_t >> was used intstead of cycle_t. This broke ppc64 builds. > There's another bug in there... You fix timespec vs. timespec64 for the > first argument of update_vsyscall_old but not the second one ... > (wall_to_monotonic). > > Also, in e2dff1ec0 you claim this is "minor", you seem to forget that > arch/powerpc also deals with 32-bit kernels which use the same time > keeping code, so we have a pretty serious regressions here... Yikes. My apologies. I had missed that issue and had forgotten ppc32 has the vsyscall support as well. Thanks for pointing it out. I'll send a fix here shortly (though I only have the ppc64le toolchain handy, so forgive me if its not quite right). > BTW. Is there some documentation you can point me to to figure out what > replace that "_OLD" stuff so we can update to whatever is "new" ? So there's not exactly documentation, but the idea is rather then doing: nsecs + (mult*(now - cycle_last) >>shift); We're preserving the sub-nanosecond precision, and doing: (shifted_nsecs + mult*(now-cycle_last)) >> shift This avoids the rounding up 1ns every tick, which we did to avoid errors from truncating the precision. I think the hard part for ppc, is if I recall, ppc's vsyscall exports the xsec unit, which less granular then nanoseconds, so it had its own version of the same precision truncation issues. I recall Paul working on that, and I thought his solution was to reduce the multiplier by one so the inter-tick time was slightly slower, then the tick update would catch up causing a slight stair-step, which isn't ideal but is better then the inconsistencies that came out of not-handling the precision properly. That said, skimming the ppc code, I don't see that logic right off, and have a fuzzy memory that maybe that solution was RHEL5 specific or something like that. thanks -john -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
On Tue, Aug 12, 2014 at 08:30:40AM -0700, John Stultz wrote: > Thanks for pointing it out. I'll send a fix here shortly (though I only > have the ppc64le toolchain handy, so forgive me if its not quite right). /me pimps the kernel.org toolchains: https://www.kernel.org/pub/tools/crosstool/files/bin/x86_64/4.9.0/x86_64-gcc-4.9.0-nolibc_powerpc64-linux.tar.xz and gets back in his box. Yours Tony
diff --git a/arch/ia64/kernel/time.c b/arch/ia64/kernel/time.c index 11dc42d..3e71ef8 100644 --- a/arch/ia64/kernel/time.c +++ b/arch/ia64/kernel/time.c @@ -441,7 +441,7 @@ void update_vsyscall_tz(void) } void update_vsyscall_old(struct timespec *wall, struct timespec *wtm, - struct clocksource *c, u32 mult, cycles_t cycle_last) + struct clocksource *c, u32 mult, cycle_t cycle_last) { write_seqcount_begin(&fsyscall_gtod_data.seq); diff --git a/include/linux/timekeeper_internal.h b/include/linux/timekeeper_internal.h index e9660e5..95640dc 100644 --- a/include/linux/timekeeper_internal.h +++ b/include/linux/timekeeper_internal.h @@ -113,7 +113,7 @@ extern void update_vsyscall_tz(void); extern void update_vsyscall_old(struct timespec *ts, struct timespec *wtm, struct clocksource *c, u32 mult, - cycles_t cycle_last); + cycle_t cycle_last); extern void update_vsyscall_tz(void); #else
In commit 4a0e637738f0 ("clocksource: Get rid of cycle_last"), currently in the -tip tree, there was a small typo where cycles_t was used intstead of cycle_t. This broke ppc64 builds. Fix this by using the proper cycle_t type for this usage, in both the definition and the ia64 implementation. Now, having both cycle_t and cycles_t types seems like a very bad idea just asking for these sorts of issues. But that will be a cleanup for another day. Cc: Stephen Rothwell <sfr@canb.auug.org.au> Cc: Thomas Gleixner <tglx@linutronix.de> Cc: Ingo Molnar <mingo@elte.hu> Cc: "H. Peter Anvin" <hpa@zytor.com> Cc: Peter Zijlstra <peterz@infradead.org> Reported-by: Stephen Rothwell <sfr@canb.auug.org.au> Signed-off-by: John Stultz <john.stultz@linaro.org> --- Note: This should be visibly correct, and I've test built on ppc64, but I don't have an ia64 toolchain, so if anyone could give this a build whirl on ia64, I'd appreciate it. arch/ia64/kernel/time.c | 2 +- include/linux/timekeeper_internal.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)