Skip to contentSkip to conversation

[2.5] hangcheck-timer

PATCH In review Joel Becker 4 participants 12 messages Jan 21, 2003 Char and misc drivers

lore
Share & tools
lore

Joel Becker

author
#0 lore raw

PATCH 1/1 [2.5] hangcheck-timer

Folks,
Attached is a patch adding hangcheck-timer. It is used to detect
when the system goes out to lunch for a period of time, and then returns. This is interesting for debugging drivers as well as for clustering environments.
The module sets a timer. When the timer goes off, it then uses
get_cycles() to determine how much real time has passed.
On a normal system, the real elapsed time will be almost
identical to the expected timer duration. However, if a device decided to udelay for 60 seconds (or some other circumstance), the module takes notice. If the margin of error passes a threshold, the driver prints a warning or the machine is rebooted.
There are three parameters.

hangcheck_tick - Timer duration
hangcheck_margin - Allowed margin of between elapsed
jiffies and elapsed wall clock
hangcheck_reboot - If nonzero, reboot if margin
exceeded.

When compiled statically, s/hangcheck/hcheck/ for the kernel
command line.
Linus, please apply.

Joel

drivers/char/Kconfig +3 -0
diff -uNr linux-2.5.59/drivers/char/Kconfig linux-2.5.59-hangcheck/drivers/char/Kconfig
@@ -992,5 +992,8 @@
 	  Once bound, I/O against /dev/raw/rawN uses efficient zero-copy I/O. 
 	  See the raw(8) manpage for more details.
 
+config HANGCHECK_TIMER
+	tristate "Hangcheck timer"
+
 endmenu
drivers/char/Makefile +1 -0
diff -uNr linux-2.5.59/drivers/char/Makefile linux-2.5.59-hangcheck/drivers/char/Makefile
@@ -84,6 +84,7 @@
 obj-$(CONFIG_PCMCIA) += pcmcia/
 obj-$(CONFIG_IPMI_HANDLER) += ipmi/
 
+obj-$(CONFIG_HANGCHECK_TIMER) += hangcheck-timer.o
 
 # Files generated that shall be removed upon make clean
 clean-files := consolemap_deftbl.c defkeymap.c qtronixmap.c
drivers/char/hangcheck-timer.c +173 -0

173 lines not on this page · raw message

Signature · 8 lines
"Friends may come and go, but enemies accumulate." 
        - Thomas Jones

Joel Becker
Senior Member of Technical Staff
Oracle Corporation
E-mail: [email protected]
Phone: (650) 506-8127
#1 lore raw

drivers/char/hangcheck-timer.c line 41

Quoted 2 lines
drivers/char/hangcheck-timer.c +2 -0
+#include <linux/module.h>
+#include <linux/config.h>

I can't your see the driver reference CONFIG_* directly anywhere.

drivers/char/hangcheck-timer.c line 57

Quoted 6 lines
drivers/char/hangcheck-timer.c +6 -0
+#define VERSION_STR "0.5.0"
+
+static void version_hash_print (void)
+{
+	printk(KERN_INFO "I/O fencing modules %s\n", VERSION_STR);
+}

This message is a bit misleading, isn't it?

drivers/char/hangcheck-timer.c line 64

Quoted 3 lines
drivers/char/hangcheck-timer.c +3 -0
+static int hangcheck_tick = DEFAULT_IOFENCE_TICK;
+static int hangcheck_margin = DEFAULT_IOFENCE_MARGIN;
+static int hangcheck_reboot = 0;  /* Do not reboot */

no need to initialize static variables to zero, they'll just go into .bss.

drivers/char/hangcheck-timer.c line 72

Quoted 7 lines
drivers/char/hangcheck-timer.c +7 -0
+/* options - modular */
+MODULE_PARM(hangcheck_tick,"i");
+MODULE_PARM_DESC(hangcheck_tick, "Timer delay.");
+MODULE_PARM(hangcheck_margin,"i");
+MODULE_PARM_DESC(hangcheck_margin, "If the hangcheck timer has been delayed more than hangcheck_margin seconds, the driver will fire.");
+MODULE_PARM(hangcheck_reboot,"i");
+MODULE_PARM_DESC(hangcheck_reboot, "If nonzero, the machine will reboot when the timer margin is exceeded.");

It might be worth using Rusty's new module paramters for new code submitted to 2.5 instead of the legacy interfaces.

drivers/char/hangcheck-timer.c line 133

Quoted 8 lines
drivers/char/hangcheck-timer.c +8 -0
+#if 0
+	printk(KERN_CRIT "tsc_diff = %lu.%lu, predicted diff is %lu.%lu.\n",
+	       (unsigned long) ((tsc_diff >> 32) & 0xFFFFFFFFULL),
+	       (unsigned long) (tsc_diff & 0xFFFFFFFFULL),
+	       (unsigned long) ((hangcheck_tsc_margin >> 32) & 0xFFFFFFFFULL),
+	       (unsigned long) (hangcheck_tsc_margin & 0xFFFFFFFFULL));
+	printk(KERN_CRIT "hangcheck_margin = %lu, HZ = %lu, current_cpu_data.loops_per_jiffy = %lu.\n", hangcheck_margin, HZ, current_cpu_data.loops_per_jiffy);
+#endif

#if DEBUG maybe? or VERBOSE?

drivers/char/hangcheck-timer.c line 152

Quoted 5 lines
drivers/char/hangcheck-timer.c +5 -0
+static int __init hangcheck_init(void)
+{
+        version_hash_print();
+	printk("Hangcheck: starting hangcheck timer (tick is %d seconds, margin is %d seconds).\n",
+	       hangcheck_tick, hangcheck_margin);

Two startup messages seems like a bit too much.

Quoted 1 line

+} /* hangcheck_init() */

Bah 8)

drivers/char/hangcheck-timer.c line 165

Quoted 3 lines
drivers/char/hangcheck-timer.c +3 -0
+static void __exit hangcheck_exit(void)
+{
+	printk("Stopping hangcheck timer.\n");

Again, this is exteremly verbose.

drivers/char/hangcheck-timer.c line 169

Quoted 3 lines
drivers/char/hangcheck-timer.c +3 -0
+	lock_kernel();
+	del_timer(&hangcheck_ticktock);
+	unlock_kernel();

No need for BKL here, but you might want to use del_timer_sync.

author
#2 lore raw
Quoted 1 line · Christoph Hellwig

I can't your see the driver reference CONFIG_* directly anywhere.

I don't, but I swear something needed it somewhere. I'll look into
removing it.

Quoted 5 lines

drivers/char/hangcheck-timer.c line 64

Quoted 3 lines
drivers/char/hangcheck-timer.c +3 -0
+static int hangcheck_tick = DEFAULT_IOFENCE_TICK;
+static int hangcheck_margin = DEFAULT_IOFENCE_MARGIN;
+static int hangcheck_reboot = 0;  /* Do not reboot */

no need to initialize static variables to zero, they'll just go into .bss.

No need, but it is good to explicity mention that the default is
not to reboot.

Quoted 2 lines

It might be worth using Rusty's new module paramters for new code submitted to 2.5 instead of the legacy interfaces.

I guess I should. I'm still encountering the fact that the
module loader in 2.5.59 has issues.

Quoted 1 line

#if DEBUG maybe? or VERBOSE?

Probably just to remove. Missed it.

Joel

Signature · 11 lines
 One look at the From:
 understanding has blossomed
 .procmailrc grows
	- Alexander Viro


Joel Becker
Senior Member of Technical Staff
Oracle Corporation
E-mail: [email protected]
Phone: (650) 506-8127
author
#3 lore raw

Updated with suggested fixes.

Joel

drivers/char/Kconfig +3 -0
diff -uNr linux-2.5.59/drivers/char/Kconfig linux-2.5.59-hangcheck/drivers/char/Kconfig
@@ -992,5 +992,8 @@
 	  Once bound, I/O against /dev/raw/rawN uses efficient zero-copy I/O. 
 	  See the raw(8) manpage for more details.
 
+config HANGCHECK_TIMER
+	tristate "Hangcheck timer"
+
 endmenu
drivers/char/Makefile +1 -0
diff -uNr linux-2.5.59/drivers/char/Makefile linux-2.5.59-hangcheck/drivers/char/Makefile
@@ -84,6 +84,7 @@
 obj-$(CONFIG_PCMCIA) += pcmcia/
 obj-$(CONFIG_IPMI_HANDLER) += ipmi/
 
+obj-$(CONFIG_HANGCHECK_TIMER) += hangcheck-timer.o
 
 # Files generated that shall be removed upon make clean
 clean-files := consolemap_deftbl.c defkeymap.c qtronixmap.c
drivers/char/hangcheck-timer.c +127 -0

127 lines not on this page · raw message

Signature · 8 lines
"Hell is oneself, hell is alone, the other figures in it, merely projections."
        - T. S. Eliot

Joel Becker
Senior Member of Technical Staff
Oracle Corporation
E-mail: [email protected]
Phone: (650) 506-8127
#4 lore raw
Quoted 1 line

+#include <linux/smp_lock.h>

not needed anymore.

drivers/char/hangcheck-timer.c line 70

Quoted 2 lines
drivers/char/hangcheck-timer.c +2 -0
+MODULE_AUTHOR("Joel Becker");
+MODULE_LICENSE("GPL");

What about a MODULE_DESCRIPTION()?

drivers/char/hangcheck-timer.c line 80

Quoted 3 lines
drivers/char/hangcheck-timer.c +3 -0
+static struct timer_list hangcheck_ticktock = {
+	function:	hangcheck_fire,
+};

Use C99 initializers instead.

Quoted 1 line

+} /* hangcheck_init() */

These comments are a bit against the usual kernel coding style, espececially for that short function..

p.s. sorry for not noticing some of the stuff earlier

author
#5 lore raw
Quoted 4 lines · Christoph Hellwig
Quoted 1 line

+} /* hangcheck_init() */

These comments are a bit against the usual kernel coding style, espececially for that short function..

Oh, but I love 'em! Removed, per style.

Joel

drivers/char/Kconfig +7 -0
diff -uNr linux-2.5.59/drivers/char/Kconfig linux-2.5.59-hangcheck/drivers/char/Kconfig
@@ -992,5 +992,12 @@
 	  Once bound, I/O against /dev/raw/rawN uses efficient zero-copy I/O. 
 	  See the raw(8) manpage for more details.
 
+config HANGCHECK_TIMER
+	tristate "Hangcheck timer"
+	help
+	  The hangcheck-timer module detects when the system has gone
+	  out to lunch past a certain margin.  It can reboot the system
+	  or merely print a warning.
+
 endmenu
drivers/char/Makefile +1 -0
diff -uNr linux-2.5.59/drivers/char/Makefile linux-2.5.59-hangcheck/drivers/char/Makefile
@@ -84,6 +84,7 @@
 obj-$(CONFIG_PCMCIA) += pcmcia/
 obj-$(CONFIG_IPMI_HANDLER) += ipmi/
 
+obj-$(CONFIG_HANGCHECK_TIMER) += hangcheck-timer.o
 
 # Files generated that shall be removed upon make clean
 clean-files := consolemap_deftbl.c defkeymap.c qtronixmap.c
drivers/char/hangcheck-timer.c +127 -0

127 lines not on this page · raw message

Signature · 9 lines
"When choosing between two evils, I always like to try the one
 I've never tried before."
        - Mae West

Joel Becker
Senior Member of Technical Staff
Oracle Corporation
E-mail: [email protected]
Phone: (650) 506-8127
#6 lore raw

This version looks fine to me. Now you'll only have to wait for Linus to reappear.. :)

#7 lore raw
In reply to Joel Becker
Quoted 8 lines · Joel Becker

Updated with suggested fixes.

Joel

diff -uNr linux-2.5.59/drivers/char/Kconfig linux-2.5.59-hangcheck/drivers/char/Kconfig

drivers/char/Kconfig line 998

Quoted 7 lines
drivers/char/Kconfig +3 -0
@@ -992,5 +992,8 @@
 	  Once bound, I/O against /dev/raw/rawN uses efficient zero-copy I/O. 
 	  See the raw(8) manpage for more details.
 
+config HANGCHECK_TIMER
+	tristate "Hangcheck timer"
+
 endmenu

Help entry missing...

Sam

#8 lore raw
In reply to Joel Becker
Quoted 4 lines · Joel Becker

Folks,
Attached is a patch adding hangcheck-timer. It is used to detect
when the system goes out to lunch for a period of time, and then returns. This is interesting for debugging drivers as well as for

Quoted 8 lines

clustering environments.
The module sets a timer. When the timer goes off, it then uses
get_cycles() to determine how much real time has passed.
On a normal system, the real elapsed time will be almost
identical to the expected timer duration. However, if a device decided to udelay for 60 seconds (or some other circumstance), the module takes notice. If the margin of error passes a threshold, the driver prints a warning or the machine is rebooted.

Hi Joel,
I'm puzzled. What does this do that softdog.c doesn't ?

Dave

Signature · 2 lines
| Dave Jones.        http://www.codemonkey.org.uk
| SuSE Labs
author
#9 lore raw
Quoted 1 line · Dave Jones

I'm puzzled. What does this do that softdog.c doesn't ?

First, softdog.c requires userspace interaction. Second, softdog.c
relies on jiffies. If the system goes out to lunch via udelay() or another hardware call that freezes the CPUs for a bit, jiffies does not increment. The system could be frozen for two minutes (qla2x00 used to do this for 90 seconds) and softdog.c never notices, because jiffies hasn't counted a single second during this time.

Joel

Signature · 15 lines
"You cannot bring about prosperity by discouraging thrift. You cannot
 strengthen the weak by weakening the strong. You cannot help the wage
 earner by pulling down the wage payer. You cannot further the
 brotherhood of man by encouraging class hatred. You cannot help the
 poor by destroying the rich. You cannot build character and courage by
 taking away a man's initiative and independence. You cannot help men
 permanently by doing for them what they could and should do for themselves."
	- Abraham Lincoln 


Joel Becker
Senior Member of Technical Staff
Oracle Corporation
E-mail: [email protected]
Phone: (650) 506-8127
#10 lore raw
Quoted 8 lines · Joel Becker
Quoted 1 line

I'm puzzled. What does this do that softdog.c doesn't ?

First, softdog.c requires userspace interaction. Second, softdog.c
relies on jiffies. If the system goes out to lunch via udelay() or another hardware call that freezes the CPUs for a bit, jiffies does not increment. The system could be frozen for two minutes (qla2x00 used to do this for 90 seconds) and softdog.c never notices, because jiffies hasn't counted a single second during this time.

Ok, seems to make sense now, thanks.
Wouldn't this belong under drivers/char/watchdog too though ? It seems very 'watchdog-ish' to me.

Dave

Signature · 2 lines
| Dave Jones.        http://www.codemonkey.org.uk
| SuSE Labs
author
#11 lore raw
Quoted 3 lines · Dave Jones

Ok, seems to make sense now, thanks.
Wouldn't this belong under drivers/char/watchdog too though ? It seems very 'watchdog-ish' to me.

I thought about that a bit. Since there is no /dev/watchdog and
no userspace communication channel, it seemed bad to put it there. Heck, with no device, it doesn't necessarily fit in drivers/char in the first place.
If a few folks think drivers/char/watchdog is better, I've no
problem with it. If anyone has a better idea...

Joel

Signature · 9 lines
Life's Little Instruction Book #511

	"Call your mother."

Joel Becker
Senior Member of Technical Staff
Oracle Corporation
E-mail: [email protected]
Phone: (650) 506-8127