Messages in this thread | | | Date | Mon, 25 Aug 2008 10:49:21 -0700 | From | Mike Travis <> | Subject | Re: [PATCH 2/6] x86_64 UV: Use LED to indicate CPU is active |
| |
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
| |