Re: [PATCH 3/3] softlockup: fix watchdog task wakeup frequency

[Johannes Weiner]

Hi,

Ingo Molnar <mingo@xxxxxxx> writes:

> * Johannes Weiner <hannes@xxxxxxxxxxxx> 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 <hannes@xxxxxxxxxxxx>
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 <hannes@xxxxxxxxxxxx>
---
 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: */
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/