From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753325AbbCLHhq (ORCPT ); Thu, 12 Mar 2015 03:37:46 -0400 Received: from mail-wi0-f182.google.com ([209.85.212.182]:44864 "EHLO mail-wi0-f182.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752658AbbCLHhp (ORCPT ); Thu, 12 Mar 2015 03:37:45 -0400 Date: Thu, 12 Mar 2015 08:37:39 +0100 From: Ingo Molnar To: John Stultz Cc: lkml , Dave Jones , Linus Torvalds , Thomas Gleixner , Richard Cochran , Prarit Bhargava , Stephen Boyd , Peter Zijlstra Subject: Re: [PATCH 07/12] time: Add warnings when overflows or underflows are observed Message-ID: <20150312073739.GA15123@gmail.com> References: <1426133800-29329-1-git-send-email-john.stultz@linaro.org> <1426133800-29329-8-git-send-email-john.stultz@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1426133800-29329-8-git-send-email-john.stultz@linaro.org> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org * John Stultz wrote: > It was suggested that the underflow/overflow protection > should probably throw some sort of warning out, rather > then just silently fixing the issue. > > So this patch adds some warnings here. The flag variables > used are not protected by locks, but since we can't print > from the reading functions, just being able to say we > saw an issue in the update interval is useful enough, > and can be slightly racy without real consequence. > + * These simple flag variables are managed > + * without locks, which is racy, but ok since > + * we don't really care about being super > + * precise about how many events were seen, > + * just that a problem was observed. > + */ > +static int timekeeping_underflow_seen; > +static int timekeeping_overflow_seen; > + > +/* last_warning is only modified under the timekeeping lock */ > +static long timekeeping_last_warning; > + > static void timekeeping_check_update(struct timekeeper *tk, cycle_t offset) > { > > @@ -134,28 +148,62 @@ static void timekeeping_check_update(struct timekeeper *tk, cycle_t offset) > offset, name, max_cycles>>1); > } > } > + > + if (timekeeping_underflow_seen) { > + if (jiffies - timekeeping_last_warning > WARNING_FREQ) { > + printk_deferred("WARNING: Clocksource %s underflow observed. You should report\n", name); > + printk_deferred(" this, consider using a different clocksource.\n"); > + timekeeping_last_warning = jiffies; > + } > + timekeeping_underflow_seen = 0; > + } > + > + if (timekeeping_overflow_seen) { > + if (jiffies - timekeeping_last_warning > WARNING_FREQ) { > + printk_deferred("WARNING: Clocksource %s overflow observed. You should report\n", name); > + printk_deferred(" this, consider using a different clocksource.\n"); > + timekeeping_last_warning = jiffies; > + } > + timekeeping_overflow_seen = 0; > + } > } > > static inline cycle_t timekeeping_get_delta(struct tk_read_base *tkr) > { > - cycle_t cycle_now, delta; > + cycle_t now, last, mask, max, delta; > + unsigned int seq; > > - /* read clocksource */ > - cycle_now = tkr->read(tkr->clock); > + /* > + * Since we're called holding a seqlock, the data may shift > + * under us while we're doing the calculation. This can cause > + * false positives, since we'd note a problem but throw the > + * results away. So nest another seqlock here to atomically > + * grab the points we are checking with. > + */ > + do { > + seq = read_seqcount_begin(&tk_core.seq); > + now = tkr->read(tkr->clock); > + last = tkr->cycle_last; > + mask = tkr->mask; > + max = tkr->clock->max_cycles; > + } while (read_seqcount_retry(&tk_core.seq, seq)); > > - /* calculate the delta since the last update_wall_time */ > - delta = clocksource_delta(cycle_now, tkr->cycle_last, tkr->mask); > + delta = clocksource_delta(now, last, mask); > > /* > * Try to catch underflows by checking if we are seeing small > * mask-relative negative values. > */ > - if (unlikely((~delta & tkr->mask) < (tkr->mask >> 3))) > + if (unlikely((~delta & mask) < (mask >> 3))) { > + timekeeping_underflow_seen = 1; > delta = 0; > + } > > /* Cap delta value to the max_cycles values to avoid mult overflows */ > - if (unlikely(delta > tkr->clock->max_cycles)) > + if (unlikely(delta > max)) { > + timekeeping_overflow_seen = 1; > delta = tkr->clock->max_cycles; > + } Please add the flags as new fields in the 'struct tk_read_base' data structure in include/linux/timekeeper_internal.h - we don't want to go back to the old pattern of adding globals to the timekeeping code, even if they are just for debugging! also, timekeeping_check_update() should pass in 'struct tk_read_base', not 'struct timekeeper' - it's really only using the tkr bits and doing this change would make it similar to timekeeping_get_delta(). It would also shorten: cycle_t max_cycles = tk->tkr.clock->max_cycles; const char *name = tk->tkr.clock->name; to the more natural looking: cycle_t max_cycles = tkr->clock->max_cycles; const char *name = tkr->clock->name; hm? Thanks, Ingo