From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752788AbbDBHr4 (ORCPT ); Thu, 2 Apr 2015 03:47:56 -0400 Received: from bombadil.infradead.org ([198.137.202.9]:45869 "EHLO bombadil.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752459AbbDBHrx (ORCPT ); Thu, 2 Apr 2015 03:47:53 -0400 Date: Thu, 2 Apr 2015 09:47:39 +0200 From: Peter Zijlstra To: John Stultz Cc: lkml , Ingo Molnar , Thomas Gleixner , Prarit Bhargava , Richard Cochran Subject: Re: [PATCH 21/21] time: Rework debugging variables so they aren't global Message-ID: <20150402074739.GP23123@twins.programming.kicks-ass.net> References: <1427945681-29972-1-git-send-email-john.stultz@linaro.org> <1427945681-29972-22-git-send-email-john.stultz@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1427945681-29972-22-git-send-email-john.stultz@linaro.org> User-Agent: Mutt/1.5.21 (2012-12-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Apr 01, 2015 at 08:34:41PM -0700, John Stultz wrote: > Ingo suggested that the timekeeping debugging variables > recently added should not be global, and should be tied > to the timekeeper's read_base. But why? its the same hardware clock for both tkr's. Surely if one goes funny the other will too. It doesn't make sense to duplicate this. > I'm a little hesitant here, since the tkr structure > has been carefully designed to fit in a cacheline. > However, these additions are all at the end of the > structure and are conditionally compiled. That. > include/linux/timekeeper_internal.h | 18 +++++++++++++++++- > kernel/time/timekeeping.c | 33 ++++++++++----------------------- > 2 files changed, 27 insertions(+), 24 deletions(-) > > diff --git a/include/linux/timekeeper_internal.h b/include/linux/timekeeper_internal.h > index fb86963..9b33027 100644 > --- a/include/linux/timekeeper_internal.h > +++ b/include/linux/timekeeper_internal.h > @@ -20,9 +20,13 @@ > * @shift: Shift value for scaled math conversion > * @xtime_nsec: Shifted (fractional) nano seconds offset for readout > * @base: ktime_t (nanoseconds) base time for readout > + * @last_warning: Warning ratelimiter (DEBUG_TIMEKEEPING) > + * @underflow_seen: Underflow warning flag (DEBUG_TIMEKEEPING) > + * @overflow_seen: Overflow warning flag (DEBUG_TIMEKEEPING) > * > * This struct has size 56 byte on 64 bit. Together with a seqcount it > - * occupies a single 64byte cache line. > + * occupies a single 64byte cache line (when DEBUG_TIMEKEEPING is not > + * enabled). > * > * The struct is separate from struct timekeeper as it is also used > * for a fast NMI safe accessors. > @@ -36,6 +40,18 @@ struct tk_read_base { > u32 shift; > u64 xtime_nsec; > ktime_t base; > +#ifdef CONFIG_DEBUG_TIMEKEEPING > + long last_warning; > + /* > + * 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. > + */ > + int underflow_seen; > + int overflow_seen; > +#endif > }; Yeah, so what is the likelyhood of a distro blanked enabling that debug? I really don't like this much. Also, you're now lacking a call to timekeeping_check_update(&tkr_raw), you update stats there but have no way to report on them.