Re: [PATCH] fix percpu heartbeats

The changeset of Cort's patch that Troy checked in is broken in several ways:

1) The slave cpus no longer call heartbeats at all because there was a
previous continue in the loop. (reordered and used remembered cpu number)

2) The rtas call rate is per-minute not per-second.  (correct the rate)

3) All the rtas calls will occur at the same time, modulo the contended
rtas lock.  (wait to divide the rate until smp_kick_cpus).

4) It added NRCPUS*3 words to ppc_md.  It also pointed out we were modifing
this structre repeatedly at runtime.  [moved the counts to the cache hot
and dirty per-cpu irqstat and allow only one choice of function, just per
cpu countdown and period.  The count is always decremented, but only when
it reaches zero do we decide to call the function by checking the reset
value for non-zero.  The period is reset after the call, so aperiodic
users (like 4xx watchdog maybe?) could clear the count during their callback.]

I know there are other alternatives, but the cost to non-users is minimal
(an extra branch and compare every 2*32 jiffies), and the the periodic
call usage is set it up and forget it.


I'll read comments through the lists, so no need to cc me.

milton

===== arch/ppc/kernel/ppc4xx_setup.c 1.46 vs 1.47 =====

--- 1.46/arch/ppc/kernel/ppc4xx_setup.c	Fri Aug  2 11:30:01 2002
+++ 1.47/arch/ppc/kernel/ppc4xx_setup.c	Sun Aug 18 21:03:03 2002

@@ -383,6 +383,8 @@
 		strcpy(cmd_line, (char *) (r6 + KERNELBASE));
 	}
 #if defined(CONFIG_PPC405_WDT)
+	ppc_md.heartbeat = ppc4xx_wdt_heartbeat;
+
 /* Look for wdt= option on command line */
 	if (strstr(cmd_line, "wdt=")) {
 		int valid_wdt = 0;

@@ -396,6 +398,26 @@
 			++q;
 		}
 		wdt_enable = valid_wdt;
+
+		/* XXX why is the wdt functoin modifing count and not
+		 * using reset ??  If this code wants kernel heartbeat
+		 * calls then:
+		 *
+		 * #include <asm/hwirq.h>
+		 * heartbeat_reset(cpu) = period_in_jiffies;
+		 * heartbeat_count(cpu) = period_in_jiffies;
+		 *
+		 * The function will be called from the timer interrupt
+		 * when the counts reaches 0, afterwards the count will
+		 * be set to the heartbeat_reset(cpu) value (which
+		 * could be cleard during call for one-shot mode).
+		 *
+		 * If the code doesn't want the count manageed it
+		 * should not use the kernel callbacks (or set reset
+		 * to 1 and use its own counters).
+		 */
+#error sanitize wdt and kernel heartbeat interactions
+
 	}
 #endif

@@ -411,11 +433,6 @@
 	ppc_md.halt = ppc4xx_halt;

 	ppc_md.calibrate_decr = ppc4xx_calibrate_decr;
-
-#ifdef CONFIG_PPC405_WDT
-	ppc_md.heartbeat = ppc4xx_wdt_heartbeat;
-#endif
-	ppc_md.heartbeat_count = 0;

 	ppc_md.find_end_of_memory = ppc4xx_find_end_of_memory;
 	ppc_md.setup_io_mappings = m4xx_map_io;