[PATCH 01/12] clocksource: Simplify clocks_calc_max_nsecs logic
The previous clocks_calc_max_nsecs had some unecessarily complex bit logic to find the max interval that could cause multiplication overflows. Since this is not in the hot path, just do the divide to make it easier to read. The previous implementation also had a subtle issue that it avoided overflows into signed 64-bit values, where as the intervals are always unsigned. This resulted in overly conservative intervals, which other safety margins were then added to, reducing the intended interval length. Cc: Dave Jones Cc: Linus Torvalds Cc: Thomas Gleixner Cc: Richard Cochran Cc: Prarit Bhargava Cc: Stephen Boyd Cc: Ingo Molnar Cc: Peter Zijlstra Signed-off-by: John Stultz --- kernel/time/clocksource.c | 15 +++ 1 file changed, 3 insertions(+), 12 deletions(-) diff --git a/kernel/time/clocksource.c b/kernel/time/clocksource.c index 4892352..11323f4 100644 --- a/kernel/time/clocksource.c +++ b/kernel/time/clocksource.c @@ -476,19 +476,10 @@ u64 clocks_calc_max_nsecs(u32 mult, u32 shift, u32 maxadj, u64 mask) /* * Calculate the maximum number of cycles that we can pass to the -* cyc2ns function without overflowing a 64-bit signed result. The -* maximum number of cycles is equal to ULLONG_MAX/(mult+maxadj) -* which is equivalent to the below. -* max_cycles < (2^63)/(mult + maxadj) -* max_cycles < 2^(log2((2^63)/(mult + maxadj))) -* max_cycles < 2^(log2(2^63) - log2(mult + maxadj)) -* max_cycles < 2^(63 - log2(mult + maxadj)) -* max_cycles < 1 << (63 - log2(mult + maxadj)) -* Please note that we add 1 to the result of the log2 to account for -* any rounding errors, ensure the above inequality is satisfied and -* no overflow will occur. +* cyc2ns function without overflowing a 64-bit result. */ - max_cycles = 1ULL << (63 - (ilog2(mult + maxadj) + 1)); + max_cycles = ULLONG_MAX; + do_div(max_cycles, mult+maxadj); /* * The actual maximum number of cycles we can defer the clocksource is -- 1.9.1 -- 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: [PATCH 01/12] clocksource: Simplify clocks_calc_max_nsecs logic
* John Stultz wrote: > The previous clocks_calc_max_nsecs had some unecessarily > complex bit logic to find the max interval that could cause > multiplication overflows. Since this is not in the hot > path, just do the divide to make it easier to read. > > The previous implementation also had a subtle issue > that it avoided overflows into signed 64bit values, where So here you write the weird '64bit' form, while in the code you write: > + * cyc2ns function without overflowing a 64-bit result. This repeats in later patches as well. I'd suggest using '64-bit' consistently throughout the whole series. > as the intervals are always unsigned. This resulted in > overly conservative intervals, which other saftey margins > were then added to, reducing the intended interval length. Typo. Thanks, Ingo -- 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/
[PATCH 01/12] clocksource: Simplify clocks_calc_max_nsecs logic
The previous clocks_calc_max_nsecs had some unecessarily complex bit logic to find the max interval that could cause multiplication overflows. Since this is not in the hot path, just do the divide to make it easier to read. The previous implementation also had a subtle issue that it avoided overflows into signed 64bit values, where as the intervals are always unsigned. This resulted in overly conservative intervals, which other saftey margins were then added to, reducing the intended interval length. Cc: Dave Jones Cc: Linus Torvalds Cc: Thomas Gleixner Cc: Richard Cochran Cc: Prarit Bhargava Cc: Stephen Boyd Cc: Ingo Molnar Cc: Peter Zijlstra Signed-off-by: John Stultz --- kernel/time/clocksource.c | 15 +++ 1 file changed, 3 insertions(+), 12 deletions(-) diff --git a/kernel/time/clocksource.c b/kernel/time/clocksource.c index 4892352..11323f4 100644 --- a/kernel/time/clocksource.c +++ b/kernel/time/clocksource.c @@ -476,19 +476,10 @@ u64 clocks_calc_max_nsecs(u32 mult, u32 shift, u32 maxadj, u64 mask) /* * Calculate the maximum number of cycles that we can pass to the -* cyc2ns function without overflowing a 64-bit signed result. The -* maximum number of cycles is equal to ULLONG_MAX/(mult+maxadj) -* which is equivalent to the below. -* max_cycles < (2^63)/(mult + maxadj) -* max_cycles < 2^(log2((2^63)/(mult + maxadj))) -* max_cycles < 2^(log2(2^63) - log2(mult + maxadj)) -* max_cycles < 2^(63 - log2(mult + maxadj)) -* max_cycles < 1 << (63 - log2(mult + maxadj)) -* Please note that we add 1 to the result of the log2 to account for -* any rounding errors, ensure the above inequality is satisfied and -* no overflow will occur. +* cyc2ns function without overflowing a 64-bit result. */ - max_cycles = 1ULL << (63 - (ilog2(mult + maxadj) + 1)); + max_cycles = ULLONG_MAX; + do_div(max_cycles, mult+maxadj); /* * The actual maximum number of cycles we can defer the clocksource is -- 1.9.1 -- 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/
[PATCH 01/12] clocksource: Simplify clocks_calc_max_nsecs logic
The previous clocks_calc_max_nsecs had some unecessarily complex bit logic to find the max interval that could cause multiplication overflows. Since this is not in the hot path, just do the divide to make it easier to read. The previous implementation also had a subtle issue that it avoided overflows into signed 64bit values, where as the intervals are always unsigned. This resulted in overly conservative intervals, which other saftey margins were then added to, reducing the intended interval length. Cc: Dave Jones Cc: Linus Torvalds Cc: Thomas Gleixner Cc: Richard Cochran Cc: Prarit Bhargava Cc: Stephen Boyd Cc: Ingo Molnar Cc: Peter Zijlstra Signed-off-by: John Stultz --- kernel/time/clocksource.c | 15 +++ 1 file changed, 3 insertions(+), 12 deletions(-) diff --git a/kernel/time/clocksource.c b/kernel/time/clocksource.c index b79f39b..c14cd03 100644 --- a/kernel/time/clocksource.c +++ b/kernel/time/clocksource.c @@ -552,19 +552,10 @@ u64 clocks_calc_max_nsecs(u32 mult, u32 shift, u32 maxadj, u64 mask) /* * Calculate the maximum number of cycles that we can pass to the -* cyc2ns function without overflowing a 64-bit signed result. The -* maximum number of cycles is equal to ULLONG_MAX/(mult+maxadj) -* which is equivalent to the below. -* max_cycles < (2^63)/(mult + maxadj) -* max_cycles < 2^(log2((2^63)/(mult + maxadj))) -* max_cycles < 2^(log2(2^63) - log2(mult + maxadj)) -* max_cycles < 2^(63 - log2(mult + maxadj)) -* max_cycles < 1 << (63 - log2(mult + maxadj)) -* Please note that we add 1 to the result of the log2 to account for -* any rounding errors, ensure the above inequality is satisfied and -* no overflow will occur. +* cyc2ns function without overflowing a 64-bit result. */ - max_cycles = 1ULL << (63 - (ilog2(mult + maxadj) + 1)); + max_cycles = ULLONG_MAX; + do_div(max_cycles, mult+maxadj); /* * The actual maximum number of cycles we can defer the clocksource is -- 1.9.1 -- 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/
