| Message ID | 20250117040132.3237808-1-keyz@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | cpuidle: psci: Add trace for PSCI domain idle | expand |
On Fri, 17 Jan 2025 12:01:32 +0800 Keita Morisaki <keyz@google.com> wrote: > @@ -74,7 +75,9 @@ static __cpuidle int __psci_enter_domain_idle_state(struct cpuidle_device *dev, > if (!state) > state = states[idx]; > > + trace_psci_domain_idle(dev->cpu, state, true, s2idle); > ret = psci_cpu_suspend_enter(state) ? -1 : idx; > + trace_psci_domain_idle(dev->cpu, state, false, s2idle); Why not make that into two different events: + trace_psci_domain_idle_enter(dev->cpu, state, s2idle); ret = psci_cpu_suspend_enter(state) ? -1 : idx; + trace_psci_domain_idle_exit(dev->cpu, state, s2idle); > > if (s2idle) > dev_pm_genpd_resume(pd_dev); > diff --git a/include/trace/events/power.h b/include/trace/events/power.h > index d2349b6b531a..82ad8bb1c477 100644 > --- a/include/trace/events/power.h > +++ b/include/trace/events/power.h > @@ -62,6 +62,31 @@ TRACE_EVENT(cpu_idle_miss, > (unsigned long)__entry->state, (__entry->below)?"below":"above") > ); > > +TRACE_EVENT(psci_domain_idle, > + > + TP_PROTO(unsigned int cpu_id, unsigned int state, bool enter, bool s2idle), > + > + TP_ARGS(cpu_id, state, enter, s2idle), > + > + TP_STRUCT__entry( > + __field(u32, cpu_id) > + __field(u32, state) > + __field(bool, enter) > + __field(bool, s2idle) > + ), > + > + TP_fast_assign( > + __entry->cpu_id = cpu_id; > + __entry->state = state; > + __entry->enter = enter; > + __entry->s2idle = s2idle; > + ), > + > + TP_printk("cpu_id=%lu state=0x%lx type=%s, is_s2idle=%s", > + (unsigned long)__entry->cpu_id, (unsigned long)__entry->state, > + (__entry->enter)?"enter":"exit", (__entry->s2idle)?"yes":"no") > +); > + Then make the above into a DECLARE_EVENT_CLASS: DECLARE_EVENT_CLASS(psci_domain_idle_template, TP_PROTO(unsigned int cpu_id, unsigned int state, bool s2idle), TP_ARGS(cpu_id, state, s2idle), TP_STRUCT__entry( __field(u32, cpu_id) __field(u32, state) __field(bool, s2idle) ), TP_fast_assign( __entry->cpu_id = cpu_id; __entry->state = state; __entry->s2idle = s2idle; ), TP_printk("cpu_id=%lu state=0x%lx type=%s, is_s2idle=%s", (unsigned long)__entry->cpu_id, (unsigned long)__entry->state, (__entry->s2idle)?"yes":"no") ); DEFINE_EVENT(psci_domain_idle_template, psci_domain_idle_enter, TP_PROTO(unsigned int cpu_id, unsigned int state, bool s2idle), TP_ARGS(cpu_id, state, s2idle), ); DEFINE_EVENT(psci_domain_idle_template, psci_domain_idle_exit, TP_PROTO(unsigned int cpu_id, unsigned int state, bool s2idle), TP_ARGS(cpu_id, state, s2idle), ); And then you could easily attach synthetic events to them to get the timings and such. -- Steve
Thank you for the review! > Why not make that into two different events: > + trace_psci_domain_idle_enter(dev->cpu, state, s2idle); > ret = psci_cpu_suspend_enter(state) ? -1 : idx; > + trace_psci_domain_idle_exit(dev->cpu, state, s2idle); > Then make the above into a DECLARE_EVENT_CLASS: > DECLARE_EVENT_CLASS(psci_domain_idle_template, > TP_PROTO(unsigned int cpu_id, unsigned int state, bool s2idle), > TP_ARGS(cpu_id, state, s2idle), > TP_STRUCT__entry( > __field(u32, cpu_id) > __field(u32, state) > __field(bool, s2idle) > ), > TP_fast_assign( > __entry->cpu_id = cpu_id; > __entry->state = state; > __entry->s2idle = s2idle; > ), > TP_printk("cpu_id=%lu state=0x%lx type=%s, is_s2idle=%s", > (unsigned long)__entry->cpu_id, (unsigned long)__entry->state, > (__entry->s2idle)?"yes":"no") > ); > DEFINE_EVENT(psci_domain_idle_template, psci_domain_idle_enter, > TP_PROTO(unsigned int cpu_id, unsigned int state, bool s2idle), > TP_ARGS(cpu_id, state, s2idle), > ); > DEFINE_EVENT(psci_domain_idle_template, psci_domain_idle_exit, > TP_PROTO(unsigned int cpu_id, unsigned int state, bool s2idle), > TP_ARGS(cpu_id, state, s2idle), > ); Looks good. Let me apply this change in v2. Thanks, Keita
diff --git a/drivers/cpuidle/cpuidle-psci.c b/drivers/cpuidle/cpuidle-psci.c index 2562dc001fc1..5888d4511787 100644 --- a/drivers/cpuidle/cpuidle-psci.c +++ b/drivers/cpuidle/cpuidle-psci.c @@ -25,6 +25,7 @@ #include <linux/syscore_ops.h> #include <asm/cpuidle.h> +#include <trace/events/power.h> #include "cpuidle-psci.h" #include "dt_idle_states.h" @@ -74,7 +75,9 @@ static __cpuidle int __psci_enter_domain_idle_state(struct cpuidle_device *dev, if (!state) state = states[idx]; + trace_psci_domain_idle(dev->cpu, state, true, s2idle); ret = psci_cpu_suspend_enter(state) ? -1 : idx; + trace_psci_domain_idle(dev->cpu, state, false, s2idle); if (s2idle) dev_pm_genpd_resume(pd_dev); diff --git a/include/trace/events/power.h b/include/trace/events/power.h index d2349b6b531a..82ad8bb1c477 100644 --- a/include/trace/events/power.h +++ b/include/trace/events/power.h @@ -62,6 +62,31 @@ TRACE_EVENT(cpu_idle_miss, (unsigned long)__entry->state, (__entry->below)?"below":"above") ); +TRACE_EVENT(psci_domain_idle, + + TP_PROTO(unsigned int cpu_id, unsigned int state, bool enter, bool s2idle), + + TP_ARGS(cpu_id, state, enter, s2idle), + + TP_STRUCT__entry( + __field(u32, cpu_id) + __field(u32, state) + __field(bool, enter) + __field(bool, s2idle) + ), + + TP_fast_assign( + __entry->cpu_id = cpu_id; + __entry->state = state; + __entry->enter = enter; + __entry->s2idle = s2idle; + ), + + TP_printk("cpu_id=%lu state=0x%lx type=%s, is_s2idle=%s", + (unsigned long)__entry->cpu_id, (unsigned long)__entry->state, + (__entry->enter)?"enter":"exit", (__entry->s2idle)?"yes":"no") +); + TRACE_EVENT(powernv_throttle, TP_PROTO(int chip_id, const char *reason, int pmax),
The trace event cpu_idle provides insufficient information for debugging PSCI requests due to lacking access to determined PSCI domain idle states. The cpu_idle usually only shows -1, 0, or 1 regardless how many CPUidle states the power domain has. Add a new trace named psci_domain_idle with a determined idle state info before the state entry, and after the state exit. This new trace event will help developers debug CPUidle issues on ARM systems using PSCI by providing more detailed information about the requested idle states. Signed-off-by: Keita Morisaki <keyz@google.com> --- drivers/cpuidle/cpuidle-psci.c | 3 +++ include/trace/events/power.h | 25 +++++++++++++++++++++++++ 2 files changed, 28 insertions(+) base-commit: 9bffa1ad25b8b3b95d8f463e5c24dabe3c87d54d