[PATCH][2.5] hangcheck-timer

[PATCH][2.5] hangcheck-timer

[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
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

diff -uNr linux-2.5.59/drivers/char/Kconfig linux-2.5.59-hangcheck/drivers/char/Kconfig
--- linux-2.5.59/drivers/char/Kconfig	Thu Jan 16 18:21:36 2003
+++ linux-2.5.59-hangcheck/drivers/char/Kconfig	Mon Jan 20 13:35:27 2003
@@ -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
 
diff -uNr linux-2.5.59/drivers/char/Makefile linux-2.5.59-hangcheck/drivers/char/Makefile
--- linux-2.5.59/drivers/char/Makefile	Thu Jan 16 18:22:44 2003
+++ linux-2.5.59-hangcheck/drivers/char/Makefile	Mon Jan 20 13:35:02 2003
@@ -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
diff -uNr linux-2.5.59/drivers/char/hangcheck-timer.c linux-2.5.59-hangcheck/drivers/char/hangcheck-timer.c
--- linux-2.5.59/drivers/char/hangcheck-timer.c	Wed Dec 31 16:00:00 1969
+++ linux-2.5.59-hangcheck/drivers/char/hangcheck-timer.c	Mon Jan 20 13:33:42 2003
@@ -0,0 +1,173 @@
+/*
+ * hangcheck-timer.c
+ *
+ * Driver for a little io fencing timer.
+ *
+ * Copyright (C) 2002 Oracle Corporation.  All rights reserved.
+ *
+ * Author: Joel Becker <joel.becker@oracle.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public
+ * License version 2 as published by the Free Software Foundation.
+ * 
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details.
+ * 
+ * You should have recieved a copy of the GNU General Public
+ * License along with this program; if not, write to the
+ * Free Software Foundation, Inc., 59 Temple Place - Suite 330,
+ * Boston, MA 021110-1307, USA.
+ */
+
+/*
+ * The hangcheck-timer driver uses the TSC to catch delays that
+ * jiffies does not notice.  A timer is set.  When the timer fires, it
+ * checks whether it was delayed and if that delay exceeds a given
+ * margin of error.  The hangcheck_tick module paramter takes the timer
+ * duration in seconds.  The hangcheck_margin parameter defines the
+ * margin of error, in seconds.  The defaults are 60 seconds for the
+ * timer and 180 seconds for the margin of error.  IOW, a timer is set
+ * for 60 seconds.  When the timer fires, the callback checks the
+ * actual duration that the timer waited.  If the duration exceeds the
+ * alloted time and margin (here 60 + 180, or 240 seconds), the machine
+ * is restarted.  A healthy machine will have the duration match the
+ * expected timeout very closely.
+ */
+
+#include <linux/module.h>
+#include <linux/config.h>
+#include <linux/types.h>
+#include <linux/kernel.h>
+#include <linux/fs.h>
+#include <linux/mm.h>
+#include <linux/reboot.h>
+#include <linux/smp_lock.h>
+#include <linux/init.h>
+#include <asm/uaccess.h>
+
+
+#define VERSION_STR "0.5.0"
+
+static void version_hash_print (void)
+{
+	printk(KERN_INFO "I/O fencing modules %s\n", VERSION_STR);
+}
+
+#define DEFAULT_IOFENCE_MARGIN 60	/* Default fudge factor, in seconds */
+#define DEFAULT_IOFENCE_TICK 180	/* Default timer timeout, in seconds */
+
+static int hangcheck_tick = DEFAULT_IOFENCE_TICK;
+static int hangcheck_margin = DEFAULT_IOFENCE_MARGIN;
+static int hangcheck_reboot = 0;  /* Do not reboot */
+
+/* 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.");
+MODULE_LICENSE("GPL");
+
+/* options - nonmodular */
+#ifndef MODULE
+
+static int __init hangcheck_parse_tick(char *str) {
+	int par;
+	if (get_option(&str,&par))
+		hangcheck_tick = par;
+	return 1;
+}
+
+static int __init hangcheck_parse_margin(char *str) {
+	int par;
+	if (get_option(&str,&par))
+		hangcheck_margin = par;
+	return 1;
+}
+
+static int __init hangcheck_parse_reboot(char *str) {
+	int par;
+	if (get_option(&str,&par))
+		hangcheck_reboot = par;
+	return 1;
+}
+
+__setup("hcheck_tick", hangcheck_parse_tick);
+__setup("hcheck_margin", hangcheck_parse_margin);
+__setup("hcheck_reboot", hangcheck_parse_reboot);
+#endif /* not MODULE */
+
+
+/* Last time scheduled */
+static unsigned long long hangcheck_tsc, hangcheck_tsc_margin;
+
+static void hangcheck_fire(unsigned long);
+
+static struct timer_list hangcheck_ticktock = {
+	function:	hangcheck_fire,
+};
+
+
+static void hangcheck_fire(unsigned long data)
+{
+	unsigned long long cur_tsc, tsc_diff;
+
+	cur_tsc = get_cycles();
+
+	if (cur_tsc > hangcheck_tsc)
+		tsc_diff = cur_tsc - hangcheck_tsc;
+	else
+		tsc_diff = (cur_tsc + (~0ULL - hangcheck_tsc)); /* or something */
+
+#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 (tsc_diff > hangcheck_tsc_margin) {
+		if (hangcheck_reboot) {
+			printk(KERN_CRIT "Hangcheck: hangcheck is restarting the machine.\n");
+			machine_restart(NULL);
+		} else {
+			printk(KERN_CRIT "Hangcheck: hangcheck value past margin!\n");
+		}
+	}
+	mod_timer(&hangcheck_ticktock, jiffies + (hangcheck_tick*HZ));
+	hangcheck_tsc = get_cycles();
+}  /* hangcheck_fire() */
+
+
+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);
+
+	hangcheck_tsc_margin = (unsigned long long)(hangcheck_margin + hangcheck_tick) * (unsigned long long)HZ * (unsigned long long)current_cpu_data.loops_per_jiffy;
+
+	hangcheck_tsc = get_cycles();
+	mod_timer(&hangcheck_ticktock, jiffies + (hangcheck_tick*HZ));
+
+	return 0;
+}  /* hangcheck_init() */
+
+
+static void __exit hangcheck_exit(void)
+{
+	printk("Stopping hangcheck timer.\n");
+
+	lock_kernel();
+	del_timer(&hangcheck_ticktock);
+	unlock_kernel();
+}  /* hangcheck_exit() */
+
+module_init(hangcheck_init);
+module_exit(hangcheck_exit);

-- 

"Friends may come and go, but enemies accumulate." 
        - Thomas Jones

Joel Becker
Senior Member of Technical Staff
Oracle Corporation
E-mail: joel.becker@oracle.com
Phone: (650) 506-8127

Re: [PATCH][2.5] hangcheck-timer

[john stultz]

>         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.

get_cycles() is a poor method for determining "real time". 
Please use do_gettimeofday().

>         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

I believe you mean "udelay for 60 micro-seconds"

>  + * The hangcheck-timer driver uses the TSC to catch delays that
>  + * jiffies does not notice.  A timer is set.  When the timer fires, it
>  + * checks whether it was delayed and if that delay exceeds a given
>  + * margin of error.  The hangcheck_tick module paramter takes the timer
>  + * duration in seconds.  The hangcheck_margin parameter defines the
>  + * margin of error, in seconds.  The defaults are 60 seconds for the
>  + * timer and 180 seconds for the margin of error.  IOW, a timer is set
>  + * for 60 seconds.  When the timer fires, the callback checks the
>  + * actual duration that the timer waited.  If the duration exceeds the
>  + * alloted time and margin (here 60 + 180, or 240 seconds), the machine
>  + * is restarted.  A healthy machine will have the duration match the
>  + * expected timeout very closely.
>  + */

Wait, so 180 seconds is the margin of error?

[snip]

>  +#define DEFAULT_IOFENCE_MARGIN 60     /* Default fudge factor, in seconds */
>  +#define DEFAULT_IOFENCE_TICK 180      /* Default timer timeout, in seconds */
>  +

ok, time in seconds...


>  +static int hangcheck_tick = DEFAULT_IOFENCE_TICK;
>  +static int hangcheck_margin = DEFAULT_IOFENCE_MARGIN;
>  +static int hangcheck_reboot = 0;  /* Do not reboot */
>  +

so hangcheck_margin's unit is seconds...

[snip]
>  +static void hangcheck_fire(unsigned long data)
>  +{
>  +      unsigned long long cur_tsc, tsc_diff;
>  +
>  +      cur_tsc = get_cycles();
>  +
>  +      if (cur_tsc > hangcheck_tsc)
>  +              tsc_diff = cur_tsc - hangcheck_tsc;
>  +      else
>  +              tsc_diff = (cur_tsc + (~0ULL - hangcheck_tsc)); /* or something */
>  +
[snip]
>  +      
>  +      if (tsc_diff > hangcheck_tsc_margin) {

but now we're using it to compare cycles!  180sec != 180 cycles

Additionally, this code doesn't take systems that have unsync'ed TSCs,
or systems that change cpu frequency into account. Again, please use
do_gettimeofday(). Then you can then talk about the values returned in
secs and usecs, and I believe things will be much more clear. 

thanks
-john

Re: [PATCH][2.5] hangcheck-timer

[Joel Becker]

On Mon, Jan 20, 2003 at 05:42:16PM -0800, john stultz wrote:
> get_cycles() is a poor method for determining "real time". 
> Please use do_gettimeofday().

	Does do_gettimeofday() exist on all platforms?  Does it indeed
give actual wall clock time, instead of the inaccurate time jiffies can
give?

> I believe you mean "udelay for 60 micro-seconds"

	No, I mean 60 *seconds*.  There have been drivers and such that
do pause the entire box that long.

> Wait, so 180 seconds is the margin of error?

	Yup, for the default.  I'm not beholden to these specific
defaults.  They're just the defaults we use in our environment.

> >  +      if (tsc_diff > hangcheck_tsc_margin) {
> 
> but now we're using it to compare cycles!  180sec != 180 cycles

	Look at the calculations.  I'm comparing cycles to cycles,
calculated from the original seconds.

> Additionally, this code doesn't take systems that have unsync'ed TSCs,
> or systems that change cpu frequency into account. Again, please use
> do_gettimeofday(). Then you can then talk about the values returned in
> secs and usecs, and I believe things will be much more clear. 

	I'll look into it, but it must absolutely be in terms of wall
clock time as measured from outside the system.

Joel

-- 

Life's Little Instruction Book #99

	"Think big thoughts, but relish small pleasures."

Joel Becker
Senior Member of Technical Staff
Oracle Corporation
E-mail: joel.becker@oracle.com
Phone: (650) 506-8127

Re: [PATCH][2.5] hangcheck-timer

[john stultz]

On Mon, 2003-01-20 at 18:00, Joel Becker wrote:
> On Mon, Jan 20, 2003 at 05:42:16PM -0800, john stultz wrote:
> > get_cycles() is a poor method for determining "real time". 
> > Please use do_gettimeofday().
> 
> 	Does do_gettimeofday() exist on all platforms?  Does it indeed
> give actual wall clock time, instead of the inaccurate time jiffies can
> give?

Yep, do_gettimeofday is called from generic code in sys_gettimeofday()
(kernel/time.c). It returns the same value userspace code would see
calling gettimeofday(). 

> > >  +      if (tsc_diff > hangcheck_tsc_margin) {
> > 
> > but now we're using it to compare cycles!  180sec != 180 cycles
> 
> 	Look at the calculations.  I'm comparing cycles to cycles,
> calculated from the original seconds.

Ah! Ok, I missed the conversion in hangcheck_init. Even so, the default
initializer is misleading. Yea, that's it... :)

> > Additionally, this code doesn't take systems that have unsync'ed TSCs,
> > or systems that change cpu frequency into account. Again, please use
> > do_gettimeofday(). Then you can then talk about the values returned in
> > secs and usecs, and I believe things will be much more clear. 
> 
> 	I'll look into it, but it must absolutely be in terms of wall
> clock time as measured from outside the system.

Completely understandable. do_gettimeofday will give you just that (w/o
the conversion muck w/ HZ and loops_per_jiffy). 

thanks
-john

Re: [PATCH][2.5] hangcheck-timer

[Christoph Hellwig]

> +#include <linux/module.h>
> +#include <linux/config.h>

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

> +#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?

> +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.

> +/* 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.

> +#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?

> +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.

> +}  /* hangcheck_init() */

Bah 8)

> +static void __exit hangcheck_exit(void)
> +{
> +	printk("Stopping hangcheck timer.\n");

Again, this is exteremly verbose.

> +	lock_kernel();
> +	del_timer(&hangcheck_ticktock);
> +	unlock_kernel();

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

Re: [PATCH][2.5] hangcheck-timer

[Dave Jones]

On Mon, Jan 20, 2003 at 05:19:54PM -0800, Joel Becker wrote:
 > 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.

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

		Dave

-- 
| Dave Jones.        http://www.codemonkey.org.uk
| SuSE Labs

Re: [PATCH][2.5] hangcheck-timer

[Joel Becker]

On Mon, Jan 20, 2003 at 06:45:57PM -0800, john stultz wrote:
> > 	I'll look into it, but it must absolutely be in terms of wall
> > clock time as measured from outside the system.
> 
> Completely understandable. do_gettimeofday will give you just that (w/o
> the conversion muck w/ HZ and loops_per_jiffy). 

	It looks as though gettimeofday calculates wall time from
jiffies.  If you udelay(), jiffies doesn't increment and you lose time
(this is exactly what I'm trying to track here).  How does gettimeofday
avoid this (maybe I'm misreading the code)?  

Joel

-- 

"Can any of you seriously say the Bill of Rights could get through
 Congress today?  It wouldn't even get out of committee."
	- F. Lee Bailey

Joel Becker
Senior Member of Technical Staff
Oracle Corporation
E-mail: joel.becker@oracle.com
Phone: (650) 506-8127

Re: [PATCH][2.5] hangcheck-timer

[Joel Becker]

On Tue, Jan 21, 2003 at 08:11:58AM +0000, Christoph Hellwig wrote:
> 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.

> > +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.

> 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.

> #if DEBUG maybe? or VERBOSE?

	Probably just to remove.  Missed it.

Joel

-- 

 One look at the From:
 understanding has blossomed
 .procmailrc grows
	- Alexander Viro


Joel Becker
Senior Member of Technical Staff
Oracle Corporation
E-mail: joel.becker@oracle.com
Phone: (650) 506-8127

Re: [PATCH][2.5] hangcheck-timer

[Joel Becker]

On Tue, Jan 21, 2003 at 12:50:39PM +0000, Dave Jones wrote:
>  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


-- 

"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: joel.becker@oracle.com
Phone: (650) 506-8127

Re: [PATCH][2.5] hangcheck-timer

[Dave Jones]

On Tue, Jan 21, 2003 at 09:40:02AM -0800, Joel Becker wrote:

 > >  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

-- 
| Dave Jones.        http://www.codemonkey.org.uk
| SuSE Labs

Re: [PATCH][2.5] hangcheck-timer

[Joel Becker]

On Tue, Jan 21, 2003 at 05:42:37PM +0000, Dave Jones wrote:
> 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

-- 

Life's Little Instruction Book #511

	"Call your mother."

Joel Becker
Senior Member of Technical Staff
Oracle Corporation
E-mail: joel.becker@oracle.com
Phone: (650) 506-8127

Re: [PATCH][2.5] hangcheck-timer

[john stultz]

On Tue, 2003-01-21 at 09:29, Joel Becker wrote:
> On Mon, Jan 20, 2003 at 06:45:57PM -0800, john stultz wrote:
> > > 	I'll look into it, but it must absolutely be in terms of wall
> > > clock time as measured from outside the system.
> > 
> > Completely understandable. do_gettimeofday will give you just that (w/o
> > the conversion muck w/ HZ and loops_per_jiffy). 
> 
> 	It looks as though gettimeofday calculates wall time from
> jiffies.  If you udelay(), jiffies doesn't increment and you lose time
> (this is exactly what I'm trying to track here).  How does gettimeofday
> avoid this (maybe I'm misreading the code)?  

gettimeofday calculates wall time by adding xtime + jiffies-wall_jiffies
+ timer->offset(). In most cases timer->offset() is calculated from the
TSC, so even if jiffies does not increment, the offset value keeps on
increasing to keep wall time increasing. 

So ideally this would provide you with what you need. But, of course,
there is one gotcha... 

In the situation you describe, an interrupt happens (call this time_0),
jiffies is incremented and timer->offset() is restarted. Should the next
interrupt be blocked for 3 ticks, timer->offset() will compensate for
that time (time_1,time_2,time_3). However, when the next interrupt does
occur, the offset will be cleared again, causing a discontinuity in time
(back to time_1). 

I have been working on a patch to fix by detecting at interrupt time if
we have lost ticks (by checking against an alternate time source) and
compensating appropriately. Currently my fix is for the cyclone-timer
code in 2.4, and I'll be releasing a 2.5 version soon as well. Once that
is tested well enough I'll generate a TSC version for submission. 

So, currently do_gettimeofday() won't quite do it for you. Your
hangcheck_fire code will get get called after an interrupt, so the
timer->offset() has already been cleared. However, that will hopefully
not be case forever. Since I assume you want your module to work on
systems which do not necessarily have sync'ed TSCs (like certain NUMA
boxes), might you consider continuing using get_cycles() for now,
however place it behind a my_gettimeofday() function. That way, once
this issue is fixed, we can just do a simple
s/my_gettimeofday/do_gettimeofday on your code and be done with it?

In addition to making this easier to fix later, going from cycles->wall
time behind my_gettimeofday() should also fix the case where the cpu_feq
changes causing your earlier second->cycle calculation to be be invalid.
Let me know what you think of this.

thanks
-john

Oh yea, one more thing: I gave the earlier released 2.4 module a whirl,
and the printk() before machine_restart() never gets flushed out to the
logs. Makes it a bit confusing if you are trying to determine if the
hangcheck module or something else bounced the box. 

Re: [PATCH][2.5] hangcheck-timer

[Joel Becker]

On Tue, Jan 21, 2003 at 12:03:58PM -0800, john stultz wrote:
> boxes), might you consider continuing using get_cycles() for now,
> however place it behind a my_gettimeofday() function. That way, once
> this issue is fixed, we can just do a simple
> s/my_gettimeofday/do_gettimeofday on your code and be done with it?

	I'll look at that.  It's not as convenient an interface as one
long long, though.  :-)

> Oh yea, one more thing: I gave the earlier released 2.4 module a whirl,
> and the printk() before machine_restart() never gets flushed out to the
> logs. Makes it a bit confusing if you are trying to determine if the
> hangcheck module or something else bounced the box. 

	What kernel were you running?  I've seen printk()s get lost on a
couple Red Hat kernels, but not on vanilla.  That's not to say it can't
happen, of course.

Joel

-- 

Life's Little Instruction Book #274

	"Leave everything a little better than you found it."

Joel Becker
Senior Member of Technical Staff
Oracle Corporation
E-mail: joel.becker@oracle.com
Phone: (650) 506-8127