Wake up the watchdog every second instead of every second. If the
current timestamp is bigger than the last touch, we are already (at
least) a second ahead.
Signed-off-by: Johannes Weiner <[email protected]>
---
kernel/softlockup.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
--- a/kernel/softlockup.c
+++ b/kernel/softlockup.c
@@ -108,7 +108,7 @@ void softlockup_tick(void)
now = get_timestamp(this_cpu);
/* Wake up the high-prio watchdog task every second: */
- if (now > (touch_timestamp + 1))
+ if (now > touch_timestamp)
wake_up_process(per_cpu(watchdog_task, this_cpu));
/* Warn about unreasonable delays: */
--
* Johannes Weiner <[email protected]> wrote:
> Wake up the watchdog every second instead of every second. [...]
now _that_ is a real improvement ;-)
> /* Wake up the high-prio watchdog task every second: */
> - if (now > (touch_timestamp + 1))
> + if (now > touch_timestamp)
> wake_up_process(per_cpu(watchdog_task, this_cpu));
you mean wake up every second instead of every two seconds?
i think the best sleep period for the watchdog is half of the threshold.
It makes no point to check sooner than that. I.e. softlockup_thresh/2
would be better?
Ingo
Ingo Molnar <[email protected]> writes:
> * Johannes Weiner <[email protected]> wrote:
>
>> Wake up the watchdog every second instead of every second. [...]
>
> now _that_ is a real improvement ;-)
>
>> /* Wake up the high-prio watchdog task every second: */
>> - if (now > (touch_timestamp + 1))
>> + if (now > touch_timestamp)
>> wake_up_process(per_cpu(watchdog_task, this_cpu));
>
> you mean wake up every second instead of every two seconds?
Every second second :D The second `second' [adjective] refers to the
first `second' [noun].
> i think the best sleep period for the watchdog is half of the threshold.
> It makes no point to check sooner than that. I.e. softlockup_thresh/2
> would be better?
Hm, it updates the timestamp, so it makes only sense if it runs at a
maximum every second (timestamp granularity) or even less. The check
for hung tasks uses the cpu timestamp as well for comparison, so that
would be okay too.
Like this?
diff --git a/kernel/softlockup.c b/kernel/softlockup.c
index c828c23..b884546 100644
--- a/kernel/softlockup.c
+++ b/kernel/softlockup.c
@@ -106,8 +106,9 @@ void softlockup_tick(void)
now = get_timestamp(this_cpu);
- /* Wake up the high-prio watchdog task every second: */
- if (now > (touch_timestamp + 1))
+ /* Wake up the high-prio watchdog task twice per
+ * threshold timespan. */
+ if (now > (touch_timestamp + softlockup_thresh / 2))
wake_up_process(per_cpu(watchdog_task, this_cpu));
/* Warn about unreasonable delays: */
* Johannes Weiner <[email protected]> wrote:
> Hm, it updates the timestamp, so it makes only sense if it runs at a
> maximum every second (timestamp granularity) or even less. The check
> for hung tasks uses the cpu timestamp as well for comparison, so that
> would be okay too.
>
> Like this?
>
> diff --git a/kernel/softlockup.c b/kernel/softlockup.c
> index c828c23..b884546 100644
> --- a/kernel/softlockup.c
> +++ b/kernel/softlockup.c
> @@ -106,8 +106,9 @@ void softlockup_tick(void)
>
> now = get_timestamp(this_cpu);
>
> - /* Wake up the high-prio watchdog task every second: */
> - if (now > (touch_timestamp + 1))
> + /* Wake up the high-prio watchdog task twice per
> + * threshold timespan. */
> + if (now > (touch_timestamp + softlockup_thresh / 2))
> wake_up_process(per_cpu(watchdog_task, this_cpu));
yeah - but please use the best possible coding style. Two-line comments
should be in the:
/*
* Here we ......................
* ........................ come:
*/
... format. And the arithmetics should be:
if (now > touch_timestamp + softlockup_thresh/2)
(the unnecessary paranthesis was a small style mistake in the original
too)
Ingo
Hi,
Ingo Molnar <[email protected]> writes:
> * Johannes Weiner <[email protected]> wrote:
>
>> Hm, it updates the timestamp, so it makes only sense if it runs at a
>> maximum every second (timestamp granularity) or even less. The check
>> for hung tasks uses the cpu timestamp as well for comparison, so that
>> would be okay too.
>>
>> Like this?
>>
>> diff --git a/kernel/softlockup.c b/kernel/softlockup.c
>> index c828c23..b884546 100644
>> --- a/kernel/softlockup.c
>> +++ b/kernel/softlockup.c
>> @@ -106,8 +106,9 @@ void softlockup_tick(void)
>>
>> now = get_timestamp(this_cpu);
>>
>> - /* Wake up the high-prio watchdog task every second: */
>> - if (now > (touch_timestamp + 1))
>> + /* Wake up the high-prio watchdog task twice per
>> + * threshold timespan. */
>> + if (now > (touch_timestamp + softlockup_thresh / 2))
>> wake_up_process(per_cpu(watchdog_task, this_cpu));
>
> yeah - but please use the best possible coding style. Two-line comments
> should be in the:
>
> /*
> * Here we ......................
> * ........................ come:
> */
>
> ... format.
Alright, that looks much better.
> And the arithmetics should be:
>
> if (now > touch_timestamp + softlockup_thresh/2)
>
> (the unnecessary paranthesis was a small style mistake in the original
> too)
I tried to fit it into the rest of the code but also prefer the one
without parens.
Thanks for the suggestions, update appended.
Hannes
---
From: Johannes Weiner <[email protected]>
Subject: [PATCH 3/3] softlockup: wake up watchdog twice per threshold timespan
Updating the timestamp more often is pointless as we print the warnings
only if we exceed the threshold. And the check for hung tasks relies on
the last timestamp, so it will keep working correctly, too.
Signed-off-by: Johannes Weiner <[email protected]>
---
kernel/softlockup.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
--- a/kernel/softlockup.c
+++ b/kernel/softlockup.c
@@ -107,8 +107,11 @@ void softlockup_tick(void)
now = get_timestamp(this_cpu);
- /* Wake up the high-prio watchdog task every second: */
- if (now > (touch_timestamp + 1))
+ /*
+ * Wake up the high-prio watchdog task twice per
+ * threshold timespan.
+ */
+ if (now > touch_timestamp + softlockup_thresh/2)
wake_up_process(per_cpu(watchdog_task, this_cpu));
/* Warn about unreasonable delays: */
Hi,
Johannes Weiner <[email protected]> writes:
> From: Johannes Weiner <[email protected]>
> Subject: [PATCH 3/3] softlockup: wake up watchdog twice per threshold timespan
>
> Updating the timestamp more often is pointless as we print the warnings
> only if we exceed the threshold. And the check for hung tasks relies on
> the last timestamp, so it will keep working correctly, too.
>
> Signed-off-by: Johannes Weiner <[email protected]>
> ---
> kernel/softlockup.c | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> --- a/kernel/softlockup.c
> +++ b/kernel/softlockup.c
> @@ -107,8 +107,11 @@ void softlockup_tick(void)
>
> now = get_timestamp(this_cpu);
>
> - /* Wake up the high-prio watchdog task every second: */
> - if (now > (touch_timestamp + 1))
> + /*
> + * Wake up the high-prio watchdog task twice per
> + * threshold timespan.
> + */
> + if (now > touch_timestamp + softlockup_thresh/2)
> wake_up_process(per_cpu(watchdog_task, this_cpu));
That defeats patch 1/3 and I think it can be dropped (#1).
Hannes
* Johannes Weiner <[email protected]> wrote:
> > + /*
> > + * Wake up the high-prio watchdog task twice per
> > + * threshold timespan.
> > + */
> > + if (now > touch_timestamp + softlockup_thresh/2)
> > wake_up_process(per_cpu(watchdog_task, this_cpu));
>
> That defeats patch 1/3 and I think it can be dropped (#1).
applied this updated patch to tip/core/softlockup. #3 didnt apply -
could you send a delta patch against tip/core/softlockup please? You can
pick it up via:
http://people.redhat.com/mingo/tip.git/README
do:
git-checkout -b core/softlockup tip/core/softlockup
to check it out.
Ingo
Hi,
Ingo Molnar <[email protected]> writes:
> * Johannes Weiner <[email protected]> wrote:
>
>> > + /*
>> > + * Wake up the high-prio watchdog task twice per
>> > + * threshold timespan.
>> > + */
>> > + if (now > touch_timestamp + softlockup_thresh/2)
>> > wake_up_process(per_cpu(watchdog_task, this_cpu));
>>
>> That defeats patch 1/3 and I think it can be dropped (#1).
>
> applied this updated patch to tip/core/softlockup. #3 didnt apply -
> could you send a delta patch against tip/core/softlockup please? You can
> pick it up via:
>
> http://people.redhat.com/mingo/tip.git/README
>
> do:
>
> git-checkout -b core/softlockup tip/core/softlockup
>
> to check it out.
Uhm, I see this patch is already in. Misunderstanding or did you fix it
up yourself?
Hannes
* Johannes Weiner <[email protected]> wrote:
> Hi,
>
> Ingo Molnar <[email protected]> writes:
>
> > * Johannes Weiner <[email protected]> wrote:
> >
> >> > + /*
> >> > + * Wake up the high-prio watchdog task twice per
> >> > + * threshold timespan.
> >> > + */
> >> > + if (now > touch_timestamp + softlockup_thresh/2)
> >> > wake_up_process(per_cpu(watchdog_task, this_cpu));
> >>
> >> That defeats patch 1/3 and I think it can be dropped (#1).
> >
> > applied this updated patch to tip/core/softlockup. #3 didnt apply -
> > could you send a delta patch against tip/core/softlockup please? You can
> > pick it up via:
> >
> > http://people.redhat.com/mingo/tip.git/README
> >
> > do:
> >
> > git-checkout -b core/softlockup tip/core/softlockup
> >
> > to check it out.
>
> Uhm, I see this patch is already in. Misunderstanding or did you fix
> it up yourself?
i applied and tested everything that would apply (modulo trivial
conflict resolution) - but not all of your patches applied so if there's
still anything missing please send a delta patch against this branch.
(or against tip/master, which too has all these changes included)
If it's "no further patch needed" that's fine - and if anything is
missing please resend that bit.
Ingo
Hi Ingo,
Ingo Molnar <[email protected]> writes:
> * Johannes Weiner <[email protected]> wrote:
>
>> Hi,
>>
>> Ingo Molnar <[email protected]> writes:
>>
>> > * Johannes Weiner <[email protected]> wrote:
>> >
>> >> > + /*
>> >> > + * Wake up the high-prio watchdog task twice per
>> >> > + * threshold timespan.
>> >> > + */
>> >> > + if (now > touch_timestamp + softlockup_thresh/2)
>> >> > wake_up_process(per_cpu(watchdog_task, this_cpu));
>> >>
>> >> That defeats patch 1/3 and I think it can be dropped (#1).
>> >
>> > applied this updated patch to tip/core/softlockup. #3 didnt apply -
>> > could you send a delta patch against tip/core/softlockup please? You can
>> > pick it up via:
>> >
>> > http://people.redhat.com/mingo/tip.git/README
>> >
>> > do:
>> >
>> > git-checkout -b core/softlockup tip/core/softlockup
>> >
>> > to check it out.
>>
>> Uhm, I see this patch is already in. Misunderstanding or did you fix
>> it up yourself?
>
> i applied and tested everything that would apply (modulo trivial
> conflict resolution) - but not all of your patches applied so if there's
> still anything missing please send a delta patch against this branch.
> (or against tip/master, which too has all these changes included)
#1 is crap, #3 is in the tree and here is #2 against tip/core/softlockup:
--
From: Johannes Weiner <[email protected]>
Subject: softlockup: sanitize timestamp comparison
The print_timestamp can never be bigger than the touch_timestamp, at
maximum it can be equal. And if it is, the second check for
touch_timestamp + 1 bigger print_timestamp is always true, too.
The check for equality is sufficient as we proceed in one-second-steps
and are at least one second away from the last print-out if we have
another timestamp.
Signed-off-by: Johannes Weiner <[email protected]>
---
kernel/softlockup.c | 5 +----
1 files changed, 1 insertions(+), 4 deletions(-)
diff --git a/kernel/softlockup.c b/kernel/softlockup.c
index d53ab70..7bd8d1a 100644
--- a/kernel/softlockup.c
+++ b/kernel/softlockup.c
@@ -116,11 +116,8 @@ void softlockup_tick(void)
print_timestamp = per_cpu(print_timestamp, this_cpu);
/* report at most once a second */
- if ((print_timestamp >= touch_timestamp &&
- print_timestamp < (touch_timestamp + 1)) ||
- did_panic) {
+ if (print_timestamp == touch_timestamp || did_panic)
return;
- }
/* do not print during early bootup: */
if (unlikely(system_state != SYSTEM_RUNNING)) {
* Johannes Weiner <[email protected]> wrote:
> > i applied and tested everything that would apply (modulo trivial
> > conflict resolution) - but not all of your patches applied so if
> > there's still anything missing please send a delta patch against
> > this branch. (or against tip/master, which too has all these changes
> > included)
>
> #1 is crap, #3 is in the tree and here is #2 against
> #tip/core/softlockup:
applied to tip/core/softlockup - thanks Johannes.
Ingo