All of lore.kernel.org
 help / color / mirror / Atom feed
From: John Stultz <john.stultz@linaro.org>
To: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: sfr@canb.auug.org.au, peterz@infradead.org,
	LKML <linux-kernel@vger.kernel.org>,
	linuxppc-dev@ozlabs.org, Paul Mackerras <paulus@samba.org>,
	hpa@zytor.com, tglx@linutronix.de, Ingo Molnar <mingo@kernel.org>
Subject: Re: [tip:timers/core] timekeeping: Fixup typo in update_vsyscall_old definition
Date: Tue, 12 Aug 2014 08:30:40 -0700	[thread overview]
Message-ID: <53EA3320.90908@linaro.org> (raw)
In-Reply-To: <1407730744.4508.67.camel@pasglop>

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/953dec21aed4038464fec02f96a2f1b8=
701a5bce
>> 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

WARNING: multiple messages have this Message-ID (diff)
From: John Stultz <john.stultz@linaro.org>
To: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Ingo Molnar <mingo@kernel.org>,
	hpa@zytor.com, LKML <linux-kernel@vger.kernel.org>,
	peterz@infradead.org, tglx@linutronix.de, sfr@canb.auug.org.au,
	linuxppc-dev@ozlabs.org, Paul Mackerras <paulus@samba.org>
Subject: Re: [tip:timers/core] timekeeping: Fixup typo in update_vsyscall_old definition
Date: Tue, 12 Aug 2014 08:30:40 -0700	[thread overview]
Message-ID: <53EA3320.90908@linaro.org> (raw)
In-Reply-To: <1407730744.4508.67.camel@pasglop>

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







  reply	other threads:[~2014-08-12 15:30 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-07-26  4:37 [PATCH] timekeeping: Fixup typo in update_vsyscall_old definition John Stultz
2014-07-30  7:31 ` [tip:timers/core] " tip-bot for John Stultz
2014-08-11  4:19   ` Benjamin Herrenschmidt
2014-08-11  4:19     ` Benjamin Herrenschmidt
2014-08-12 15:30     ` John Stultz [this message]
2014-08-12 15:30       ` John Stultz
2014-08-13  4:03       ` Tony Breeds
2014-08-13  4:03         ` Tony Breeds

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=53EA3320.90908@linaro.org \
    --to=john.stultz@linaro.org \
    --cc=benh@kernel.crashing.org \
    --cc=hpa@zytor.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linuxppc-dev@ozlabs.org \
    --cc=mingo@kernel.org \
    --cc=paulus@samba.org \
    --cc=peterz@infradead.org \
    --cc=sfr@canb.auug.org.au \
    --cc=tglx@linutronix.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.