Messages in this thread Patch in this message | | | Date | Wed, 9 Nov 2011 09:46:32 +0800 | From | Yong Zhang <> | Subject | Re: [PATCH] clocksource: Avoid selecting mult values that might overflow when adjusted |
| |
On Tue, Nov 08, 2011 at 01:39:02PM -0800, John Stultz wrote: > On Tue, 2011-11-08 at 13:02 +0800, Yong Zhang wrote: > > On Mon, Nov 07, 2011 at 07:09:00PM -0800, John Stultz wrote: > > > Yong: Can you also give this a test run to make sure you don't see any > > > problems? > > > > Still get warning (3.2-rc1 + your patch): > > > > [ 0.017009] ------------[ cut here ]------------ > > [ 0.022156] WARNING: at /build/linux/kernel/time/timekeeping.c:828 do_timer+0x402/0x4e0() > > [ 0.035917] Adjusting jiffies more then 11% (1024068096 vs 1024064000) > > Ah. We're tripping the warning here in early boot. We use jiffies as the > default clocksource initially even before it is registered and the > maxadj is then set. So since its null here, any adjustment triggers the > warning. > > That's easy enough to avoid. Can you give this updated version a try to > make sure I didn't miss anything else?
This version boot well on my side.
echo acpi_pm > /sys/devices/system/clocksource/clocksource0/current_clocksource echo hpet > /sys/devices/system/clocksource/clocksource0/current_clocksource
both work well.
Tested-by: Yong Zhang <yong.zhang0@gmail.com>
> > Thanks so much for the great testing and reports! > -john > > >From d2c1397e75ccf561bea767e31c06fb944b5391e8 Mon Sep 17 00:00:00 2001 > From: John Stultz <john.stultz@linaro.org> > Date: Mon, 31 Oct 2011 17:06:35 -0400 > Subject: [PATCH] clocksource: Avoid selecting mult values that might overflow when adjusted > > For some frequqencies, the clocks_calc_mult_shift() function will > unfortunately select mult values very close to 0xffffffff. This > has the potential to overflow when NTP adjusts the clock, adding > to the mult value. > > This patch adds a clocksource.maxadj value, which provides > an approximation of an 11% adjustment(NTP limits adjustments to > 500ppm and the tick adjustment is limited to 10%), which could > be made to the clocksource.mult value. This is then used to both > check that the current mult value won't overflow/underflow, as > well as warning us if the timekeeping_adjust() code pushes over > that 11% boundary. > > v2: Fix max_adjustment calculation, and improve WARN_ONCE > messages. > > v3: Don't warn before maxadj has actually been set > > CC: Yong Zhang <yong.zhang0@gmail.com> > CC: David Daney <ddaney.cavm@gmail.com> > CC: Thomas Gleixner <tglx@linutronix.de> > CC: Chen Jie <chenj@lemote.com> > CC: zhangfx <zhangfx@lemote.com> > Reported-by: Chen Jie <chenj@lemote.com> > Reported-by: zhangfx <zhangfx@lemote.com> > Signed-off-by: John Stultz <john.stultz@linaro.org> > --- > include/linux/clocksource.h | 3 +- > kernel/time/clocksource.c | 58 +++++++++++++++++++++++++++++++++++------- > kernel/time/timekeeping.c | 7 +++++ > 3 files changed, 57 insertions(+), 11 deletions(-) > > diff --git a/include/linux/clocksource.h b/include/linux/clocksource.h > index 139c4db..c86c940 100644 > --- a/include/linux/clocksource.h > +++ b/include/linux/clocksource.h > @@ -156,6 +156,7 @@ extern u64 timecounter_cyc2time(struct timecounter *tc, > * @mult: cycle to nanosecond multiplier > * @shift: cycle to nanosecond divisor (power of two) > * @max_idle_ns: max idle time permitted by the clocksource (nsecs) > + * @maxadj maximum adjustment value to mult (~11%) > * @flags: flags describing special properties > * @archdata: arch-specific data > * @suspend: suspend function for the clocksource, if necessary > @@ -172,7 +173,7 @@ struct clocksource { > u32 mult; > u32 shift; > u64 max_idle_ns; > - > + u32 maxadj; > #ifdef CONFIG_ARCH_CLOCKSOURCE_DATA > struct arch_clocksource_data archdata; > #endif > diff --git a/kernel/time/clocksource.c b/kernel/time/clocksource.c > index cf52fda..cfc65e1 100644 > --- a/kernel/time/clocksource.c > +++ b/kernel/time/clocksource.c > @@ -492,6 +492,22 @@ void clocksource_touch_watchdog(void) > } > > /** > + * clocksource_max_adjustment- Returns max adjustment amount > + * @cs: Pointer to clocksource > + * > + */ > +static u32 clocksource_max_adjustment(struct clocksource *cs) > +{ > + u64 ret; > + /* > + * We won't try to correct for more then 11% adjustments (110,000 ppm), > + */ > + ret = (u64)cs->mult * 11; > + do_div(ret,100); > + return (u32)ret; > +} > + > +/** > * clocksource_max_deferment - Returns max time the clocksource can be deferred > * @cs: Pointer to clocksource > * > @@ -503,25 +519,28 @@ static u64 clocksource_max_deferment(struct clocksource *cs) > /* > * 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/cs->mult which > - * is equivalent to the below. > - * max_cycles < (2^63)/cs->mult > - * max_cycles < 2^(log2((2^63)/cs->mult)) > - * max_cycles < 2^(log2(2^63) - log2(cs->mult)) > - * max_cycles < 2^(63 - log2(cs->mult)) > - * max_cycles < 1 << (63 - log2(cs->mult)) > + * maximum number of cycles is equal to ULLONG_MAX/(cs->mult+cs->maxadj) > + * which is equivalent to the below. > + * max_cycles < (2^63)/(cs->mult + cs->maxadj) > + * max_cycles < 2^(log2((2^63)/(cs->mult + cs->maxadj))) > + * max_cycles < 2^(log2(2^63) - log2(cs->mult + cs->maxadj)) > + * max_cycles < 2^(63 - log2(cs->mult + cs->maxadj)) > + * max_cycles < 1 << (63 - log2(cs->mult + cs->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. > */ > - max_cycles = 1ULL << (63 - (ilog2(cs->mult) + 1)); > + max_cycles = 1ULL << (63 - (ilog2(cs->mult + cs->maxadj) + 1)); > > /* > * The actual maximum number of cycles we can defer the clocksource is > * determined by the minimum of max_cycles and cs->mask. > + * Note: Here we subtract the maxadj to make sure we don't sleep for > + * too long if there's a large negative adjustment. > */ > max_cycles = min_t(u64, max_cycles, (u64) cs->mask); > - max_nsecs = clocksource_cyc2ns(max_cycles, cs->mult, cs->shift); > + max_nsecs = clocksource_cyc2ns(max_cycles, cs->mult - cs->maxadj, > + cs->shift); > > /* > * To ensure that the clocksource does not wrap whilst we are idle, > @@ -640,7 +659,6 @@ static void clocksource_enqueue(struct clocksource *cs) > void __clocksource_updatefreq_scale(struct clocksource *cs, u32 scale, u32 freq) > { > u64 sec; > - > /* > * Calc the maximum number of seconds which we can run before > * wrapping around. For clocksources which have a mask > 32bit > @@ -661,6 +679,20 @@ void __clocksource_updatefreq_scale(struct clocksource *cs, u32 scale, u32 freq) > > clocks_calc_mult_shift(&cs->mult, &cs->shift, freq, > NSEC_PER_SEC / scale, sec * scale); > + > + /* > + * for clocksources that have large mults, to avoid overflow. > + * Since mult may be adjusted by ntp, add an safety extra margin > + * > + */ > + cs->maxadj = clocksource_max_adjustment(cs); > + while ((cs->mult + cs->maxadj < cs->mult) > + || (cs->mult - cs->maxadj > cs->mult)) { > + cs->mult >>= 1; > + cs->shift--; > + cs->maxadj = clocksource_max_adjustment(cs); > + } > + > cs->max_idle_ns = clocksource_max_deferment(cs); > } > EXPORT_SYMBOL_GPL(__clocksource_updatefreq_scale); > @@ -701,6 +733,12 @@ EXPORT_SYMBOL_GPL(__clocksource_register_scale); > */ > int clocksource_register(struct clocksource *cs) > { > + /* calculate max adjustment for given mult/shift */ > + cs->maxadj = clocksource_max_adjustment(cs); > + WARN_ONCE(cs->mult + cs->maxadj < cs->mult, > + "Clocksource %s might overflow on 11%% adjustment\n", > + cs->name); > + > /* calculate max idle time permitted for this clocksource */ > cs->max_idle_ns = clocksource_max_deferment(cs); > > diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c > index 2b021b0e..e65ff31 100644 > --- a/kernel/time/timekeeping.c > +++ b/kernel/time/timekeeping.c > @@ -820,6 +820,13 @@ static void timekeeping_adjust(s64 offset) > } else > return; > > + WARN_ONCE(timekeeper.clock->maxadj && > + (timekeeper.mult + adj > timekeeper.clock->mult + > + timekeeper.clock->maxadj), > + "Adjusting %s more then 11%% (%ld vs %ld)\n", > + timekeeper.clock->name, (long)timekeeper.mult + adj, > + (long)timekeeper.clock->mult + > + timekeeper.clock->maxadj); > timekeeper.mult += adj; > timekeeper.xtime_interval += interval; > timekeeper.xtime_nsec -= offset; > -- > 1.7.3.2.146.gca209 > > > > -- > 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/
-- Only stand for myself
| |