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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87d4m3xbzq.fsf@skyscraper.fehenstaub.lan>
Date:	Fri, 27 Jun 2008 15:07:21 +0200
From:	Johannes Weiner <hannes@...urebad.de>
To:	Ingo Molnar <mingo@...e.hu>
Cc:	linux-kernel@...r.kernel.org,
	Peter Zijlstra <a.p.zijlstra@...llo.nl>
Subject: Re: [PATCH 3/3] softlockup: fix watchdog task wakeup frequency

Hi,

Ingo Molnar <mingo@...e.hu> writes:

> * Johannes Weiner <hannes@...urebad.de> 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@...urebad.de>
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@...urebad.de>
---
 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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/