clocksource: avoid unnecessary overflow in cyclecounter_cyc2ns()
RFC PATCH 1/1 clocksource: avoid unnecessary overflow in cyclecounter_cyc2ns()
Greetings,
While rummaging around looking for HTH a gaggle of weird a$$ machines can manage to timewarp back and forth by exactly 208 days, I stumbled across $subject which looks like it may want to borrow Salman's fix.
clocksource: avoid unnecessary overflow in cyclecounter_cyc2ns()
As per 4cecf6d401a "sched, x86: Avoid unnecessary overflow in sched_clock", cycles * mult >> shift is overflow prone. so give it the same treatment.
--- include/linux/clocksource.h | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)
include/linux/clocksource.h +8 -3
@@ -77,13 +77,18 @@ struct timecounter {* * XXX - This could use some mult_lxl_ll() asm optimization. Same code * as in cyc2ns, but with unsigned result. +* +* Because it is the same as x86 __cycles_2_ns, give it the same treatment as +* commit 4cecf6d401a "sched, x86: Avoid unnecessary overflow in sched_clock" +* to avoid a potential cycles * mult overflow. */ static inline u64 cyclecounter_cyc2ns (const struct cyclecounter *cc,cycle_t cycles) { -u64 ret = (u64 )cycles; - ret = (ret * cc->mult) >> cc->shift; -return ret; +u64 quot = (u64 )cycles >> cc->shift; +u64 rem = (u64 )cycles & ((1ULL << cc->shift) -1 ); + +return quot * cc->mult + ((rem * cc->mult) >> cc->shift); }/**
Quoted 21 lines · Mike Galbraith
Greetings,
While rummaging around looking for HTH a gaggle of weird a$$ machines can manage to timewarp back and forth by exactly 208 days, I stumbled across $subject which looks like it may want to borrow Salman's fix.
clocksource: avoid unnecessary overflow in
cyclecounter_cyc2ns()As per
4cecf6d401a"sched, x86: Avoid unnecessary overflow insched_clock", cycles * mult >> shift is overflow prone. so give it the same treatment.Cc Salman Qazi <[email protected]>Cc John Stultz <[email protected]>Signed-off-by Mike Galbraith <[email protected]>--- include/linux/clocksource.h | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)
include/linux/clocksource.h struct timecounter { · line 83
Quoted 7 lines
include/linux/clocksource.h +4
-0@@ -77,13 +77,18 @@ struct timecounter {* * XXX - This could use some mult_lxl_ll() asm optimization. Same code * as in cyc2ns, but with unsigned result. +* +* Because it is the same as x86 __cycles_2_ns, give it the same treatment as +* commit 4cecf6d401a "sched, x86: Avoid unnecessary overflow in sched_clock" +* to avoid a potential cycles * mult overflow.
Do we normally reference a particular commit in a comment? Why not just grab the same comment and add a "this is grabbed from arch/x86/... ?
include/linux/clocksource.h struct timecounter { · line 92
Quoted 12 lines
include/linux/clocksource.h +4
-3*/ static inline u64 cyclecounter_cyc2ns (const struct cyclecounter *cc,cycle_t cycles) { -u64 ret = (u64 )cycles; - ret = (ret * cc->mult) >> cc->shift; -return ret; +u64 quot = (u64 )cycles >> cc->shift; +u64 rem = (u64 )cycles & ((1ULL << cc->shift) -1 ); + +return quot * cc->mult + ((rem * cc->mult) >> cc->shift); }
Makes sense to me, for whatever that's worth :)
Also, tile could probably do with a similar approach for ns2cycles (not that I have observed any problems, but in the sense of being consistent and all). I'll send a patch in a separate email as not to clutter this thread too much :)
Signature · 1 line
Henrik Austad
Quoted 19 lines · Henrik Austad
Quoted 18 lines · Mike Galbraith
Greetings,
While rummaging around looking for HTH a gaggle of weird a$$ machines can manage to timewarp back and forth by exactly 208 days, I stumbled across $subject which looks like it may want to borrow Salman's fix.
clocksource: avoid unnecessary overflow in
cyclecounter_cyc2ns()As per
4cecf6d401a"sched, x86: Avoid unnecessary overflow insched_clock", cycles * mult >> shift is overflow prone. so give it the same treatment.Cc Salman Qazi <[email protected]>Cc John Stultz <[email protected]>Signed-off-by Mike Galbraith <[email protected]>--- include/linux/clocksource.h | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)
Quoted 13 lines
include/linux/clocksource.h struct timecounter { · line 83
Quoted 10 lines
include/linux/clocksource.h +4
-0@@ -77,13 +77,18 @@ struct timecounter {* * XXX - This could use some mult_lxl_ll() asm optimization. Same code * as in cyc2ns, but with unsigned result. +* +* Because it is the same as x86 __cycles_2_ns, give it the same treatment as +* commit 4cecf6d401a "sched, x86: Avoid unnecessary overflow in sched_clock" +* to avoid a potential cycles * mult overflow. Do we normally reference a particular commit in a comment? Why not just grab the same comment and add a "this is grabbed from
arch/x86/... ?
Fewer '+' signs? History doesn't go away, so seems fine to me.
-Mike
(boing boing boing... hell with it, today doesn't exist;)
Quoted 43 lines · Mike Galbraith
Quoted 19 lines · Henrik Austad
Quoted 18 lines · Mike Galbraith
Greetings,
While rummaging around looking for HTH a gaggle of weird a$$ machines can manage to timewarp back and forth by exactly 208 days, I stumbled across $subject which looks like it may want to borrow Salman's fix.
clocksource: avoid unnecessary overflow in
cyclecounter_cyc2ns()As per
4cecf6d401a"sched, x86: Avoid unnecessary overflow insched_clock", cycles * mult >> shift is overflow prone. so give it the same treatment.Cc Salman Qazi <[email protected]>Cc John Stultz <[email protected]>Signed-off-by Mike Galbraith <[email protected]>--- include/linux/clocksource.h | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)Quoted 13 lines
include/linux/clocksource.h struct timecounter { · line 83
Quoted 10 lines
include/linux/clocksource.h +4
-0@@ -77,13 +77,18 @@ struct timecounter {* * XXX - This could use some mult_lxl_ll() asm optimization. Same code * as in cyc2ns, but with unsigned result. +* +* Because it is the same as x86 __cycles_2_ns, give it the same treatment as +* commit 4cecf6d401a "sched, x86: Avoid unnecessary overflow in sched_clock" +* to avoid a potential cycles * mult overflow. Do we normally reference a particular commit in a comment? Why not just grab the same comment and add a "this is grabbed from
arch/x86/... ?Fewer '+' signs? History doesn't go away, so seems fine to me.
-Mike
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Quoted 1 line · Mike Galbraith
(boing boing boing... hell with it, today doesn't exist;)
you lost me at boing.. :)
Quoted 35 lines · Mike Galbraith
Quoted 34 lines · Henrik Austad
Quoted 32 lines · Mike Galbraith
Quoted 21 lines
Greetings,
While rummaging around looking for HTH a gaggle of weird a$$ machines can manage to timewarp back and forth by exactly 208 days, I stumbled across $subject which looks like it may want to borrow Salman's fix.
clocksource: avoid unnecessary overflow in
cyclecounter_cyc2ns()As per
4cecf6d401a"sched, x86: Avoid unnecessary overflow insched_clock", cycles * mult >> shift is overflow prone. so give it the same treatment.Cc Salman Qazi <[email protected]>Cc John Stultz <[email protected]>Signed-off-by Mike Galbraith <[email protected]>--- include/linux/clocksource.h | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)include/linux/clocksource.h struct timecounter { · line 83
Quoted 7 lines
include/linux/clocksource.h +4
-0@@ -77,13 +77,18 @@ struct timecounter {* * XXX - This could use some mult_lxl_ll() asm optimization. Same code * as in cyc2ns, but with unsigned result. +* +* Because it is the same as x86 __cycles_2_ns, give it the same treatment as +* commit 4cecf6d401a "sched, x86: Avoid unnecessary overflow in sched_clock" +* to avoid a potential cycles * mult overflow. Do we normally reference a particular commit in a comment? Why not just grab the same comment and add a "this is grabbed from
arch/x86/... ?
Quoted 1 line
Quoted 1 line
Fewer '+' signs? History doesn't go away, so seems fine to me.
I wasn't thinking about the number of +'s in the code, but rather referencing other parts of the code from the code and particular commits in the commit-msg itself. It was the code<->commitmsg interface I was pondering.
Besides, it wasn't meant as "you shouldn't do that", but more "is it ok to do that?" :)
Signature · 1 line
Henrik Austad
Quoted 4 lines · Henrik Austad
Quoted 1 line · Mike Galbraith
(boing boing boing... hell with it, today doesn't exist;)
you lost me at boing.. :)
Mail bounces due to dead addresses. Bad hair day in progress. Now my shiny new address, which is the result of _every damn address_ I tried at gmail saying "sorry, taken, try again endlessly", has escaped.
Quoted 37 lines
Quoted 35 lines · Mike Galbraith
Quoted 34 lines · Henrik Austad
Quoted 32 lines · Mike Galbraith
Quoted 21 lines
Greetings,
While rummaging around looking for HTH a gaggle of weird a$$ machines can manage to timewarp back and forth by exactly 208 days, I stumbled across $subject which looks like it may want to borrow Salman's fix.
clocksource: avoid unnecessary overflow in
cyclecounter_cyc2ns()As per
4cecf6d401a"sched, x86: Avoid unnecessary overflow insched_clock", cycles * mult >> shift is overflow prone. so give it the same treatment.Cc Salman Qazi <[email protected]>Cc John Stultz <[email protected]>Signed-off-by Mike Galbraith <[email protected]>--- include/linux/clocksource.h | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)include/linux/clocksource.h struct timecounter { · line 83
Quoted 7 lines
include/linux/clocksource.h +4
-0@@ -77,13 +77,18 @@ struct timecounter {* * XXX - This could use some mult_lxl_ll() asm optimization. Same code * as in cyc2ns, but with unsigned result. +* +* Because it is the same as x86 __cycles_2_ns, give it the same treatment as +* commit 4cecf6d401a "sched, x86: Avoid unnecessary overflow in sched_clock" +* to avoid a potential cycles * mult overflow. Do we normally reference a particular commit in a comment? Why not just grab the same comment and add a "this is grabbed from
arch/x86/... ?Quoted 1 line
Quoted 1 line
Fewer '+' signs? History doesn't go away, so seems fine to me.
Quoted 7 lines
I wasn't thinking about the number of +'s in the code, but rather referencing other parts of the code from the code and particular commits in the commit-msg itself. It was the code<->commitmsg interface I was pondering.
Besides, it wasn't meant as "you shouldn't do that", but more "is it ok to do that?" :)
It's perfectly fine until the maintainer says "NAK" :) If he does, I'll go generate '+' signs.. that he can then whack when he gets around to what he said was likely to happen to that code.
-Mike