Re: [RFC PATCH v2 0/4] Add support for S3 non-stop TSC support.
On Tue, Mar 05, 2013 at 02:17:59PM +0800, John Stultz wrote:
> On 03/05/2013 12:32 PM, Jason Gunthorpe wrote:
> >On Tue, Mar 05, 2013 at 11:53:02AM +0800, Feng Tang wrote:
> >
> >>>// Drops some small precision along the way but is simple..
> >>>static inline u64 cyclecounter_cyc2ns_128(const struct cyclecounter *cc,
> >>> cycle_t cycles)
> >>>{
> >>> u64 max = U64_MAX/cc->mult;
> >>> u64 num = cycles/max;
> >>> u64 result = num * ((max * cc->mult) >> cc->shift);
> >>> return result + cyclecounter_cyc2ns(cc, cycles - num*cc->mult);
> >>>}
> >
> Probably want to use clocksource instead of cyclecounter, but I
> think Jason's approach sounds ok. I might suggest that you initially
> make the function static to the timekeeping code, just so we don't
> get unexpected users.
Thought more about it, can we directly make clocksource_cyc2ns() cover
the overflow case? Something like:
diff --git a/include/linux/clocksource.h b/include/linux/clocksource.h
index aa7032c..1ecc872 100644
--- a/include/linux/clocksource.h
+++ b/include/linux/clocksource.h
@@ -274,7 +274,16 @@ static inline u32 clocksource_hz2mult(u32 hz, u32
shift_constant)
*/
static inline s64 clocksource_cyc2ns(cycle_t cycles, u32 mult, u32 shift)
{
- return ((u64) cycles * mult) >> shift;
+ u64 max = ULLONG_MAX / mult;
+ s64 nsec = 0;
+
+ /* The (mult * cycles) may overflow 64 bits, so add a max check */
+ if (cycles > max) {
+ nsec = ((max * mult) >> shift) * (cycles / max);
+ cycles %= max;
+ }
+ nsec += ((u64) cycles * mult) >> shift;
+ return nsec;
}
Thanks,
Feng
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH v2 0/4] Add support for S3 non-stop TSC support.
On Tue, Mar 05, 2013 at 02:17:59PM +0800, John Stultz wrote:
On 03/05/2013 12:32 PM, Jason Gunthorpe wrote:
On Tue, Mar 05, 2013 at 11:53:02AM +0800, Feng Tang wrote:
// Drops some small precision along the way but is simple..
static inline u64 cyclecounter_cyc2ns_128(const struct cyclecounter *cc,
cycle_t cycles)
{
u64 max = U64_MAX/cc-mult;
u64 num = cycles/max;
u64 result = num * ((max * cc-mult) cc-shift);
return result + cyclecounter_cyc2ns(cc, cycles - num*cc-mult);
}
Probably want to use clocksource instead of cyclecounter, but I
think Jason's approach sounds ok. I might suggest that you initially
make the function static to the timekeeping code, just so we don't
get unexpected users.
Thought more about it, can we directly make clocksource_cyc2ns() cover
the overflow case? Something like:
diff --git a/include/linux/clocksource.h b/include/linux/clocksource.h
index aa7032c..1ecc872 100644
--- a/include/linux/clocksource.h
+++ b/include/linux/clocksource.h
@@ -274,7 +274,16 @@ static inline u32 clocksource_hz2mult(u32 hz, u32
shift_constant)
*/
static inline s64 clocksource_cyc2ns(cycle_t cycles, u32 mult, u32 shift)
{
- return ((u64) cycles * mult) shift;
+ u64 max = ULLONG_MAX / mult;
+ s64 nsec = 0;
+
+ /* The (mult * cycles) may overflow 64 bits, so add a max check */
+ if (cycles max) {
+ nsec = ((max * mult) shift) * (cycles / max);
+ cycles %= max;
+ }
+ nsec += ((u64) cycles * mult) shift;
+ return nsec;
}
Thanks,
Feng
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH v2 0/4] Add support for S3 non-stop TSC support.
On Mon, Mar 04, 2013 at 09:32:03PM -0700, Jason Gunthorpe wrote: > On Tue, Mar 05, 2013 at 11:53:02AM +0800, Feng Tang wrote: > > > You may want to also CC the maintainers of all the ARM subsystems that > > > use read_persistent_clock and check with them to ensure this new > > > interface will let them migrate their implementations as well. > > > > Maybe I didn't get it well, my patches didn't change the > > read_persistent_clock(), but inject a new way of counting suspended > > time. It should have no functional changes to existing platforms. > > Right, your patches are fine stand alone. > > The ARM case of plat-omap/counter_32k.c would ideally be converted to > use your new API though, that is what I ment about involving them. I see now. Yes, the counter_32k could be converted to a clocksource with SUSPEND_NONSTOP flag set, and no need for it to use the read_persistent_clock any more. > > I'm not sure about mach-tegra/timer.c though - it seems to be using a > counter as well but somehow sharing registers with the RTC? I just searched the 3.9-rc1 code, seems the file has been moved to drivers/clocksource/tegra20_timer.c, and its persistent clock seems to also be based on a RTC like device. Thanks, Feng -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH v2 0/4] Add support for S3 non-stop TSC support.
On 03/05/2013 12:32 PM, Jason Gunthorpe wrote:
On Tue, Mar 05, 2013 at 11:53:02AM +0800, Feng Tang wrote:
// Drops some small precision along the way but is simple..
static inline u64 cyclecounter_cyc2ns_128(const struct cyclecounter *cc,
cycle_t cycles)
{
u64 max = U64_MAX/cc->mult;
u64 num = cycles/max;
u64 result = num * ((max * cc->mult) >> cc->shift);
return result + cyclecounter_cyc2ns(cc, cycles - num*cc->mult);
}
Your way is surely more accurate, if maintainers are ok with adding
the new API, I will use it.
Okay, give it a good look though, I only wrote it out in email, never
tested it :)
Probably want to use clocksource instead of cyclecounter, but I think
Jason's approach sounds ok. I might suggest that you initially make the
function static to the timekeeping code, just so we don't get unexpected
users.
thanks
-john
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH v2 0/4] Add support for S3 non-stop TSC support.
On Tue, Mar 05, 2013 at 11:53:02AM +0800, Feng Tang wrote:
> > // Drops some small precision along the way but is simple..
> > static inline u64 cyclecounter_cyc2ns_128(const struct cyclecounter *cc,
> > cycle_t cycles)
> > {
> > u64 max = U64_MAX/cc->mult;
> > u64 num = cycles/max;
> > u64 result = num * ((max * cc->mult) >> cc->shift);
> > return result + cyclecounter_cyc2ns(cc, cycles - num*cc->mult);
> > }
>
> Your way is surely more accurate, if maintainers are ok with adding
> the new API, I will use it.
Okay, give it a good look though, I only wrote it out in email, never
tested it :)
> > You may want to also CC the maintainers of all the ARM subsystems that
> > use read_persistent_clock and check with them to ensure this new
> > interface will let them migrate their implementations as well.
>
> Maybe I didn't get it well, my patches didn't change the
> read_persistent_clock(), but inject a new way of counting suspended
> time. It should have no functional changes to existing platforms.
Right, your patches are fine stand alone.
The ARM case of plat-omap/counter_32k.c would ideally be converted to
use your new API though, that is what I ment about involving them.
I'm not sure about mach-tegra/timer.c though - it seems to be using a
counter as well but somehow sharing registers with the RTC?
Regards,
Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH v2 0/4] Add support for S3 non-stop TSC support.
Hi Jason,
Thanks for the review and suggestions!
On Mon, Mar 04, 2013 at 08:20:36PM -0700, Jason Gunthorpe wrote:
> On Tue, Mar 05, 2013 at 02:34:25AM +, Tang, Feng wrote:
> > Hi Jason,
> >
> > Sorry, I forgot to add you in cc list in the first place. Please
> > help to review the patch series, thanks!
>
> Sure, I didn't get CC'd on the patches, so this is an imperfect reply,
> but..
I've copied your comments back to the main thread.
>
> Did you consider an approach closer to the function I outlined to
> Jonh:
>
> // Drops some small precision along the way but is simple..
> static inline u64 cyclecounter_cyc2ns_128(const struct cyclecounter *cc,
> cycle_t cycles)
> {
> u64 max = U64_MAX/cc->mult;
> u64 num = cycles/max;
> u64 result = num * ((max * cc->mult) >> cc->shift);
> return result + cyclecounter_cyc2ns(cc, cycles - num*cc->mult);
> }
Your way is surely more accurate, if maintainers are ok with adding
the new API, I will use it.
>
> Rather than the while loop, which, I suspect, drops more precision
> that something like the above. (replace cyclecounter with clocksource)
> At the very least, keeping it as a distinct inline will let someone
> come by one day and implement a proper 128 bit multiply...
>
> You may want to also CC the maintainers of all the ARM subsystems that
> use read_persistent_clock and check with them to ensure this new
> interface will let them migrate their implementations as well.
Maybe I didn't get it well, my patches didn't change the
read_persistent_clock(),
but inject a new way of counting suspended time. It should have no
functional changes to existing platforms.
> > * Solve the problem of judging S3/S4, as the clocksource
> > counter will be reset after coming out S4.
>
> Hrm, what if it wraps during suspend? This probably isn't a problem
> for a 64 bit TSC though..
Nice catch! I've added a wrap check in the patch, and will stop to use it
if there is a wrap founded.
+ if ((clock->flags & CLOCK_SOURCE_SUSPEND_NOTSTOP) &&
+ cycle_now > clock->cycle_last) {
>
> Is it impossible to track if S4 or S3 was entered in the clocksource?
Currently there is no easy way to check it.
Thanks,
Feng
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: FW: [RFC PATCH v2 0/4] Add support for S3 non-stop TSC support.
On Tue, Mar 05, 2013 at 02:34:25AM +, Tang, Feng wrote:
> Hi Jason,
>
> Sorry, I forgot to add you in cc list in the first place. Please
> help to review the patch series, thanks!
Sure, I didn't get CC'd on the patches, so this is an imperfect reply,
but..
Did you consider an approach closer to the function I outlined to
Jonh:
// Drops some small precision along the way but is simple..
static inline u64 cyclecounter_cyc2ns_128(const struct cyclecounter *cc,
cycle_t cycles)
{
u64 max = U64_MAX/cc->mult;
u64 num = cycles/max;
u64 result = num * ((max * cc->mult) >> cc->shift);
return result + cyclecounter_cyc2ns(cc, cycles - num*cc->mult);
}
Rather than the while loop, which, I suspect, drops more precision
that something like the above. (replace cyclecounter with clocksource)
At the very least, keeping it as a distinct inline will let someone
come by one day and implement a proper 128 bit multiply...
You may want to also CC the maintainers of all the ARM subsystems that
use read_persistent_clock and check with them to ensure this new
interface will let them migrate their implementations as well.
> * Solve the problem of judging S3/S4, as the clocksource
> counter will be reset after coming out S4.
Hrm, what if it wraps during suspend? This probably isn't a problem
for a 64 bit TSC though..
Is it impossible to track if S4 or S3 was entered in the clocksource?
Reagrds,
Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[RFC PATCH v2 0/4] Add support for S3 non-stop TSC support.
Hi All, On some new Intel Atom processors (Penwell and Cloverview), there is a feature that the TSC won't stop in S3, say the TSC value won't be reset to 0 after resume. This feature makes TSC a more reliable clocksource and could benefit the timekeeping code during system suspend/resume cycles. The enabling efforts include adding new flags for this feature, modifying clocksource.c and timekeeping.c to support and utilizing it. The major change to timekeeping is the way to count suspened time, current way is trying to use the persistent clock first, and then try the rtc if persistent clock can't do it. This patch will change the trying order to: suspend-nonstop clocksource -> persistent clock -> rtc Please help to review them, thanks a lot! Changelog: v2: * Dump the code changing a clocksource's mult and shit, as suggested by John to not hurt accuracy * Modify the CPU feature flag name to be consistent with other flags * Solve the problem of judging S3/S4, as the clocksource counter will be reset after coming out S4. - Feng Feng Tang (4): x86: Add cpu capability flag X86_FEATURE_NONSTOP_TSC_S3 clocksource: Add new feature flag CLOCK_SOURCE_SUSPEND_NOTSTOP x86: tsc: Add support for new S3_NOTSTOP feature timekeeping: utilize the suspend-nonstop clocksource to count suspended time arch/x86/include/asm/cpufeature.h |1 + arch/x86/kernel/cpu/intel.c | 12 arch/x86/kernel/tsc.c |6 +++- include/linux/clocksource.h |1 + kernel/time/timekeeping.c | 57 +++- 5 files changed, 68 insertions(+), 9 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[RFC PATCH v2 0/4] Add support for S3 non-stop TSC support.
Hi All, On some new Intel Atom processors (Penwell and Cloverview), there is a feature that the TSC won't stop in S3, say the TSC value won't be reset to 0 after resume. This feature makes TSC a more reliable clocksource and could benefit the timekeeping code during system suspend/resume cycles. The enabling efforts include adding new flags for this feature, modifying clocksource.c and timekeeping.c to support and utilizing it. The major change to timekeeping is the way to count suspened time, current way is trying to use the persistent clock first, and then try the rtc if persistent clock can't do it. This patch will change the trying order to: suspend-nonstop clocksource - persistent clock - rtc Please help to review them, thanks a lot! Changelog: v2: * Dump the code changing a clocksource's mult and shit, as suggested by John to not hurt accuracy * Modify the CPU feature flag name to be consistent with other flags * Solve the problem of judging S3/S4, as the clocksource counter will be reset after coming out S4. - Feng Feng Tang (4): x86: Add cpu capability flag X86_FEATURE_NONSTOP_TSC_S3 clocksource: Add new feature flag CLOCK_SOURCE_SUSPEND_NOTSTOP x86: tsc: Add support for new S3_NOTSTOP feature timekeeping: utilize the suspend-nonstop clocksource to count suspended time arch/x86/include/asm/cpufeature.h |1 + arch/x86/kernel/cpu/intel.c | 12 arch/x86/kernel/tsc.c |6 +++- include/linux/clocksource.h |1 + kernel/time/timekeeping.c | 57 +++- 5 files changed, 68 insertions(+), 9 deletions(-) -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: FW: [RFC PATCH v2 0/4] Add support for S3 non-stop TSC support.
On Tue, Mar 05, 2013 at 02:34:25AM +, Tang, Feng wrote:
Hi Jason,
Sorry, I forgot to add you in cc list in the first place. Please
help to review the patch series, thanks!
Sure, I didn't get CC'd on the patches, so this is an imperfect reply,
but..
Did you consider an approach closer to the function I outlined to
Jonh:
// Drops some small precision along the way but is simple..
static inline u64 cyclecounter_cyc2ns_128(const struct cyclecounter *cc,
cycle_t cycles)
{
u64 max = U64_MAX/cc-mult;
u64 num = cycles/max;
u64 result = num * ((max * cc-mult) cc-shift);
return result + cyclecounter_cyc2ns(cc, cycles - num*cc-mult);
}
Rather than the while loop, which, I suspect, drops more precision
that something like the above. (replace cyclecounter with clocksource)
At the very least, keeping it as a distinct inline will let someone
come by one day and implement a proper 128 bit multiply...
You may want to also CC the maintainers of all the ARM subsystems that
use read_persistent_clock and check with them to ensure this new
interface will let them migrate their implementations as well.
* Solve the problem of judging S3/S4, as the clocksource
counter will be reset after coming out S4.
Hrm, what if it wraps during suspend? This probably isn't a problem
for a 64 bit TSC though..
Is it impossible to track if S4 or S3 was entered in the clocksource?
Reagrds,
Jason
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH v2 0/4] Add support for S3 non-stop TSC support.
Hi Jason,
Thanks for the review and suggestions!
On Mon, Mar 04, 2013 at 08:20:36PM -0700, Jason Gunthorpe wrote:
On Tue, Mar 05, 2013 at 02:34:25AM +, Tang, Feng wrote:
Hi Jason,
Sorry, I forgot to add you in cc list in the first place. Please
help to review the patch series, thanks!
Sure, I didn't get CC'd on the patches, so this is an imperfect reply,
but..
I've copied your comments back to the main thread.
Did you consider an approach closer to the function I outlined to
Jonh:
// Drops some small precision along the way but is simple..
static inline u64 cyclecounter_cyc2ns_128(const struct cyclecounter *cc,
cycle_t cycles)
{
u64 max = U64_MAX/cc-mult;
u64 num = cycles/max;
u64 result = num * ((max * cc-mult) cc-shift);
return result + cyclecounter_cyc2ns(cc, cycles - num*cc-mult);
}
Your way is surely more accurate, if maintainers are ok with adding
the new API, I will use it.
Rather than the while loop, which, I suspect, drops more precision
that something like the above. (replace cyclecounter with clocksource)
At the very least, keeping it as a distinct inline will let someone
come by one day and implement a proper 128 bit multiply...
You may want to also CC the maintainers of all the ARM subsystems that
use read_persistent_clock and check with them to ensure this new
interface will let them migrate their implementations as well.
Maybe I didn't get it well, my patches didn't change the
read_persistent_clock(),
but inject a new way of counting suspended time. It should have no
functional changes to existing platforms.
* Solve the problem of judging S3/S4, as the clocksource
counter will be reset after coming out S4.
Hrm, what if it wraps during suspend? This probably isn't a problem
for a 64 bit TSC though..
Nice catch! I've added a wrap check in the patch, and will stop to use it
if there is a wrap founded.
+ if ((clock-flags CLOCK_SOURCE_SUSPEND_NOTSTOP)
+ cycle_now clock-cycle_last) {
Is it impossible to track if S4 or S3 was entered in the clocksource?
Currently there is no easy way to check it.
Thanks,
Feng
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH v2 0/4] Add support for S3 non-stop TSC support.
On Tue, Mar 05, 2013 at 11:53:02AM +0800, Feng Tang wrote:
// Drops some small precision along the way but is simple..
static inline u64 cyclecounter_cyc2ns_128(const struct cyclecounter *cc,
cycle_t cycles)
{
u64 max = U64_MAX/cc-mult;
u64 num = cycles/max;
u64 result = num * ((max * cc-mult) cc-shift);
return result + cyclecounter_cyc2ns(cc, cycles - num*cc-mult);
}
Your way is surely more accurate, if maintainers are ok with adding
the new API, I will use it.
Okay, give it a good look though, I only wrote it out in email, never
tested it :)
You may want to also CC the maintainers of all the ARM subsystems that
use read_persistent_clock and check with them to ensure this new
interface will let them migrate their implementations as well.
Maybe I didn't get it well, my patches didn't change the
read_persistent_clock(), but inject a new way of counting suspended
time. It should have no functional changes to existing platforms.
Right, your patches are fine stand alone.
The ARM case of plat-omap/counter_32k.c would ideally be converted to
use your new API though, that is what I ment about involving them.
I'm not sure about mach-tegra/timer.c though - it seems to be using a
counter as well but somehow sharing registers with the RTC?
Regards,
Jason
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH v2 0/4] Add support for S3 non-stop TSC support.
On 03/05/2013 12:32 PM, Jason Gunthorpe wrote:
On Tue, Mar 05, 2013 at 11:53:02AM +0800, Feng Tang wrote:
// Drops some small precision along the way but is simple..
static inline u64 cyclecounter_cyc2ns_128(const struct cyclecounter *cc,
cycle_t cycles)
{
u64 max = U64_MAX/cc-mult;
u64 num = cycles/max;
u64 result = num * ((max * cc-mult) cc-shift);
return result + cyclecounter_cyc2ns(cc, cycles - num*cc-mult);
}
Your way is surely more accurate, if maintainers are ok with adding
the new API, I will use it.
Okay, give it a good look though, I only wrote it out in email, never
tested it :)
Probably want to use clocksource instead of cyclecounter, but I think
Jason's approach sounds ok. I might suggest that you initially make the
function static to the timekeeping code, just so we don't get unexpected
users.
thanks
-john
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH v2 0/4] Add support for S3 non-stop TSC support.
On Mon, Mar 04, 2013 at 09:32:03PM -0700, Jason Gunthorpe wrote: On Tue, Mar 05, 2013 at 11:53:02AM +0800, Feng Tang wrote: You may want to also CC the maintainers of all the ARM subsystems that use read_persistent_clock and check with them to ensure this new interface will let them migrate their implementations as well. Maybe I didn't get it well, my patches didn't change the read_persistent_clock(), but inject a new way of counting suspended time. It should have no functional changes to existing platforms. Right, your patches are fine stand alone. The ARM case of plat-omap/counter_32k.c would ideally be converted to use your new API though, that is what I ment about involving them. I see now. Yes, the counter_32k could be converted to a clocksource with SUSPEND_NONSTOP flag set, and no need for it to use the read_persistent_clock any more. I'm not sure about mach-tegra/timer.c though - it seems to be using a counter as well but somehow sharing registers with the RTC? I just searched the 3.9-rc1 code, seems the file has been moved to drivers/clocksource/tegra20_timer.c, and its persistent clock seems to also be based on a RTC like device. Thanks, Feng -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
