Re: [PATCH 08/12] time: Add warnings when overflows or underflows are observed
* John Stultz wrote: > Thanks Ingo for the very close review, and apologies for my poor > keyboardmanship (I hope I didn't burn much of your good will here). No problem. I usually fix typos up when the patch is otherwise good, except for Git pulls, where I cannot, so I'm pushing back ... > I'll work to get these trivial changes integrated along with the > more substantial feedback as well. It's all nice changes otherwise. I'm fairly sure the new sanity checks are going to show us interesting things in the future. 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/
Re: [PATCH 08/12] time: Add warnings when overflows or underflows are observed
On Sat, Mar 7, 2015 at 1:40 AM, Ingo Molnar wrote: ... > > Typo. ... > > Typo. > ... > > Typo. > ... > > Typo... > ... > > Spurious space. I know they are cheap, but still. And a big D with a circle around it. Back to grade-school with me. :) Thanks Ingo for the very close review, and apologies for my poor keyboardmanship (I hope I didn't burn much of your good will here). I'll work to get these trivial changes integrated along with the more substantial feedback as well. thanks -john -- 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 08/12] time: Add warnings when overflows or underflows are observed
On Sat, Mar 7, 2015 at 1:40 AM, Ingo Molnar mi...@kernel.org wrote: ... Typo. ... Typo. ... Typo. ... Typo... ... Spurious space. I know they are cheap, but still. And a big D with a circle around it. Back to grade-school with me. :) Thanks Ingo for the very close review, and apologies for my poor keyboardmanship (I hope I didn't burn much of your good will here). I'll work to get these trivial changes integrated along with the more substantial feedback as well. thanks -john -- 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 08/12] time: Add warnings when overflows or underflows are observed
* John Stultz john.stu...@linaro.org wrote: Thanks Ingo for the very close review, and apologies for my poor keyboardmanship (I hope I didn't burn much of your good will here). No problem. I usually fix typos up when the patch is otherwise good, except for Git pulls, where I cannot, so I'm pushing back ... I'll work to get these trivial changes integrated along with the more substantial feedback as well. It's all nice changes otherwise. I'm fairly sure the new sanity checks are going to show us interesting things in the future. 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/
Re: [PATCH 08/12] time: Add warnings when overflows or underflows are observed
* 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.
Typo.
> 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 consequnece.
Typo.
> The big complication is that we're only under a read
> seqlock, so the data could shift under us during
> our calcualtion to see if there was a problem. This
Typo.
> patch avoids this issue by nesting another seqlock
> which allows us to snapshot the just required values
> atomically. So we shouldn't see false positives.
>
> I also added some basic ratelimiting here, since
> on one build machine w/ skewed TSCs it was fairly
> noisy at bootup.
> +#define WARNINGFREQ (HZ*300) /* 5 minute rate-limiting */
Nit: so in general wereallytrytokeepwordsapart, so I'd suggest a
name of WARNING_FREQ or so?
> cycle_t max_cycles = tk->tkr.clock->max_cycles;
> const char *name = tk->tkr.clock->name;
> + static long last_warning; /* we always hold write on timekeeper lock */
So I'm not sure I ever heard the phrase 'to hold write', this doesn't
parse for me.
Also, static global variables should really, really not be immersed
amongst on-stack variables, they are so easy to overlook. Just put
them in front of the function.
>
> if (offset > max_cycles)
> printk_deferred("ERROR: cycle offset (%lld) is larger then"
> @@ -133,28 +145,60 @@ static void timekeeping_check_update(struct timekeeper
> *tk, cycle_t offset)
> printk_deferred("WARNING: cycle offset (%lld) is past"
> " the %s 50%% safety margin (%lld)\n",
> offset, name, max_cycles>>1);
> +
> + if (timekeeping_underflow_seen) {
> + if (jiffies - last_warning > WARNINGFREQ) {
> + printk_deferred("WARNING: Clocksource underflow
> observed\n");
> + last_warning = jiffies;
> + }
> + timekeeping_underflow_seen = 0;
> + }
> + if (timekeeping_overflow_seen) {
> + if (jiffies - last_warning > WARNINGFREQ) {
> + printk_deferred("WARNING: Clocksource overflow
> observed\n");
I think the warning should be more informative. If a distro turns this
on and a user sees this value, what will he think? Is the kernel still
OK? What can he do about it?
> + 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 doign the calculation. This can cause
Typo...
> + * false positives, since we'd note a problem but throw the
> + * results away. So nest another seqlock here to atomically
Spurious space. I know they are cheap, but still.
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/
Re: [PATCH 08/12] time: Add warnings when overflows or underflows are observed
* John Stultz john.stu...@linaro.org wrote:
It was suggested that the underflow/overflow protection
should probably throw some sort of warning out, rather
then just silently fixing the issue.
Typo.
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 consequnece.
Typo.
The big complication is that we're only under a read
seqlock, so the data could shift under us during
our calcualtion to see if there was a problem. This
Typo.
patch avoids this issue by nesting another seqlock
which allows us to snapshot the just required values
atomically. So we shouldn't see false positives.
I also added some basic ratelimiting here, since
on one build machine w/ skewed TSCs it was fairly
noisy at bootup.
+#define WARNINGFREQ (HZ*300) /* 5 minute rate-limiting */
Nit: so in general wereallytrytokeepwordsapart, so I'd suggest a
name of WARNING_FREQ or so?
cycle_t max_cycles = tk-tkr.clock-max_cycles;
const char *name = tk-tkr.clock-name;
+ static long last_warning; /* we always hold write on timekeeper lock */
So I'm not sure I ever heard the phrase 'to hold write', this doesn't
parse for me.
Also, static global variables should really, really not be immersed
amongst on-stack variables, they are so easy to overlook. Just put
them in front of the function.
if (offset max_cycles)
printk_deferred(ERROR: cycle offset (%lld) is larger then
@@ -133,28 +145,60 @@ static void timekeeping_check_update(struct timekeeper
*tk, cycle_t offset)
printk_deferred(WARNING: cycle offset (%lld) is past
the %s 50%% safety margin (%lld)\n,
offset, name, max_cycles1);
+
+ if (timekeeping_underflow_seen) {
+ if (jiffies - last_warning WARNINGFREQ) {
+ printk_deferred(WARNING: Clocksource underflow
observed\n);
+ last_warning = jiffies;
+ }
+ timekeeping_underflow_seen = 0;
+ }
+ if (timekeeping_overflow_seen) {
+ if (jiffies - last_warning WARNINGFREQ) {
+ printk_deferred(WARNING: Clocksource overflow
observed\n);
I think the warning should be more informative. If a distro turns this
on and a user sees this value, what will he think? Is the kernel still
OK? What can he do about it?
+ 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 doign the calculation. This can cause
Typo...
+ * false positives, since we'd note a problem but throw the
+ * results away. So nest another seqlock here to atomically
Spurious space. I know they are cheap, but still.
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 08/12] time: Add warnings when overflows or underflows are observed
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 consequnece.
The big complication is that we're only under a read
seqlock, so the data could shift under us during
our calcualtion to see if there was a problem. This
patch avoids this issue by nesting another seqlock
which allows us to snapshot the just required values
atomically. So we shouldn't see false positives.
I also added some basic ratelimiting here, since
on one build machine w/ skewed TSCs it was fairly
noisy at bootup.
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/timekeeping.c | 58 +--
1 file changed, 51 insertions(+), 7 deletions(-)
diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index 4e8ccde..5f62308 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -119,11 +119,23 @@ static inline void tk_update_sleep_time(struct timekeeper
*tk, ktime_t delta)
}
#ifdef CONFIG_DEBUG_TIMEKEEPING
+#define WARNINGFREQ (HZ*300) /* 5 minute rate-limiting */
+/*
+ * 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;
+
static void timekeeping_check_update(struct timekeeper *tk, cycle_t offset)
{
cycle_t max_cycles = tk->tkr.clock->max_cycles;
const char *name = tk->tkr.clock->name;
+ static long last_warning; /* we always hold write on timekeeper lock */
if (offset > max_cycles)
printk_deferred("ERROR: cycle offset (%lld) is larger then"
@@ -133,28 +145,60 @@ static void timekeeping_check_update(struct timekeeper
*tk, cycle_t offset)
printk_deferred("WARNING: cycle offset (%lld) is past"
" the %s 50%% safety margin (%lld)\n",
offset, name, max_cycles>>1);
+
+ if (timekeeping_underflow_seen) {
+ if (jiffies - last_warning > WARNINGFREQ) {
+ printk_deferred("WARNING: Clocksource underflow
observed\n");
+ last_warning = jiffies;
+ }
+ timekeeping_underflow_seen = 0;
+ }
+ if (timekeeping_overflow_seen) {
+ if (jiffies - last_warning > WARNINGFREQ) {
+ printk_deferred("WARNING: Clocksource overflow
observed\n");
+ 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 doign 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(_core.seq);
+ now = tkr->read(tkr->clock);
+ last = tkr->cycle_last;
+ mask = tkr->mask;
+ max = tkr->clock->max_cycles;
+ } while (read_seqcount_retry(_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;
+ }
return delta;
}
--
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
[PATCH 08/12] time: Add warnings when overflows or underflows are observed
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 consequnece.
The big complication is that we're only under a read
seqlock, so the data could shift under us during
our calcualtion to see if there was a problem. This
patch avoids this issue by nesting another seqlock
which allows us to snapshot the just required values
atomically. So we shouldn't see false positives.
I also added some basic ratelimiting here, since
on one build machine w/ skewed TSCs it was fairly
noisy at bootup.
Cc: Dave Jones da...@codemonkey.org.uk
Cc: Linus Torvalds torva...@linux-foundation.org
Cc: Thomas Gleixner t...@linutronix.de
Cc: Richard Cochran richardcoch...@gmail.com
Cc: Prarit Bhargava pra...@redhat.com
Cc: Stephen Boyd sb...@codeaurora.org
Cc: Ingo Molnar mi...@kernel.org
Cc: Peter Zijlstra pet...@infradead.org
Signed-off-by: John Stultz john.stu...@linaro.org
---
kernel/time/timekeeping.c | 58 +--
1 file changed, 51 insertions(+), 7 deletions(-)
diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index 4e8ccde..5f62308 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -119,11 +119,23 @@ static inline void tk_update_sleep_time(struct timekeeper
*tk, ktime_t delta)
}
#ifdef CONFIG_DEBUG_TIMEKEEPING
+#define WARNINGFREQ (HZ*300) /* 5 minute rate-limiting */
+/*
+ * 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;
+
static void timekeeping_check_update(struct timekeeper *tk, cycle_t offset)
{
cycle_t max_cycles = tk-tkr.clock-max_cycles;
const char *name = tk-tkr.clock-name;
+ static long last_warning; /* we always hold write on timekeeper lock */
if (offset max_cycles)
printk_deferred(ERROR: cycle offset (%lld) is larger then
@@ -133,28 +145,60 @@ static void timekeeping_check_update(struct timekeeper
*tk, cycle_t offset)
printk_deferred(WARNING: cycle offset (%lld) is past
the %s 50%% safety margin (%lld)\n,
offset, name, max_cycles1);
+
+ if (timekeeping_underflow_seen) {
+ if (jiffies - last_warning WARNINGFREQ) {
+ printk_deferred(WARNING: Clocksource underflow
observed\n);
+ last_warning = jiffies;
+ }
+ timekeeping_underflow_seen = 0;
+ }
+ if (timekeeping_overflow_seen) {
+ if (jiffies - last_warning WARNINGFREQ) {
+ printk_deferred(WARNING: Clocksource overflow
observed\n);
+ 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 doign 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;
+ }
return
Re: [PATCH 08/12] time: Add warnings when overflows or underflows are observed
On Thu, Jan 22, 2015 at 04:09:23PM -0800, John Stultz wrote:
> #ifdef CONFIG_DEBUG_TIMEKEEPING
> +#define WARNINGFREQ (HZ*300) /* 5 minute rate-limiting */
> +/*
> + * 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;
> +
> static void timekeeping_check_update(struct timekeeper *tk, cycle_t offset)
> {
>
> cycle_t max_cycles = tk->tkr.clock->max_cycles;
> const char *name = tk->tkr.clock->name;
> + static long last_warning; /* we always hold write on timekeeper lock */
>
> if (offset > max_cycles)
> printk_deferred("ERROR: cycle offset (%lld) is larger then"
> @@ -133,28 +145,60 @@ static void timekeeping_check_update(struct timekeeper
> *tk, cycle_t offset)
> printk_deferred("WARNING: cycle offset (%lld) is past"
> " the %s 50%% safety margin (%lld)\n",
> offset, name, max_cycles>>1);
> +
> + if (timekeeping_underflow_seen) {
> + if (jiffies - last_warning > WARNINGFREQ) {
> + printk_deferred("WARNING: Clocksource underflow
> observed\n");
> + last_warning = jiffies;
> + }
> + timekeeping_underflow_seen = 0;
> + }
> + if (timekeeping_overflow_seen) {
> + if (jiffies - last_warning > WARNINGFREQ) {
> + printk_deferred("WARNING: Clocksource overflow
> observed\n");
> + last_warning = jiffies;
> + }
> + timekeeping_overflow_seen = 0;
> + }
> +
> }
Ah, ignore my last comment. Excellent!
--
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 08/12] time: Add warnings when overflows or underflows are observed
On Thu, Jan 22, 2015 at 04:09:23PM -0800, John Stultz wrote:
#ifdef CONFIG_DEBUG_TIMEKEEPING
+#define WARNINGFREQ (HZ*300) /* 5 minute rate-limiting */
+/*
+ * 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;
+
static void timekeeping_check_update(struct timekeeper *tk, cycle_t offset)
{
cycle_t max_cycles = tk-tkr.clock-max_cycles;
const char *name = tk-tkr.clock-name;
+ static long last_warning; /* we always hold write on timekeeper lock */
if (offset max_cycles)
printk_deferred(ERROR: cycle offset (%lld) is larger then
@@ -133,28 +145,60 @@ static void timekeeping_check_update(struct timekeeper
*tk, cycle_t offset)
printk_deferred(WARNING: cycle offset (%lld) is past
the %s 50%% safety margin (%lld)\n,
offset, name, max_cycles1);
+
+ if (timekeeping_underflow_seen) {
+ if (jiffies - last_warning WARNINGFREQ) {
+ printk_deferred(WARNING: Clocksource underflow
observed\n);
+ last_warning = jiffies;
+ }
+ timekeeping_underflow_seen = 0;
+ }
+ if (timekeeping_overflow_seen) {
+ if (jiffies - last_warning WARNINGFREQ) {
+ printk_deferred(WARNING: Clocksource overflow
observed\n);
+ last_warning = jiffies;
+ }
+ timekeeping_overflow_seen = 0;
+ }
+
}
Ah, ignore my last comment. Excellent!
--
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 08/12] time: Add warnings when overflows or underflows are observed
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 consequnece.
The big complication is that we're only under a read
seqlock, so the data could shift under us during
our calcualtion to see if there was a problem. This
patch avoids this issue by nesting another seqlock
which allows us to snapshot the just required values
atomically. So we shouldn't see false positives.
I also added some basic ratelimiting here, since
on one build machine w/ skewed TSCs it was fairly
noisy at bootup.
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/timekeeping.c | 58 +--
1 file changed, 51 insertions(+), 7 deletions(-)
diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index 568186c..d216b0e 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -119,11 +119,23 @@ static inline void tk_update_sleep_time(struct timekeeper
*tk, ktime_t delta)
}
#ifdef CONFIG_DEBUG_TIMEKEEPING
+#define WARNINGFREQ (HZ*300) /* 5 minute rate-limiting */
+/*
+ * 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;
+
static void timekeeping_check_update(struct timekeeper *tk, cycle_t offset)
{
cycle_t max_cycles = tk->tkr.clock->max_cycles;
const char *name = tk->tkr.clock->name;
+ static long last_warning; /* we always hold write on timekeeper lock */
if (offset > max_cycles)
printk_deferred("ERROR: cycle offset (%lld) is larger then"
@@ -133,28 +145,60 @@ static void timekeeping_check_update(struct timekeeper
*tk, cycle_t offset)
printk_deferred("WARNING: cycle offset (%lld) is past"
" the %s 50%% safety margin (%lld)\n",
offset, name, max_cycles>>1);
+
+ if (timekeeping_underflow_seen) {
+ if (jiffies - last_warning > WARNINGFREQ) {
+ printk_deferred("WARNING: Clocksource underflow
observed\n");
+ last_warning = jiffies;
+ }
+ timekeeping_underflow_seen = 0;
+ }
+ if (timekeeping_overflow_seen) {
+ if (jiffies - last_warning > WARNINGFREQ) {
+ printk_deferred("WARNING: Clocksource overflow
observed\n");
+ 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 doign 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(_core.seq);
+ now = tkr->read(tkr->clock);
+ last = tkr->cycle_last;
+ mask = tkr->mask;
+ max = tkr->clock->max_cycles;
+ } while (read_seqcount_retry(_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;
+ }
return delta;
}
--
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
[PATCH 08/12] time: Add warnings when overflows or underflows are observed
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 consequnece.
The big complication is that we're only under a read
seqlock, so the data could shift under us during
our calcualtion to see if there was a problem. This
patch avoids this issue by nesting another seqlock
which allows us to snapshot the just required values
atomically. So we shouldn't see false positives.
I also added some basic ratelimiting here, since
on one build machine w/ skewed TSCs it was fairly
noisy at bootup.
Cc: Dave Jones da...@codemonkey.org.uk
Cc: Linus Torvalds torva...@linux-foundation.org
Cc: Thomas Gleixner t...@linutronix.de
Cc: Richard Cochran richardcoch...@gmail.com
Cc: Prarit Bhargava pra...@redhat.com
Cc: Stephen Boyd sb...@codeaurora.org
Cc: Ingo Molnar mi...@kernel.org
Cc: Peter Zijlstra pet...@infradead.org
Signed-off-by: John Stultz john.stu...@linaro.org
---
kernel/time/timekeeping.c | 58 +--
1 file changed, 51 insertions(+), 7 deletions(-)
diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index 568186c..d216b0e 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -119,11 +119,23 @@ static inline void tk_update_sleep_time(struct timekeeper
*tk, ktime_t delta)
}
#ifdef CONFIG_DEBUG_TIMEKEEPING
+#define WARNINGFREQ (HZ*300) /* 5 minute rate-limiting */
+/*
+ * 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;
+
static void timekeeping_check_update(struct timekeeper *tk, cycle_t offset)
{
cycle_t max_cycles = tk-tkr.clock-max_cycles;
const char *name = tk-tkr.clock-name;
+ static long last_warning; /* we always hold write on timekeeper lock */
if (offset max_cycles)
printk_deferred(ERROR: cycle offset (%lld) is larger then
@@ -133,28 +145,60 @@ static void timekeeping_check_update(struct timekeeper
*tk, cycle_t offset)
printk_deferred(WARNING: cycle offset (%lld) is past
the %s 50%% safety margin (%lld)\n,
offset, name, max_cycles1);
+
+ if (timekeeping_underflow_seen) {
+ if (jiffies - last_warning WARNINGFREQ) {
+ printk_deferred(WARNING: Clocksource underflow
observed\n);
+ last_warning = jiffies;
+ }
+ timekeeping_underflow_seen = 0;
+ }
+ if (timekeeping_overflow_seen) {
+ if (jiffies - last_warning WARNINGFREQ) {
+ printk_deferred(WARNING: Clocksource overflow
observed\n);
+ 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 doign 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;
+ }
return
