Messages in this thread Patch in this message | | | Date | Fri, 11 Jun 2010 12:26:47 +0200 | From | Ronny Tschüter <> | Subject | Re: Tracing of power:power_start events doesn't work |
| |
On 12/05/2010 10:57, Robert Schöne wrote: > On Wed, den 05.05.2010-05-05, 07:33 -0700 schrieb Arjan van de Ven: > >> On 5/5/2010 7:31, Steven Rostedt wrote: >> >>> On Wed, 2010-05-05 at 16:11 +0200, Ronny Tsch�ter wrote: >>> >>> >>>> /* >>>> * Include the apic definitions for x86 to have the APIC timer related >>>> defines >>>> @@ -796,6 +797,18 @@ static int acpi_idle_bm_check(void) >>>> */ >>>> static inline void acpi_idle_do_entry(struct acpi_processor_cx *cx) >>>> { >>>> + switch (cx->type) { >>>> + case ACPI_STATE_C1: >>>> + trace_power_start(POWER_CSTATE, 1); >>>> + break; >>>> + case ACPI_STATE_C2: >>>> + trace_power_start(POWER_CSTATE, 2); >>>> + break; >>>> + case ACPI_STATE_C3: >>>> + trace_power_start(POWER_CSTATE, 3); >>>> + break; >>>> + } >>>> + >>>> >>> Depending on gcc, the above can bloat the code since each call to >>> trace_power_start() is a macro expanded. Try to call it just once. >>> Perhaps one of the following: >>> >> the code is also incorrect fundamentally. >> You need to pass in the mwait value or equivalent; the ACPI STATE type is >> pretty much useless random garbage and should completely be ignored. >> > You're correct for your case (switching to c-state via monitor/mwait). > But the current implementation is broken too. It works only if your > kernel uses processor_idle AND monitor/mwait. For all other cases it > fails - may it be on Ronnys system, that uses IO port based C-states or > my system, that uses cpu_idle (not processor_idle). > > I'd suggest, that it should work for IO port based switches and could be > included in the syntactic block of that case. > Afaik, halt means that the processor goes to C1. So in this block, there > could be a power_start event too with 1 as new state. (Maybe there are > problems with AMDs C1E which would be reported as C1.) > > Robert > Unfortunately the bug still exists in kernel 2.6.34. On my system I'm not able to trace power:power_start events, because they are not reported. Regarding to the preceding comments I moved my code snippet into the IO port based C-state part of acpi_idle_do_entry(). Therefore it should not interfere with the other two C-state calls in this function. Furthermore a lot of other code sequences in processor_idle.c use the ACPI STATE type in if- or switch-statements. So I don't think that ACPI STATE type is useless garbage.
Finally here is my new patch:
diff --git a/old/processor_idle.c b/new/processor_idle.c index 5939e7f..5818522 100644 --- a/old/processor_idle.c +++ b/new/processor_idle.c @@ -43,6 +43,7 @@ #include <linux/clockchips.h> #include <linux/cpuidle.h> #include <linux/irqflags.h> +#include <trace/events/power.h>
/* * Include the apic definitions for x86 to have the APIC timer related defines @@ -807,6 +808,10 @@ static inline void acpi_idle_do_entry(struct acpi_processor_cx *cx) } else { int unused; /* IO port based C-state */ + trace_power_start(POWER_CSTATE, + cx->type == ACPI_STATE_C1 ? 1 : + cx->type == ACPI_STATE_C2 ? 2 : + 3); inb(cx->address); /* Dummy wait op - must do something useless after P_LVL2 read because chipsets cannot guarantee that STPCLK# signal -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |