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

[Mike Travis <travis@sgi.com>]

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