By default, the watchdog threshold is 10, it means every 4s
every CPU will receive one hrtimer interrupt, for low power
device, it will cause 4-5mV power impact when device is deep
sleep.
So here want to optimize it as below:
4s + 4s + 4s + 4s + 4s
== >
12s + 2s + 2s + 2s + 2s
3/5 1/10 1/10 1/10 1/10
In 5 chances, once one chance is hit, then we can start the
hrtimer with a longer period sample(12s). Until the current
chance is not hit, will start the hrtimer with a shorted
period sample(2s).
With this patch, in most case the hrtimer will be 12s instead
of 4s averagely. It can save the device power indeed.
Signed-off-by: liu chuansheng <[email protected]>
---
kernel/watchdog.c | 30 ++++++++++++++++++++++++++++--
1 files changed, 28 insertions(+), 2 deletions(-)
diff --git a/kernel/watchdog.c b/kernel/watchdog.c
index dd4b80a..6457e62 100644
--- a/kernel/watchdog.c
+++ b/kernel/watchdog.c
@@ -125,7 +125,24 @@ static u64 get_sample_period(void)
* and hard thresholds) to increment before the
* hardlockup detector generates a warning
*/
- return get_softlockup_thresh() * ((u64)NSEC_PER_SEC / 5);
+ return get_softlockup_thresh() * ((u64)NSEC_PER_SEC / 10);
+}
+
+static u64 get_long_sample_period(void)
+{
+ /*
+ * convert watchdog_thresh from seconds to ns
+ * We want to give 5 chances to detect softlockup,
+ * for power saving, once one chance is succeeding,
+ * we can set long period to avoid power consumption.
+ * Currently, set the long sample period is:
+ * 20s * 3/5 = 12s, once this 12s chance is not hit,
+ * we will divide the left 8s into 4 pieces, give every
+ * chance every 2s, so it will be likely:
+ * 12s + 2s + 2s + 2s + 2s,
+ * Anyway, we just use 12s is enough in normal case.
+ */
+ return get_softlockup_thresh() * ((u64)NSEC_PER_SEC * 3 / 5);
}
/* Commands for resetting the watchdog */
@@ -267,6 +284,10 @@ static enum hrtimer_restart watchdog_timer_fn(struct hrtimer *hrtimer)
unsigned long touch_ts = __this_cpu_read(watchdog_touch_ts);
struct pt_regs *regs = get_irq_regs();
int duration;
+ bool is_touched;
+
+ is_touched = (__this_cpu_read(hrtimer_interrupts) ==
+ __this_cpu_read(soft_lockup_hrtimer_cnt));
/* kick the hardlockup detector */
watchdog_interrupt_count();
@@ -275,7 +296,12 @@ static enum hrtimer_restart watchdog_timer_fn(struct hrtimer *hrtimer)
wake_up_process(__this_cpu_read(softlockup_watchdog));
/* .. and repeat */
- hrtimer_forward_now(hrtimer, ns_to_ktime(get_sample_period()));
+ if (is_touched) {
+ hrtimer_forward_now(hrtimer,
+ ns_to_ktime(get_long_sample_period()));
+ } else {
+ hrtimer_forward_now(hrtimer, ns_to_ktime(get_sample_period()));
+ }
if (touch_ts == 0) {
if (unlikely(__this_cpu_read(softlockup_touch_sync))) {
--
1.7.0.4
On Fri, Nov 23, 2012 at 12:48:26AM +0800, Chuansheng Liu wrote:
>
> By default, the watchdog threshold is 10, it means every 4s
> every CPU will receive one hrtimer interrupt, for low power
> device, it will cause 4-5mV power impact when device is deep
> sleep.
>
> So here want to optimize it as below:
> 4s + 4s + 4s + 4s + 4s
> == >
> 12s + 2s + 2s + 2s + 2s
> 3/5 1/10 1/10 1/10 1/10
>
> In 5 chances, once one chance is hit, then we can start the
> hrtimer with a longer period sample(12s). Until the current
> chance is not hit, will start the hrtimer with a shorted
> period sample(2s).
Hmm. Have you tried this patch with cpuspeed (or cpupower) disabled?
The reason I ask is the watchdog threshold is set to 10 which means the
hardlockup watchdog will go off in 10 seconds if it isn't kicked by
hrtimers.
So 12 seconds will miss the window repeatedly.
However, the hardlockup works by calculating the max cpu frequency and
converting it to 10 seconds. Thanks to cpuspeed, most machines don't run
at max frequency. Therefore a 10 second window is usually 60 seconds or
more. So your initial testing might have missed the fact that 12 seconds
is greater than the 10 second hardlockup period.
Cheers,
Don
>
> With this patch, in most case the hrtimer will be 12s instead
> of 4s averagely. It can save the device power indeed.
>
> Signed-off-by: liu chuansheng <[email protected]>
> ---
> kernel/watchdog.c | 30 ++++++++++++++++++++++++++++--
> 1 files changed, 28 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/watchdog.c b/kernel/watchdog.c
> index dd4b80a..6457e62 100644
> --- a/kernel/watchdog.c
> +++ b/kernel/watchdog.c
> @@ -125,7 +125,24 @@ static u64 get_sample_period(void)
> * and hard thresholds) to increment before the
> * hardlockup detector generates a warning
> */
> - return get_softlockup_thresh() * ((u64)NSEC_PER_SEC / 5);
> + return get_softlockup_thresh() * ((u64)NSEC_PER_SEC / 10);
> +}
> +
> +static u64 get_long_sample_period(void)
> +{
> + /*
> + * convert watchdog_thresh from seconds to ns
> + * We want to give 5 chances to detect softlockup,
> + * for power saving, once one chance is succeeding,
> + * we can set long period to avoid power consumption.
> + * Currently, set the long sample period is:
> + * 20s * 3/5 = 12s, once this 12s chance is not hit,
> + * we will divide the left 8s into 4 pieces, give every
> + * chance every 2s, so it will be likely:
> + * 12s + 2s + 2s + 2s + 2s,
> + * Anyway, we just use 12s is enough in normal case.
> + */
> + return get_softlockup_thresh() * ((u64)NSEC_PER_SEC * 3 / 5);
> }
>
> /* Commands for resetting the watchdog */
> @@ -267,6 +284,10 @@ static enum hrtimer_restart watchdog_timer_fn(struct hrtimer *hrtimer)
> unsigned long touch_ts = __this_cpu_read(watchdog_touch_ts);
> struct pt_regs *regs = get_irq_regs();
> int duration;
> + bool is_touched;
> +
> + is_touched = (__this_cpu_read(hrtimer_interrupts) ==
> + __this_cpu_read(soft_lockup_hrtimer_cnt));
>
> /* kick the hardlockup detector */
> watchdog_interrupt_count();
> @@ -275,7 +296,12 @@ static enum hrtimer_restart watchdog_timer_fn(struct hrtimer *hrtimer)
> wake_up_process(__this_cpu_read(softlockup_watchdog));
>
> /* .. and repeat */
> - hrtimer_forward_now(hrtimer, ns_to_ktime(get_sample_period()));
> + if (is_touched) {
> + hrtimer_forward_now(hrtimer,
> + ns_to_ktime(get_long_sample_period()));
> + } else {
> + hrtimer_forward_now(hrtimer, ns_to_ktime(get_sample_period()));
> + }
>
> if (touch_ts == 0) {
> if (unlikely(__this_cpu_read(softlockup_touch_sync))) {
> --
> 1.7.0.4
>
>
>
> -----Original Message-----
> From: Don Zickus [mailto:[email protected]]
> Sent: Tuesday, November 27, 2012 4:09 AM
> To: Liu, Chuansheng
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]
> Subject: Re: [PATCH] watchdog: optimizing the hrtimer interval for power
> saving
>
> On Fri, Nov 23, 2012 at 12:48:26AM +0800, Chuansheng Liu wrote:
> >
> > By default, the watchdog threshold is 10, it means every 4s
> > every CPU will receive one hrtimer interrupt, for low power
> > device, it will cause 4-5mV power impact when device is deep
> > sleep.
> >
> > So here want to optimize it as below:
> > 4s + 4s + 4s + 4s + 4s
> > == >
> > 12s + 2s + 2s + 2s + 2s
> > 3/5 1/10 1/10 1/10 1/10
> >
> > In 5 chances, once one chance is hit, then we can start the
> > hrtimer with a longer period sample(12s). Until the current
> > chance is not hit, will start the hrtimer with a shorted
> > period sample(2s).
>
> Hmm. Have you tried this patch with cpuspeed (or cpupower) disabled?
> The reason I ask is the watchdog threshold is set to 10 which means the
> hardlockup watchdog will go off in 10 seconds if it isn't kicked by
> hrtimers.
>
> So 12 seconds will miss the window repeatedly.
Thanks your pointing out.
So I will submit V2 patch with the below checking about starting 2s or 12s hrtimer:
void watchdog_timer_fn()
{
If(watchdog_nmi_touch is false)
start 2s timer;
else
start 12s timer;
}
It can cover the case of 12s > 10s although in most cases it will not happen,
as you said, the 10s window is usually 60s.
>
> However, the hardlockup works by calculating the max cpu frequency and
> converting it to 10 seconds. Thanks to cpuspeed, most machines don't run
> at max frequency. Therefore a 10 second window is usually 60 seconds or
> more. So your initial testing might have missed the fact that 12 seconds
> is greater than the 10 second hardlockup period.
I have tried the case of set the CPU frequency to MAX, the nmi interrupt interval
is still about 20s.
>
> Cheers,
> Don
>
> >
> > With this patch, in most case the hrtimer will be 12s instead
> > of 4s averagely. It can save the device power indeed.
> >
> > Signed-off-by: liu chuansheng <[email protected]>
> > ---
> > kernel/watchdog.c | 30 ++++++++++++++++++++++++++++--
> > 1 files changed, 28 insertions(+), 2 deletions(-)
> >
> > diff --git a/kernel/watchdog.c b/kernel/watchdog.c
> > index dd4b80a..6457e62 100644
> > --- a/kernel/watchdog.c
> > +++ b/kernel/watchdog.c
> > @@ -125,7 +125,24 @@ static u64 get_sample_period(void)
> > * and hard thresholds) to increment before the
> > * hardlockup detector generates a warning
> > */
> > - return get_softlockup_thresh() * ((u64)NSEC_PER_SEC / 5);
> > + return get_softlockup_thresh() * ((u64)NSEC_PER_SEC / 10);
> > +}
> > +
> > +static u64 get_long_sample_period(void)
> > +{
> > + /*
> > + * convert watchdog_thresh from seconds to ns
> > + * We want to give 5 chances to detect softlockup,
> > + * for power saving, once one chance is succeeding,
> > + * we can set long period to avoid power consumption.
> > + * Currently, set the long sample period is:
> > + * 20s * 3/5 = 12s, once this 12s chance is not hit,
> > + * we will divide the left 8s into 4 pieces, give every
> > + * chance every 2s, so it will be likely:
> > + * 12s + 2s + 2s + 2s + 2s,
> > + * Anyway, we just use 12s is enough in normal case.
> > + */
> > + return get_softlockup_thresh() * ((u64)NSEC_PER_SEC * 3 / 5);
> > }
> >
> > /* Commands for resetting the watchdog */
> > @@ -267,6 +284,10 @@ static enum hrtimer_restart
> watchdog_timer_fn(struct hrtimer *hrtimer)
> > unsigned long touch_ts = __this_cpu_read(watchdog_touch_ts);
> > struct pt_regs *regs = get_irq_regs();
> > int duration;
> > + bool is_touched;
> > +
> > + is_touched = (__this_cpu_read(hrtimer_interrupts) ==
> > + __this_cpu_read(soft_lockup_hrtimer_cnt));
> >
> > /* kick the hardlockup detector */
> > watchdog_interrupt_count();
> > @@ -275,7 +296,12 @@ static enum hrtimer_restart
> watchdog_timer_fn(struct hrtimer *hrtimer)
> > wake_up_process(__this_cpu_read(softlockup_watchdog));
> >
> > /* .. and repeat */
> > - hrtimer_forward_now(hrtimer, ns_to_ktime(get_sample_period()));
> > + if (is_touched) {
> > + hrtimer_forward_now(hrtimer,
> > + ns_to_ktime(get_long_sample_period()));
> > + } else {
> > + hrtimer_forward_now(hrtimer, ns_to_ktime(get_sample_period()));
> > + }
> >
> > if (touch_ts == 0) {
> > if (unlikely(__this_cpu_read(softlockup_touch_sync))) {
> > --
> > 1.7.0.4
> >
> >
> >
By default, the watchdog threshold is 10, it means every 4s
every CPU will receive one hrtimer interrupt, for low power
device, it will cause 4-5mV power impact when device is deep
sleep.
So here want to optimize it as below:
4s + 4s + 4s + 4s + 4s
== >
1s + 9s + 9s ...
Or
1s + 1s..+ 9s + 9s ....
For soft lockup detection, it will have more than 5 chances to
hit, once one chance is successful, we will start 9s hrtimer
instead of 1s;
For hard lockup dection, it will have more than 2 chances to hit,
As Don said, the min window is 10s just when CPU is always running
as MAX frequency. In most case, the window is 60s, so we should
have much more than 2 chances.
With this patch, in most cases the hrtimer will be 9s instead
of 4s averagely. It can save the device power indeed.
Change log:
Since V1: In V1, Don pointed out, "12 seconds will miss the window
repeatedly." So here set the long period < min window 10s.
Signed-off-by: liu chuansheng <[email protected]>
---
kernel/watchdog.c | 30 ++++++++++++++++++++++++++++--
1 files changed, 28 insertions(+), 2 deletions(-)
diff --git a/kernel/watchdog.c b/kernel/watchdog.c
index dd4b80a..b37d682 100644
--- a/kernel/watchdog.c
+++ b/kernel/watchdog.c
@@ -125,7 +125,24 @@ static u64 get_sample_period(void)
* and hard thresholds) to increment before the
* hardlockup detector generates a warning
*/
- return get_softlockup_thresh() * ((u64)NSEC_PER_SEC / 5);
+ return get_softlockup_thresh() * ((u64)NSEC_PER_SEC / 20);
+}
+
+static u64 get_long_sample_period(void)
+{
+ /*
+ * convert watchdog_thresh from seconds to ns
+ * We want to give 5 chances to detect softlockup,
+ * for power saving, once one chance is succeeding,
+ * we can set long period to avoid power consumption.
+ * Currently, set the long sample period is:
+ * 9s = 10s - 1s, the reason is for covering the window
+ * of nmi interrupt 10s interval.
+ * So at least, for hard lockup, it has >=2 chances,
+ * for soft lockup, it has >= 5 chances.
+ *
+ */
+ return (watchdog_thresh * (u64)NSEC_PER_SEC) - get_sample_period();
}
/* Commands for resetting the watchdog */
@@ -267,6 +284,10 @@ static enum hrtimer_restart watchdog_timer_fn(struct hrtimer *hrtimer)
unsigned long touch_ts = __this_cpu_read(watchdog_touch_ts);
struct pt_regs *regs = get_irq_regs();
int duration;
+ bool is_touched;
+
+ is_touched = (__this_cpu_read(hrtimer_interrupts) ==
+ __this_cpu_read(soft_lockup_hrtimer_cnt));
/* kick the hardlockup detector */
watchdog_interrupt_count();
@@ -275,7 +296,12 @@ static enum hrtimer_restart watchdog_timer_fn(struct hrtimer *hrtimer)
wake_up_process(__this_cpu_read(softlockup_watchdog));
/* .. and repeat */
- hrtimer_forward_now(hrtimer, ns_to_ktime(get_sample_period()));
+ if (is_touched) {
+ hrtimer_forward_now(hrtimer,
+ ns_to_ktime(get_long_sample_period()));
+ } else {
+ hrtimer_forward_now(hrtimer, ns_to_ktime(get_sample_period()));
+ }
if (touch_ts == 0) {
if (unlikely(__this_cpu_read(softlockup_touch_sync))) {
--
1.7.0.4
On Wed, Nov 28, 2012 at 07:24:52PM +0800, Chuansheng Liu wrote:
>
> By default, the watchdog threshold is 10, it means every 4s
> every CPU will receive one hrtimer interrupt, for low power
> device, it will cause 4-5mV power impact when device is deep
> sleep.
>
> So here want to optimize it as below:
> 4s + 4s + 4s + 4s + 4s
> == >
> 1s + 9s + 9s ...
> Or
> 1s + 1s..+ 9s + 9s ....
>
> For soft lockup detection, it will have more than 5 chances to
> hit, once one chance is successful, we will start 9s hrtimer
> instead of 1s;
>
> For hard lockup dection, it will have more than 2 chances to hit,
> As Don said, the min window is 10s just when CPU is always running
> as MAX frequency. In most case, the window is 60s, so we should
> have much more than 2 chances.
>
> With this patch, in most cases the hrtimer will be 9s instead
> of 4s averagely. It can save the device power indeed.
>
> Change log:
> Since V1: In V1, Don pointed out, "12 seconds will miss the window
> repeatedly." So here set the long period < min window 10s.
Hmm. My only concern is if you are solving this the right way. The
Chrome folks wanted this threshold to be smaller like 2 seconds, which
would defeat the whole point of this patch.
It seems like a better approach would be to adjust the timer somehow when
you change c-states. The whole point of the hard and softlockup is to
detect if scheduled code is either deadlock or hogging the cpu for too long.
If the cpu is in a deep sleep, then nothing is running, right? Which
means nothing can deadlock or hog the cpu. In those cases you can
probably temporarily disable the lockup detector until the cpu wakes up
from that c-state and starts scheduling code again.
In that case you can really maximize your power savings (and probably get
powerTop to stop telling everyone to disable the nmi_watchdog :-) ).
Ideally in a deep sleep you don't want any soft interrupts running, no?
Just a thought.
Cheers,
Don
> It seems like a better approach would be to adjust the timer somehow when
> you change c-states. The whole point of the hard and softlockup is to
> detect if scheduled code is either deadlock or hogging the cpu for too long.
>
> If the cpu is in a deep sleep, then nothing is running, right? Which
> means nothing can deadlock or hog the cpu. In those cases you can
> probably temporarily disable the lockup detector until the cpu wakes up
> from that c-state and starts scheduling code again.
>
You are right, I ever tried the thought, when CPU is idle, we can pause the hrtimer,
After wakeup, we resume the hrtimer again. But I found sometimes the in idle and out of
idle is too frequent.
Anyway, you advice seems the right way, I will try to dig something more deeply.
Thanks.
> In that case you can really maximize your power savings (and probably get
> powerTop to stop telling everyone to disable the nmi_watchdog :-) ).
>
> Ideally in a deep sleep you don't want any soft interrupts running, no?
>
> Just a thought.
>
> Cheers,
> Don