Skip to contentSkip to conversation

CPU LED trigger: add hooks to generic code so that it works on x86

PATCH Pavel Machek 2 participants 6 messages May 27, 2015

Subsystems (2)
Scheduler LED
lore
Share & tools
lore

Pavel Machek

author
#0 lore raw

CPU LED trigger hooks are currently hidden in arm-specific code, which means that this trigger only works on arm. Add it to the generic code, so that it works on x86, too.

Signed-off-by Pavel Machek <[email protected]>
kernel/sched/idle.c +3 -0
index fefcb1f..087acd6 100644
@@ -8,6 +8,7 @@
 #include <linux/mm.h>
 #include <linux/stackprotector.h>
 #include <linux/suspend.h>
+#include <linux/leds.h>
 
 #include <asm/tlb.h>
 

@@ -232,6 +233,7 @@ static void cpu_idle_loop(void)
 			}
 
 			local_irq_disable();
+			ledtrig_cpu(CPU_LED_IDLE_START);
 			arch_cpu_idle_enter();
 
 			/*

@@ -249,6 +251,7 @@ static void cpu_idle_loop(void)
 				cpuidle_idle_call();
 
 			arch_cpu_idle_exit();
+			ledtrig_cpu(CPU_LED_IDLE_END);
 		}
 
 		/*
Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
maintainer
#1 lore raw
Quoted 4 lines · Pavel Machek

CPU LED trigger hooks are currently hidden in arm-specific code, which means that this trigger only works on arm. Add it to the generic code, so that it works on x86, too.

And why do we want to go and slow down the idle loop for everyone?

author
#2 lore raw
Quoted 7 lines · Peter Zijlstra
Quoted 4 lines · Pavel Machek

CPU LED trigger hooks are currently hidden in arm-specific code, which means that this trigger only works on arm. Add it to the generic code, so that it works on x86, too.

And why do we want to go and slow down the idle loop for everyone?

If the trigger is not configured, this is not slowing down anyone.

If the trigger is configured, well, then the user requested the functionality. So we are not slowing down _everyone_, just the people that want the blinking.

Pavel

include/linux/leds.h:

#ifdef CONFIG_LEDS_TRIGGER_CPU
extern void ledtrig_cpu(enum cpu_led_event evt); #else
static inline void ledtrig_cpu(enum cpu_led_event evt)

{
	return;
	}

#endif

Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
maintainer
#3 lore raw
Quoted 7 lines · Pavel Machek
Quoted 6 lines · Peter Zijlstra
Quoted 4 lines · Pavel Machek

CPU LED trigger hooks are currently hidden in arm-specific code, which means that this trigger only works on arm. Add it to the generic code, so that it works on x86, too.

Quoted 7 lines
Quoted 1 line

And why do we want to go and slow down the idle loop for everyone?

If the trigger is not configured, this is not slowing down anyone.

If the trigger is configured, well, then the user requested the functionality. So we are not slowing down _everyone_, just the people that want the blinking.

What kind of x86 hardware will support this? If its _everything_ distros will have to enable this and it suddenly becomes a lot of people.

author
#4 lore raw
Quoted 10 lines · Peter Zijlstra
Quoted 9 lines · Pavel Machek
Quoted 7 lines · Peter Zijlstra
Quoted 4 lines · Pavel Machek

CPU LED trigger hooks are currently hidden in arm-specific code, which means that this trigger only works on arm. Add it to the generic code, so that it works on x86, too.

And why do we want to go and slow down the idle loop for everyone?

Quoted 8 lines
Quoted 5 lines

If the trigger is not configured, this is not slowing down anyone.

If the trigger is configured, well, then the user requested the functionality. So we are not slowing down _everyone_, just the people that want the blinking.

What kind of x86 hardware will support this? If its _everything_ distros will have to enable this and it suddenly becomes a lot of people.

It needs directly connected leds. It is useful on thinkpad t40p, for example.

Distros should not enable this, IMO. But I don't think overhead is that big in either case.

But I wanted to enable it, and result was trigger that can be configured, enabled, and just plain does not work (which is nasty).
Pavel

Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
maintainer
#5 lore raw
Quoted 3 lines · Pavel Machek

Distros should not enable this, IMO. But I don't think overhead is that big in either case.

Coming back from idle there might not be any cache, and you're adding at least 1 more cache miss for loading cpu_trig -- those hurt. Looking at that code, in case of !trig, you do a pointless function call before bailing.

The thing is, there already is too much code in the wake from idle path, adding more is not my favourite thing.

One thing you could do is wrap the call to ledtrig_cpu() in a static_key, that way all we do is add some i$ pollution, but at least we avoid the data cache miss when there's nothing to do.