[PATCH 0/6] SGI UV: Provide a LED driver and some System Activity Indicators

[PATCH 0/6] SGI UV: Provide a LED driver and some System Activity Indicators

[Mike Travis]

Add a LED driver for UV systems and some functions to display system
activities such as when a CPU is active, and when a CPU is handling
timer interrupts.

Based on linux-2.6.tip/master.

Signed-off-by: Mike Travis <travis@sgi.com>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Tony Luck <tony.luck@intel.com>

-- 

[PATCH 1/6] x86_64 UV: Provide a LED driver for UV Systems

[Mike Travis]

[-- Attachment #1: uv_led_driver_x86 --]
[-- Type: text/plain, Size: 4056 bytes --]

  * Add base functionality for writing to the LED's on
    the x86_64 UV architecture.

Note that this is a RAS feature that allows external monitoring of
various cpu state indicators, not just providing "pretty blinking
lights", as the LED state is readable by the system controller.

Based on linux-2.6.tip/master.

Signed-off-by: Mike Travis <travis@sgi.com>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
---
 arch/x86/kernel/genx2apic_uv_x.c |    2 +
 include/asm-x86/uv/uv_hub.h      |   60 ++++++++++++++++++++++++++++++++++++++-
 2 files changed, 61 insertions(+), 1 deletion(-)

--- linux-2.6.tip.orig/arch/x86/kernel/genx2apic_uv_x.c
+++ linux-2.6.tip/arch/x86/kernel/genx2apic_uv_x.c
@@ -433,6 +433,8 @@ static __init void uv_system_init(void)
 		uv_cpu_hub_info(cpu)->gnode_upper = gnode_upper;
 		uv_cpu_hub_info(cpu)->global_mmr_base = mmr_base;
 		uv_cpu_hub_info(cpu)->coherency_domain_number = 0;/* ZZZ */
+		uv_cpu_hub_info(cpu)->led_offset = LED_LOCAL_MMR_BASE + lcpu;
+		uv_cpu_hub_info(cpu)->led_state = 0;
 		uv_node_to_blade[nid] = blade;
 		uv_cpu_to_blade[cpu] = blade;
 		max_pnode = max(pnode, max_pnode);
--- linux-2.6.tip.orig/include/asm-x86/uv/uv_hub.h
+++ linux-2.6.tip/include/asm-x86/uv/uv_hub.h
@@ -123,6 +123,7 @@ struct uv_hub_info_s {
 	unsigned long	gnode_upper;
 	unsigned long	lowmem_remap_top;
 	unsigned long	lowmem_remap_base;
+	unsigned long	led_offset;
 	unsigned short	pnode;
 	unsigned short	pnode_mask;
 	unsigned short	coherency_domain_number;
@@ -130,6 +131,7 @@ struct uv_hub_info_s {
 	unsigned char	blade_processor_id;
 	unsigned char	m_val;
 	unsigned char	n_val;
+	unsigned char	led_state;
 };
 DECLARE_PER_CPU(struct uv_hub_info_s, __uv_hub_info);
 #define uv_hub_info 		(&__get_cpu_var(__uv_hub_info))
@@ -161,6 +163,19 @@ DECLARE_PER_CPU(struct uv_hub_info_s, __
 	((unsigned long)(p) << UV_GLOBAL_MMR64_PNODE_SHIFT)
 
 #define UV_APIC_PNODE_SHIFT	6
+/* Local Bus from cpu's perspective */
+#define LOCAL_BUS_BASE		0x1c00000
+#define LOCAL_BUS_SIZE		(4 * 1024 * 1024)
+
+/* LED windows - located at top of ACPI MMR space */
+#define LED_WINDOW_COUNT	64
+#define LED_LOCAL_MMR_BASE	(LOCAL_BUS_BASE + LOCAL_BUS_SIZE - \
+							LED_WINDOW_COUNT)
+
+#define LED_CPU_HEARTBEAT	0x01	/* timer interrupt */
+#define LED_CPU_ACTIVITY	0x02	/* not idle */
+#define LED_CPU_BLINK		0xffff	/* blink led */
+#define LED_CPU_HB_INTERVAL	(HZ/2)	/* blink once per second */
 
 /*
  * Macros for converting between kernel virtual addresses, socket local physical
@@ -276,6 +291,16 @@ static inline void uv_write_local_mmr(un
 	*uv_local_mmr_address(offset) = val;
 }
 
+static inline unsigned char uv_read_local_mmr8(unsigned long offset)
+{
+	return *((unsigned char *)uv_local_mmr_address(offset));
+}
+
+static inline void uv_write_local_mmr8(unsigned long offset, unsigned char val)
+{
+	*((unsigned char *)uv_local_mmr_address(offset)) = val;
+}
+
 /*
  * Structures and definitions for converting between cpu, node, pnode, and blade
  * numbers.
@@ -350,5 +375,38 @@ static inline int uv_num_possible_blades
 	return uv_possible_blades;
 }
 
-#endif /* ASM_X86__UV__UV_HUB_H */
+/* Light up the leds */
+static inline void uv_set_led_bits(unsigned short value, unsigned char mask)
+{
+	unsigned char state = uv_hub_info->led_state;
+
+	if (value == LED_CPU_BLINK)
+		state ^= mask;
+	else
+		state = (state & ~mask) | (value & mask);
 
+	if (uv_hub_info->led_state != state) {
+		uv_hub_info->led_state = state;
+		uv_write_local_mmr8(uv_hub_info->led_offset, state);
+	}
+}
+
+/* Light up the leds */
+static inline void uv_set_led_bits_on(int cpu, unsigned short value,
+					       unsigned char mask)
+
+{
+	unsigned char state = uv_cpu_hub_info(cpu)->led_state;
+
+	if (value == LED_CPU_BLINK)
+		state ^= mask;
+	else
+		state = (state & ~mask) | (value & mask);
+
+	if (uv_cpu_hub_info(cpu)->led_state != state) {
+		uv_cpu_hub_info(cpu)->led_state = state;
+		uv_write_local_mmr8(uv_cpu_hub_info(cpu)->led_offset, state);
+	}
+}
+
+#endif /* ASM_X86__UV__UV_HUB_H */

-- 

[PATCH 2/6] x86_64 UV: Use LED to indicate CPU is active

[Mike Travis]

[-- Attachment #1: uv_led_display_idle_x86 --]
[-- Type: text/plain, Size: 3479 bytes --]

  * Add an idle callback to turn on/off a LED indicating that
    the CPU is "active" (ON) or "idle" (OFF).

  * Introduces a callback for "post-smp_cpus_done" processing
    to setup callback.

Note that this is a RAS feature that allows external monitoring of
various cpu state indicators, not just providing "pretty blinking
lights", as the LED state is readable by the system controller.

Based on linux-2.6.tip/master.

Signed-off-by: Mike Travis <travis@sgi.com>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
---
 arch/x86/kernel/genx2apic_uv_x.c |   40 +++++++++++++++++++++++++++++++++++++++
 arch/x86/kernel/smpboot.c        |    6 +++++
 include/asm-x86/genapic_64.h     |    3 ++
 3 files changed, 49 insertions(+)

--- linux-2.6.tip.orig/arch/x86/kernel/genx2apic_uv_x.c
+++ linux-2.6.tip/arch/x86/kernel/genx2apic_uv_x.c
@@ -18,6 +18,7 @@
 #include <linux/bootmem.h>
 #include <linux/module.h>
 #include <linux/hardirq.h>
+#include <asm/idle.h>
 #include <asm/smp.h>
 #include <asm/ipi.h>
 #include <asm/genapic.h>
@@ -28,6 +29,8 @@
 
 DEFINE_PER_CPU(int, x2apic_extra_bits);
 
+static __init void uv_start_system(void);
+
 static enum uv_system_type uv_system_type;
 
 static int __init uv_acpi_madt_oem_check(char *oem_id, char *oem_table_id)
@@ -449,6 +452,9 @@ static __init void uv_system_init(void)
 	map_mmr_high(max_pnode);
 	map_config_high(max_pnode);
 	map_mmioh_high(max_pnode);
+
+	/* enable post-smp_cpus_done processing */
+	smp_cpus_done_system = uv_start_system;
 }
 
 /*
@@ -466,4 +472,38 @@ void __cpuinit uv_cpu_init(void)
 		set_x2apic_extra_bits(uv_hub_info->pnode);
 }
 
+/*
+ * Illuminate "activity" LED when CPU is going "active",
+ * extinguish when going "idle".
+ */
+static int uv_idle(struct notifier_block *nfb, unsigned long action, void *junk)
+{
+	if (action == IDLE_START)
+		uv_set_led_bits(0, LED_CPU_ACTIVITY);
+
+	else if (action == IDLE_END)
+		uv_set_led_bits(LED_CPU_ACTIVITY, LED_CPU_ACTIVITY);
+
+	return NOTIFY_OK;
+}
+
+static struct notifier_block uv_idle_notifier = {
+	.notifier_call = uv_idle,
+};
 
+/*
+ * Initialize idle led callback function
+ */
+static __init void uv_init_led_idle_display(void)
+{
+	/* initialize timer for activity monitor */
+	idle_notifier_register(&uv_idle_notifier);
+}
+
+/*
+ * Initialize subsystems that need to start after the system is up
+ */
+static __init void uv_start_system(void)
+{
+	uv_init_led_idle_display();
+}
--- linux-2.6.tip.orig/arch/x86/kernel/smpboot.c
+++ linux-2.6.tip/arch/x86/kernel/smpboot.c
@@ -1200,6 +1200,9 @@ void __init native_smp_prepare_boot_cpu(
 	per_cpu(cpu_state, me) = CPU_ONLINE;
 }
 
+/* post-smp_cpus_done processing */
+void (*smp_cpus_done_system)(void);
+
 void __init native_smp_cpus_done(unsigned int max_cpus)
 {
 	pr_debug("Boot done.\n");
@@ -1210,6 +1213,9 @@ void __init native_smp_cpus_done(unsigne
 	setup_ioapic_dest();
 #endif
 	check_nmi_watchdog();
+
+	if (smp_cpus_done_system)
+		smp_cpus_done_system();
 }
 
 #ifdef CONFIG_HOTPLUG_CPU
--- linux-2.6.tip.orig/include/asm-x86/genapic_64.h
+++ linux-2.6.tip/include/asm-x86/genapic_64.h
@@ -47,6 +47,9 @@ enum uv_system_type {UV_NONE, UV_LEGACY_
 extern enum uv_system_type get_uv_system_type(void);
 extern int is_uv_system(void);
 
+/* system tasks to run after smp_cpus_done */
+extern void (*smp_cpus_done_system)(void);
+
 extern struct genapic apic_x2apic_uv_x;
 DECLARE_PER_CPU(int, x2apic_extra_bits);
 extern void uv_cpu_init(void);

-- 

Re: [PATCH 2/6] x86_64 UV: Use LED to indicate CPU is active

[Ingo Molnar]

* Mike Travis <travis@sgi.com> wrote:

> +/*
> + * Illuminate "activity" LED when CPU is going "active",
> + * extinguish when going "idle".
> + */
> +static int uv_idle(struct notifier_block *nfb, unsigned long action, void *junk)
> +{
> +	if (action == IDLE_START)
> +		uv_set_led_bits(0, LED_CPU_ACTIVITY);
> +
> +	else if (action == IDLE_END)
> +		uv_set_led_bits(LED_CPU_ACTIVITY, LED_CPU_ACTIVITY);
> +
> +	return NOTIFY_OK;
> +}
> +
> +static struct notifier_block uv_idle_notifier = {
> +	.notifier_call = uv_idle,
> +};
>  
> +/*
> + * Initialize idle led callback function
> + */
> +static __init void uv_init_led_idle_display(void)
> +{
> +	/* initialize timer for activity monitor */
> +	idle_notifier_register(&uv_idle_notifier);
> +}

hm, i think this is a bad idea. While putting it into the go-to-idle 
codepath probably doesnt matter, putting anything into the idle wakeup 
path increases latency of a rather critical codepath. The MMR write that 
the LED driver is using will go out to the local bus, which, even if 
it's POST-ed, if it's done frequently enough will be the cost of an IO 
cycle.

No human needs to know the LED status at _that_ frequency anyway, so 
it's quite pointless as well.

A much better (and faster) approach would be to sample the utilization 
of the CPU and indicate that via the LED(s).

	Ingo

Re: [PATCH 2/6] x86_64 UV: Use LED to indicate CPU is active

[Pavel Machek]

Hi!

> > +/*
> > + * Illuminate "activity" LED when CPU is going "active",
> > + * extinguish when going "idle".
> > + */
> > +static int uv_idle(struct notifier_block *nfb, unsigned long action, void *junk)
> > +{
> > +	if (action == IDLE_START)
> > +		uv_set_led_bits(0, LED_CPU_ACTIVITY);
> > +
> > +	else if (action == IDLE_END)
> > +		uv_set_led_bits(LED_CPU_ACTIVITY, LED_CPU_ACTIVITY);
> > +
> > +	return NOTIFY_OK;
> > +}
> > +
> > +static struct notifier_block uv_idle_notifier = {
> > +	.notifier_call = uv_idle,
> > +};
> >  
> > +/*
> > + * Initialize idle led callback function
> > + */
> > +static __init void uv_init_led_idle_display(void)
> > +{
> > +	/* initialize timer for activity monitor */
> > +	idle_notifier_register(&uv_idle_notifier);
> > +}
> 
> hm, i think this is a bad idea. While putting it into the go-to-idle 

Plus this really should not be uv-specific. We already have generic
triggers.

And you should really CC led maintainers!
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH 2/6] x86_64 UV: Use LED to indicate CPU is active

[Mike Travis]

Pavel Machek wrote:
> Hi!
> 
>>> +/*
>>> + * Illuminate "activity" LED when CPU is going "active",
>>> + * extinguish when going "idle".
>>> + */
>>> +static int uv_idle(struct notifier_block *nfb, unsigned long action, void *junk)
>>> +{
>>> +	if (action == IDLE_START)
>>> +		uv_set_led_bits(0, LED_CPU_ACTIVITY);
>>> +
>>> +	else if (action == IDLE_END)
>>> +		uv_set_led_bits(LED_CPU_ACTIVITY, LED_CPU_ACTIVITY);
>>> +
>>> +	return NOTIFY_OK;
>>> +}
>>> +
>>> +static struct notifier_block uv_idle_notifier = {
>>> +	.notifier_call = uv_idle,
>>> +};
>>>  
>>> +/*
>>> + * Initialize idle led callback function
>>> + */
>>> +static __init void uv_init_led_idle_display(void)
>>> +{
>>> +	/* initialize timer for activity monitor */
>>> +	idle_notifier_register(&uv_idle_notifier);
>>> +}
>> hm, i think this is a bad idea. While putting it into the go-to-idle 
> 
> Plus this really should not be uv-specific. We already have generic
> triggers.
> 
> And you should really CC led maintainers!
> 									Pavel

Thanks for the pointers, I will definitely look into this.

(And sorry for not discovering the LED docs and maintainers earlier.)

Mike

Re: [PATCH 2/6] x86_64 UV: Use LED to indicate CPU is active

[Mike Travis]

Ingo Molnar wrote:
> * Mike Travis <travis@sgi.com> wrote:
> 
>> +/*
>> + * Illuminate "activity" LED when CPU is going "active",
>> + * extinguish when going "idle".
>> + */
>> +static int uv_idle(struct notifier_block *nfb, unsigned long action, void *junk)
>> +{
>> +	if (action == IDLE_START)
>> +		uv_set_led_bits(0, LED_CPU_ACTIVITY);
>> +
>> +	else if (action == IDLE_END)
>> +		uv_set_led_bits(LED_CPU_ACTIVITY, LED_CPU_ACTIVITY);
>> +
>> +	return NOTIFY_OK;
>> +}
>> +
>> +static struct notifier_block uv_idle_notifier = {
>> +	.notifier_call = uv_idle,
>> +};
>>  
>> +/*
>> + * Initialize idle led callback function
>> + */
>> +static __init void uv_init_led_idle_display(void)
>> +{
>> +	/* initialize timer for activity monitor */
>> +	idle_notifier_register(&uv_idle_notifier);
>> +}
> 
> hm, i think this is a bad idea. While putting it into the go-to-idle 
> codepath probably doesnt matter, putting anything into the idle wakeup 
> path increases latency of a rather critical codepath. The MMR write that 
> the LED driver is using will go out to the local bus, which, even if 
> it's POST-ed, if it's done frequently enough will be the cost of an IO 
> cycle.
> 
> No human needs to know the LED status at _that_ frequency anyway, so 
> it's quite pointless as well.
> 
> A much better (and faster) approach would be to sample the utilization 
> of the CPU and indicate that via the LED(s).
> 
> 	Ingo

Hi Ingo,

I'll take a closer look at this.  The actual usage is more in line with
an SNMP type system controller that has direct access to the BIOS registers
including the "LED" register and is used mostly for diagnosing system
problems.  The other research I need to do is how fast/slow is this path.
It may be a simple memory write in our UV NUMA hub chip that's not directed
to the general I/O bus and hence, negligible overhead.

Looking at the IA64 code, it has a similar interface which uses a function
pointer variable to set/reset "idle" state.  I just wish x86_64 had a similar
common interface for the clock handler that would occur per cpu and not per
group of cpus that clocksource_watchdog() currently uses.

The other architectural change I would like to make is to expose the necessary
data/address via the pda (or something similar) and allow the complete write
to be inlined to one or two instructions.

Thanks,
Mike

[PATCH 3/6] x86_64 UV: Use blinking LED for heartbeat display

[Mike Travis]

[-- Attachment #1: uv_led_display_hb_x86 --]
[-- Type: text/plain, Size: 4158 bytes --]

  * Add a function to blink the "heartbeat" LED indicating
    that the CPU is responding to timer interrupts.

  * Introduces a callback for "heartbeat" display in the
    clocksource_watchdog function, which in turn requires
    CONFIG_CLOCKSOURCE_WATCHDOG to be set.

Note that this is a RAS feature that allows external monitoring of
various cpu state indicators, not just providing "pretty blinking
lights", as the LED state is readable by the system controller.

Based on linux-2.6.tip/master.

Signed-off-by: Mike Travis <travis@sgi.com>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
---
 arch/x86/kernel/genx2apic_uv_x.c |   29 +++++++++++++++++++++++++++++
 include/asm-x86/uv/uv_hub.h      |    1 +
 include/linux/clocksource.h      |    5 +++++
 kernel/time/clocksource.c        |   10 +++++++++-
 4 files changed, 44 insertions(+), 1 deletion(-)

--- linux-2.6.tip.orig/arch/x86/kernel/genx2apic_uv_x.c
+++ linux-2.6.tip/arch/x86/kernel/genx2apic_uv_x.c
@@ -10,6 +10,7 @@
 
 #include <linux/kernel.h>
 #include <linux/threads.h>
+#include <linux/clocksource.h>
 #include <linux/cpumask.h>
 #include <linux/string.h>
 #include <linux/ctype.h>
@@ -360,6 +361,25 @@ static __init void uv_rtc_init(void)
 		sn_rtc_cycles_per_second = ticks_per_sec;
 }
 
+#ifdef CONFIG_CLOCKSOURCE_WATCHDOG
+static void uv_display_heartbeat(void)
+{
+	int cpu;
+
+	uv_hub_info->led_heartbeat_count = nr_cpu_ids;
+
+	for_each_online_cpu(cpu) {
+		struct uv_hub_info_s *hub = uv_cpu_hub_info(cpu);
+
+		if (hub->led_heartbeat_count > 0) {
+			uv_set_led_bits_on(cpu, LED_CPU_BLINK,
+						LED_CPU_HEARTBEAT);
+			--hub->led_heartbeat_count;
+		}
+	}
+}
+#endif
+
 static __init void uv_system_init(void)
 {
 	union uvh_si_addr_map_config_u m_n_config;
@@ -438,6 +458,7 @@ static __init void uv_system_init(void)
 		uv_cpu_hub_info(cpu)->coherency_domain_number = 0;/* ZZZ */
 		uv_cpu_hub_info(cpu)->led_offset = LED_LOCAL_MMR_BASE + lcpu;
 		uv_cpu_hub_info(cpu)->led_state = 0;
+		uv_cpu_hub_info(cpu)->led_heartbeat_count = nr_cpu_ids;
 		uv_node_to_blade[nid] = blade;
 		uv_cpu_to_blade[cpu] = blade;
 		max_pnode = max(pnode, max_pnode);
@@ -455,6 +476,14 @@ static __init void uv_system_init(void)
 
 	/* enable post-smp_cpus_done processing */
 	smp_cpus_done_system = uv_start_system;
+
+#ifdef CONFIG_CLOCKSOURCE_WATCHDOG
+	/* enable heartbeat display callback */
+	display_heartbeat = uv_display_heartbeat;
+#else
+	printk(KERN_NOTICE "UV: CLOCKSOURCE_WATCHDOG not configured, "
+			   "LED Heartbeat NOT enabled\n");
+#endif
 }
 
 /*
--- linux-2.6.tip.orig/include/asm-x86/uv/uv_hub.h
+++ linux-2.6.tip/include/asm-x86/uv/uv_hub.h
@@ -124,6 +124,7 @@ struct uv_hub_info_s {
 	unsigned long	lowmem_remap_top;
 	unsigned long	lowmem_remap_base;
 	unsigned long	led_offset;
+	unsigned int	led_heartbeat_count;
 	unsigned short	pnode;
 	unsigned short	pnode_mask;
 	unsigned short	coherency_domain_number;
--- linux-2.6.tip.orig/include/linux/clocksource.h
+++ linux-2.6.tip/include/linux/clocksource.h
@@ -236,4 +236,9 @@ static inline void update_vsyscall_tz(vo
 }
 #endif
 
+#ifdef CONFIG_CLOCKSOURCE_WATCHDOG
+/* typically blinks "heartbeat" LED */
+extern void (*display_heartbeat)(void);
+#endif
+
 #endif /* _LINUX_CLOCKSOURCE_H */
--- linux-2.6.tip.orig/kernel/time/clocksource.c
+++ linux-2.6.tip/kernel/time/clocksource.c
@@ -76,6 +76,9 @@ static DEFINE_SPINLOCK(watchdog_lock);
 static cycle_t watchdog_last;
 static unsigned long watchdog_resumed;
 
+/* arch specific heartbeat display */
+void (*display_heartbeat)(void);
+
 /*
  * Interval: 0.5sec Threshold: 0.0625s
  */
@@ -140,12 +143,17 @@ static void clocksource_watchdog(unsigne
 		}
 	}
 
+	/* If arch has a heartbeat display, then poke it */
+	if (display_heartbeat)
+		display_heartbeat();
+
 	if (!list_empty(&watchdog_list)) {
 		/*
 		 * Cycle through CPUs to check if the CPUs stay
 		 * synchronized to each other.
 		 */
-		int next_cpu = next_cpu_nr(raw_smp_processor_id(), cpu_online_map);
+		int next_cpu = next_cpu_nr(raw_smp_processor_id(),
+					   cpu_online_map);
 
 		if (next_cpu >= nr_cpu_ids)
 			next_cpu = first_cpu(cpu_online_map);

-- 

Re: [PATCH 3/6] x86_64 UV: Use blinking LED for heartbeat display

[Ingo Molnar]

* Mike Travis <travis@sgi.com> wrote:

> +#ifdef CONFIG_CLOCKSOURCE_WATCHDOG
> +static void uv_display_heartbeat(void)
> +{
> +	int cpu;
> +
> +	uv_hub_info->led_heartbeat_count = nr_cpu_ids;
> +
> +	for_each_online_cpu(cpu) {
> +		struct uv_hub_info_s *hub = uv_cpu_hub_info(cpu);
> +
> +		if (hub->led_heartbeat_count > 0) {
> +			uv_set_led_bits_on(cpu, LED_CPU_BLINK,
> +						LED_CPU_HEARTBEAT);
> +			--hub->led_heartbeat_count;
> +		}

this too is a bad idea. Imagine 16K cores and assume that each such 
iteration takes a few usecs (we write cross CPU) and you've got a 
GHz-ish CPU. That can easily be _milliseconds_ of delay (or more) - and 
in a function (the clocksource watchdog) that is all about precise 
timings.

It is also very non-preemptable.

Why not have a separate per cpu kthread for this that does this in a 
preemptable manner?

Also, why not let each CPU's heartbeat be set in a hierarchy instead of 
by _all_ CPUs. That way you get a nice constant-ish overhead instead of 
the current crazy quadratic(nr_cpus) behavior. I.e. let each CPU be 
monitored by its neighbor (cpu_id + 1), by it's second-order neighbor 
(cpu_id + 2), third-order neighbor (cpu_id + 4), etc.

That still gives pretty good coverage in practice while avoiding the 
quadratic nature.

	Ingo

Re: [PATCH 3/6] x86_64 UV: Use blinking LED for heartbeat display

[Pavel Machek]

> 
> * Mike Travis <travis@sgi.com> wrote:
> 
> > +#ifdef CONFIG_CLOCKSOURCE_WATCHDOG
> > +static void uv_display_heartbeat(void)
> > +{
> > +	int cpu;
> > +
> > +	uv_hub_info->led_heartbeat_count = nr_cpu_ids;
> > +
> > +	for_each_online_cpu(cpu) {
> > +		struct uv_hub_info_s *hub = uv_cpu_hub_info(cpu);
> > +
> > +		if (hub->led_heartbeat_count > 0) {
> > +			uv_set_led_bits_on(cpu, LED_CPU_BLINK,
> > +						LED_CPU_HEARTBEAT);
> > +			--hub->led_heartbeat_count;
> > +		}
> 
> this too is a bad idea. Imagine 16K cores and assume that each such 
> iteration takes a few usecs (we write cross CPU) and you've got a 
> GHz-ish CPU. That can easily be _milliseconds_ of delay (or more) - and 
> in a function (the clocksource watchdog) that is all about precise 
> timings.
> 
> It is also very non-preemptable.

LED subsystem already has nice heartbeat trigger.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH 3/6] x86_64 UV: Use blinking LED for heartbeat display

[Mike Travis]

Pavel Machek wrote:
>> * Mike Travis <travis@sgi.com> wrote:
>>
>>> +#ifdef CONFIG_CLOCKSOURCE_WATCHDOG
>>> +static void uv_display_heartbeat(void)
>>> +{
>>> +	int cpu;
>>> +
>>> +	uv_hub_info->led_heartbeat_count = nr_cpu_ids;
>>> +
>>> +	for_each_online_cpu(cpu) {
>>> +		struct uv_hub_info_s *hub = uv_cpu_hub_info(cpu);
>>> +
>>> +		if (hub->led_heartbeat_count > 0) {
>>> +			uv_set_led_bits_on(cpu, LED_CPU_BLINK,
>>> +						LED_CPU_HEARTBEAT);
>>> +			--hub->led_heartbeat_count;
>>> +		}
>> this too is a bad idea. Imagine 16K cores and assume that each such 
>> iteration takes a few usecs (we write cross CPU) and you've got a 
>> GHz-ish CPU. That can easily be _milliseconds_ of delay (or more) - and 
>> in a function (the clocksource watchdog) that is all about precise 
>> timings.
>>
>> It is also very non-preemptable.
> 
> LED subsystem already has nice heartbeat trigger.
> 									Pavel

>From Documentation/leds-class.txt:


Future Development
==================

At the moment, a trigger can't be created specifically for a single LED.
There are a number of cases where a trigger might only be mappable to a
particular LED (ACPI?). The addition of triggers provided by the LED driver
should cover this option and be possible to add without breaking the
current interface.

--
The SGI system has a set of leds per cpu, and the goal is that the leds
display heartbeat and "idle-ness" information specific to that cpu.  At
first glance I don't see this capability in the LED subsystem.  Am I
missing something?

Additionally, the 8 leds are written as one byte with each led being
full on or full off.  It seems to be a big overhead to support the led
class  since I'd have to kluge together some means of sharing the current
led register value [we really don't want to have to read the current reg
value before updating a single led.]

But I will definitely look at the led_heartbeat_function more closely.

Any advice gladly welcomed!

Thanks!
Mike

Re: [PATCH 3/6] x86_64 UV: Use blinking LED for heartbeat display

[Pavel Machek]

On Tue 2008-08-26 13:53:48, Mike Travis wrote:
> Pavel Machek wrote:
> >> * Mike Travis <travis@sgi.com> wrote:
> >>
> >>> +#ifdef CONFIG_CLOCKSOURCE_WATCHDOG
> >>> +static void uv_display_heartbeat(void)
> >>> +{
> >>> +	int cpu;
> >>> +
> >>> +	uv_hub_info->led_heartbeat_count = nr_cpu_ids;
> >>> +
> >>> +	for_each_online_cpu(cpu) {
> >>> +		struct uv_hub_info_s *hub = uv_cpu_hub_info(cpu);
> >>> +
> >>> +		if (hub->led_heartbeat_count > 0) {
> >>> +			uv_set_led_bits_on(cpu, LED_CPU_BLINK,
> >>> +						LED_CPU_HEARTBEAT);
> >>> +			--hub->led_heartbeat_count;
> >>> +		}
> >> this too is a bad idea. Imagine 16K cores and assume that each such 
> >> iteration takes a few usecs (we write cross CPU) and you've got a 
> >> GHz-ish CPU. That can easily be _milliseconds_ of delay (or more) - and 
> >> in a function (the clocksource watchdog) that is all about precise 
> >> timings.
> >>
> >> It is also very non-preemptable.
> > 
> > LED subsystem already has nice heartbeat trigger.
> > 									Pavel
> 
> From Documentation/leds-class.txt:

> Future Development
> ==================
> 
> At the moment, a trigger can't be created specifically for a single LED.
> There are a number of cases where a trigger might only be mappable to a
> particular LED (ACPI?). The addition of triggers provided by the LED driver
> should cover this option and be possible to add without breaking the
> current interface.

I don't think this affects you.

> --
> The SGI system has a set of leds per cpu, and the goal is that the leds
> display heartbeat and "idle-ness" information specific to that cpu.  At
> first glance I don't see this capability in the LED subsystem.  Am I
> missing something?

Set the heartbeat trigger to the led you care about. Should be doable
with simple echo to /sys after you implement your leds properly.

> Additionally, the 8 leds are written as one byte with each led being
> full on or full off.  It seems to be a big overhead to support the led
> class  since I'd have to kluge together some means of sharing the current
> led register value [we really don't want to have to read the current reg
> value before updating a single led.]

Big overhead? Just use shadow register of led state in ram.

> But I will definitely look at the led_heartbeat_function more closely.
> 
> Any advice gladly welcomed!

You still did not cc LED developers.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH 3/6] x86_64 UV: Use blinking LED for heartbeat display

[Mike Travis]

Ingo Molnar wrote:
> * Mike Travis <travis@sgi.com> wrote:
> 
>> +#ifdef CONFIG_CLOCKSOURCE_WATCHDOG
>> +static void uv_display_heartbeat(void)
>> +{
>> +	int cpu;
>> +
>> +	uv_hub_info->led_heartbeat_count = nr_cpu_ids;
>> +
>> +	for_each_online_cpu(cpu) {
>> +		struct uv_hub_info_s *hub = uv_cpu_hub_info(cpu);
>> +
>> +		if (hub->led_heartbeat_count > 0) {
>> +			uv_set_led_bits_on(cpu, LED_CPU_BLINK,
>> +						LED_CPU_HEARTBEAT);
>> +			--hub->led_heartbeat_count;
>> +		}
> 
> this too is a bad idea. Imagine 16K cores and assume that each such 
> iteration takes a few usecs (we write cross CPU) and you've got a 
> GHz-ish CPU. That can easily be _milliseconds_ of delay (or more) - and 
> in a function (the clocksource watchdog) that is all about precise 
> timings.
> 
> It is also very non-preemptable.
> 
> Why not have a separate per cpu kthread for this that does this in a 
> preemptable manner?
> 
> Also, why not let each CPU's heartbeat be set in a hierarchy instead of 
> by _all_ CPUs. That way you get a nice constant-ish overhead instead of 
> the current crazy quadratic(nr_cpus) behavior. I.e. let each CPU be 
> monitored by its neighbor (cpu_id + 1), by it's second-order neighbor 
> (cpu_id + 2), third-order neighbor (cpu_id + 4), etc.
> 
> That still gives pretty good coverage in practice while avoiding the 
> quadratic nature.
> 
> 	Ingo

Yes, I agree 100%.  There was a trade off with various approaches but I
was hoping for some feedback on alternate approaches (and thanks for that!)

Mike

[PATCH 4/6] ia64 UV: Provide a LED driver for UV Systems

[Mike Travis]

[-- Attachment #1: uv_led_driver_ia64 --]
[-- Type: text/plain, Size: 3998 bytes --]

  * Add base functionality for writing to the LED's on
    the ia64 UV architecture.

Note that this is a RAS feature that allows external monitoring of
various cpu state indicators, not just providing "pretty blinking
lights", as the LED state is readable by the system controller.

Based on linux-2.6.tip/master.

Signed-off-by: Mike Travis <travis@sgi.com>
Cc: Tony Luck <tony.luck@intel.com>
---
 arch/ia64/uv/kernel/setup.c  |    2 +
 include/asm-ia64/uv/uv_hub.h |   61 ++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 62 insertions(+), 1 deletion(-)

--- linux-2.6.tip.orig/arch/ia64/uv/kernel/setup.c
+++ linux-2.6.tip/arch/ia64/uv/kernel/setup.c
@@ -104,6 +104,8 @@ void __init uv_setup(char **cmdline_p)
 		uv_cpu_hub_info(cpu)->gnode_upper = gnode_upper;
 		uv_cpu_hub_info(cpu)->global_mmr_base = mmr_base;
 		uv_cpu_hub_info(cpu)->coherency_domain_number = 0;/* ZZZ */
+		uv_cpu_hub_info(cpu)->led_offset = LED_LOCAL_MMR_BASE + lcpu;
+		uv_cpu_hub_info(cpu)->led_state = 0;
 		printk(KERN_DEBUG "UV cpu %d, nid %d\n", cpu, nid);
 	}
 }
--- linux-2.6.tip.orig/include/asm-ia64/uv/uv_hub.h
+++ linux-2.6.tip/include/asm-ia64/uv/uv_hub.h
@@ -99,6 +99,7 @@ struct uv_hub_info_s {
 	unsigned long	gnode_upper;
 	unsigned long	lowmem_remap_top;
 	unsigned long	lowmem_remap_base;
+	unsigned long	led_offset;
 	unsigned short	pnode;
 	unsigned short	pnode_mask;
 	unsigned short	coherency_domain_number;
@@ -106,6 +107,7 @@ struct uv_hub_info_s {
 	unsigned char	blade_processor_id;
 	unsigned char	m_val;
 	unsigned char	n_val;
+	unsigned char	led_state;
 };
 DECLARE_PER_CPU(struct uv_hub_info_s, __uv_hub_info);
 #define uv_hub_info 		(&__get_cpu_var(__uv_hub_info))
@@ -134,6 +136,20 @@ DECLARE_PER_CPU(struct uv_hub_info_s, __
 #define UV_GLOBAL_MMR64_PNODE_BITS(p)					\
 	((unsigned long)(p) << UV_GLOBAL_MMR64_PNODE_SHIFT)
 
+/* Local Bus from cpu's perspective */
+#define LOCAL_BUS_BASE		0x1c00000
+#define LOCAL_BUS_SIZE		(4 * 1024 * 1024)
+
+/* LED windows - located at top of ACPI MMR space */
+#define LED_WINDOW_COUNT	64
+#define LED_LOCAL_MMR_BASE	(LOCAL_BUS_BASE + LOCAL_BUS_SIZE - \
+							LED_WINDOW_COUNT)
+
+#define LED_CPU_HEARTBEAT	0x01	/* timer interrupt */
+#define LED_CPU_ACTIVITY	0x02	/* not idle */
+#define LED_CPU_BLINK		0xffff	/* blink led */
+#define LED_CPU_HB_INTERVAL	(HZ/2)	/* blink once per second */
+
 /*
  * Macros for converting between kernel virtual addresses, socket local physical
  * addresses, and UV global physical addresses.
@@ -240,6 +256,16 @@ static inline void uv_write_local_mmr(un
 	*uv_local_mmr_address(offset) = val;
 }
 
+static inline unsigned char uv_read_local_mmr8(unsigned long offset)
+{
+	return *((unsigned char *)uv_local_mmr_address(offset));
+}
+
+static inline void uv_write_local_mmr8(unsigned long offset, unsigned char val)
+{
+	*((unsigned char *)uv_local_mmr_address(offset)) = val;
+}
+
 /*
  * Structures and definitions for converting between cpu, node, pnode, and blade
  * numbers.
@@ -305,5 +331,38 @@ static inline int uv_num_possible_blades
 	return 1;
 }
 
-#endif /* __ASM_IA64_UV_HUB__ */
+/* Light up the leds */
+static inline void uv_set_led_bits(unsigned short value, unsigned char mask)
+{
+	unsigned char state = uv_hub_info->led_state;
+
+	if (value == LED_CPU_BLINK)
+		state ^= mask;
+	else
+		state = (state & ~mask) | (value & mask);
 
+	if (uv_hub_info->led_state != state) {
+		uv_hub_info->led_state = state;
+		uv_write_local_mmr8(uv_hub_info->led_offset, state);
+	}
+}
+
+/* Light up the leds */
+static inline void uv_set_led_bits_on(int cpu, unsigned short value,
+					       unsigned char mask)
+
+{
+	unsigned char state = uv_cpu_hub_info(cpu)->led_state;
+
+	if (value == LED_CPU_BLINK)
+		state ^= mask;
+	else
+		state = (state & ~mask) | (value & mask);
+
+	if (uv_cpu_hub_info(cpu)->led_state != state) {
+		uv_cpu_hub_info(cpu)->led_state = state;
+		uv_write_local_mmr8(uv_cpu_hub_info(cpu)->led_offset, state);
+	}
+}
+
+#endif /* __ASM_IA64_UV_HUB__ */

-- 

Re: [PATCH 4/6] ia64 UV: Provide a LED driver for UV Systems

[Andrew Morton]

On Thu, 07 Aug 2008 17:56:43 -0700 Mike Travis <travis@sgi.com> wrote:

>  * Add base functionality for writing to the LED's on
>     the ia64 UV architecture.
> 
> Note that this is a RAS feature that allows external monitoring of
> various cpu state indicators, not just providing "pretty blinking
> lights", as the LED state is readable by the system controller.
> 
> Based on linux-2.6.tip/master.
> 
> Signed-off-by: Mike Travis <travis@sgi.com>
> Cc: Tony Luck <tony.luck@intel.com>
> ---
>  arch/ia64/uv/kernel/setup.c  |    2 +
>  include/asm-ia64/uv/uv_hub.h |   61 ++++++++++++++++++++++++++++++++++++++++++-

It's arch/ia64/include/asm/uv/uv_hub.h in my kernel.  Weird.

[PATCH 5/6] ia64 UV: Use LED to indicate CPU is active

[Mike Travis]

[-- Attachment #1: uv_led_display_idle_ia64 --]
[-- Type: text/plain, Size: 2174 bytes --]

  * Add an idle callback to turn on/off a LED indicating that
    the CPU is "active" (ON) or "idle" (OFF).

  * Introduces a callback for "post-smp_cpus_done" processing
    to setup callback.

Note that this is a RAS feature that allows external monitoring of
various cpu state indicators, not just providing "pretty blinking
lights", as the LED state is readable by the system controller.

Based on linux-2.6.tip/master.

Signed-off-by: Mike Travis <travis@sgi.com>
Cc: Tony Luck <tony.luck@intel.com>
---
 arch/ia64/uv/kernel/setup.c  |   19 +++++++++++++++++--
 include/asm-ia64/uv/uv_hub.h |    3 +++
 2 files changed, 20 insertions(+), 2 deletions(-)

--- linux-2.6.tip.orig/arch/ia64/uv/kernel/setup.c
+++ linux-2.6.tip/arch/ia64/uv/kernel/setup.c
@@ -52,6 +52,19 @@ static __init void get_lowmem_redirect(u
 	BUG();
 }
 
+/*
+ * Illuminate "activity" LED when CPU is going "active",
+ * extinguish when going "idle".
+ */
+static void uv_idle(int state)
+{
+	if (state)
+		uv_set_led_bits(0, LED_CPU_ACTIVITY);
+
+	else
+		uv_set_led_bits(LED_CPU_ACTIVITY, LED_CPU_ACTIVITY);
+}
+
 void __init uv_setup(char **cmdline_p)
 {
 	union uvh_si_addr_map_config_u m_n_config;
@@ -104,9 +117,11 @@ void __init uv_setup(char **cmdline_p)
 		uv_cpu_hub_info(cpu)->gnode_upper = gnode_upper;
 		uv_cpu_hub_info(cpu)->global_mmr_base = mmr_base;
 		uv_cpu_hub_info(cpu)->coherency_domain_number = 0;/* ZZZ */
-		uv_cpu_hub_info(cpu)->led_offset = LED_LOCAL_MMR_BASE + lcpu;
+		uv_cpu_hub_info(cpu)->led_offset = LED_LOCAL_MMR_BASE + cpu;
 		uv_cpu_hub_info(cpu)->led_state = 0;
 		printk(KERN_DEBUG "UV cpu %d, nid %d\n", cpu, nid);
 	}
-}
 
+	/* enable idle callback */
+	ia64_mark_idle = &uv_idle;
+}
--- linux-2.6.tip.orig/include/asm-ia64/uv/uv_hub.h
+++ linux-2.6.tip/include/asm-ia64/uv/uv_hub.h
@@ -150,6 +150,9 @@ DECLARE_PER_CPU(struct uv_hub_info_s, __
 #define LED_CPU_BLINK		0xffff	/* blink led */
 #define LED_CPU_HB_INTERVAL	(HZ/2)	/* blink once per second */
 
+/* idle callback */
+extern void (*ia64_mark_idle) (int);
+
 /*
  * Macros for converting between kernel virtual addresses, socket local physical
  * addresses, and UV global physical addresses.

-- 

[PATCH 6/6] ia64 UV: Use blinking LED for heartbeat display

[Mike Travis]

[-- Attachment #1: uv_led_display_hb_ia64 --]
[-- Type: text/plain, Size: 2674 bytes --]

  * Add a function to blink the "heartbeat" LED indicating
    that the CPU is responding to timer interrupts.

  * Introduces a callback for "heartbeat" display in the
    clocksource_watchdog function, which in turn requires
    CONFIG_CLOCKSOURCE_WATCHDOG to be set.

Note that this is a RAS feature that allows external monitoring of
various cpu state indicators, not just providing "pretty blinking
lights", as the LED state is readable by the system controller.

Based on linux-2.6.tip/master.

Signed-off-by: Mike Travis <travis@sgi.com>
Cc: Tony Luck <tony.luck@intel.com>
---
 arch/ia64/uv/kernel/setup.c  |   28 ++++++++++++++++++++++++++++
 include/asm-ia64/uv/uv_hub.h |    4 ++++
 2 files changed, 32 insertions(+)

--- linux-2.6.tip.orig/arch/ia64/uv/kernel/setup.c
+++ linux-2.6.tip/arch/ia64/uv/kernel/setup.c
@@ -8,6 +8,7 @@
  * Copyright (C) 2008 Silicon Graphics, Inc. All rights reserved.
  */
 
+#include <linux/clocksource.h>
 #include <linux/module.h>
 #include <linux/percpu.h>
 #include <asm/sn/simulator.h>
@@ -65,6 +66,25 @@ static void uv_idle(int state)
 		uv_set_led_bits(LED_CPU_ACTIVITY, LED_CPU_ACTIVITY);
 }
 
+#ifdef CONFIG_CLOCKSOURCE_WATCHDOG
+static void uv_display_heartbeat(void)
+{
+	int cpu;
+
+	uv_hub_info->uv_heartbeat = nr_cpu_ids;
+
+	for_each_online_cpu(cpu) {
+		struct uv_hub_info_s *hub = uv_cpu_hub_info(cpu);
+
+		if (hub->led_heartbeat_count > 0) {
+			uv_set_led_bits_on(cpu, LED_CPU_BLINK,
+						LED_CPU_HEARTBEAT);
+			--hub->led_heartbeat_count;
+		}
+	}
+}
+#endif
+
 void __init uv_setup(char **cmdline_p)
 {
 	union uvh_si_addr_map_config_u m_n_config;
@@ -124,4 +144,12 @@ void __init uv_setup(char **cmdline_p)
 
 	/* enable idle callback */
 	ia64_mark_idle = &uv_idle;
+
+#ifdef CONFIG_CLOCKSOURCE_WATCHDOG
+	/* enable heartbeat display callback */
+	display_heartbeat = uv_display_heartbeat;
+#else
+	printk(KERN_NOTICE "UV: CLOCKSOURCE_WATCHDOG not configured, "
+			   "LED Heartbeat NOT enabled\n");
+#endif
 }
--- linux-2.6.tip.orig/include/asm-ia64/uv/uv_hub.h
+++ linux-2.6.tip/include/asm-ia64/uv/uv_hub.h
@@ -100,6 +100,7 @@ struct uv_hub_info_s {
 	unsigned long	lowmem_remap_top;
 	unsigned long	lowmem_remap_base;
 	unsigned long	led_offset;
+	unsigned int	led_heartbeat_count;
 	unsigned short	pnode;
 	unsigned short	pnode_mask;
 	unsigned short	coherency_domain_number;
@@ -153,6 +154,9 @@ DECLARE_PER_CPU(struct uv_hub_info_s, __
 /* idle callback */
 extern void (*ia64_mark_idle) (int);
 
+/* idle callback */
+extern void (*ia64_mark_idle) (int);
+
 /*
  * Macros for converting between kernel virtual addresses, socket local physical
  * addresses, and UV global physical addresses.

--