From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Srivatsa S. Bhat" Subject: Re: [PATCH] PM: trace events for suspend/resume Date: Fri, 23 May 2014 03:46:54 +0530 Message-ID: <537E7756.1000503@linux.vnet.ibm.com> References: <20140519230226.GA14382@linux.intel.com> <2794431.xZpr01emRq@vostro.rjw.lan> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Return-path: Received: from e23smtp01.au.ibm.com ([202.81.31.143]:45243 "EHLO e23smtp01.au.ibm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750923AbaEVWSJ (ORCPT ); Thu, 22 May 2014 18:18:09 -0400 Received: from /spool/local by e23smtp01.au.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Fri, 23 May 2014 08:18:06 +1000 Received: from d23relay04.au.ibm.com (d23relay04.au.ibm.com [9.190.234.120]) by d23dlp02.au.ibm.com (Postfix) with ESMTP id EEEDF2BB004A for ; Fri, 23 May 2014 08:18:01 +1000 (EST) Received: from d23av01.au.ibm.com (d23av01.au.ibm.com [9.190.234.96]) by d23relay04.au.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id s4MLuXK652756654 for ; Fri, 23 May 2014 07:56:33 +1000 Received: from d23av01.au.ibm.com (localhost [127.0.0.1]) by d23av01.au.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id s4MMI1nk015983 for ; Fri, 23 May 2014 08:18:01 +1000 In-Reply-To: <2794431.xZpr01emRq@vostro.rjw.lan> Sender: linux-pm-owner@vger.kernel.org List-Id: linux-pm@vger.kernel.org To: "Rafael J. Wysocki" , todd.e.brandt@linux.intel.com, Steven Rostedt Cc: linux-pm@vger.kernel.org, rafael.j.wysocki@intel.com On 05/22/2014 05:31 AM, Rafael J. Wysocki wrote: > On Monday, May 19, 2014 04:02:26 PM Todd E Brandt wrote: >> Adds trace events that give finer resolution into suspend/resume. These >> events are graphed in the timelines generated by the analyze_suspend.py >> script. They represent large areas of time consumed that are typical in >> suspend and resume. >> >> The event is triggered by calling the function "trace_suspend_resume" >> with two arguments: a string (the name of the event to be displayed >> in the timeline), and a boolean (where true is used to denote the start >> of the timeline event, and false to denote the end). >> >> Signed-off-by: Todd Brandt >> --- >> drivers/acpi/nvs.c | 6 ++++++ >> drivers/acpi/osl.c | 3 +++ >> drivers/base/syscore.c | 5 +++++ >> include/trace/events/power.h | 19 +++++++++++++++++++ >> kernel/cpu.c | 9 +++++++++ >> kernel/power/process.c | 3 +++ >> kernel/power/suspend.c | 4 ++++ >> 7 files changed, 49 insertions(+) >> >> For more info see: >> https://01.org/suspendresume/blogs/tebrandt/2014/custom-timeline-event-support-0 >> >> I'm also interested to know if any tools depend on the machine_suspend >> tracepoints, as this patch expands on the same info. I'd be willing to >> merge the existing machine_suspend calls into this patch and rename >> everything to suspend_resume provided it doesn't hurt any downstream tools. >> >> diff --git a/drivers/acpi/nvs.c b/drivers/acpi/nvs.c >> index de4fe03..b52b686 100644 >> --- a/drivers/acpi/nvs.c >> +++ b/drivers/acpi/nvs.c >> @@ -12,6 +12,7 @@ >> #include >> #include >> #include >> +#include >> >> #include "internal.h" >> >> @@ -171,6 +172,7 @@ int suspend_nvs_save(void) >> { >> struct nvs_page *entry; >> >> + trace_suspend_resume("save_nvs_memory", true); >> printk(KERN_INFO "PM: Saving platform NVS memory\n"); >> >> list_for_each_entry(entry, &nvs_list, node) >> @@ -185,11 +187,13 @@ int suspend_nvs_save(void) >> } >> if (!entry->kaddr) { >> suspend_nvs_free(); >> + trace_suspend_resume("save_nvs_memory", false); >> return -ENOMEM; >> } >> memcpy(entry->data, entry->kaddr, entry->size); >> } >> >> + trace_suspend_resume("save_nvs_memory", false); >> return 0; >> } >> >> @@ -203,10 +207,12 @@ void suspend_nvs_restore(void) >> { >> struct nvs_page *entry; >> >> + trace_suspend_resume("restore_nvs_memory", true); >> printk(KERN_INFO "PM: Restoring platform NVS memory\n"); >> >> list_for_each_entry(entry, &nvs_list, node) >> if (entry->data) >> memcpy(entry->kaddr, entry->data, entry->size); >> + trace_suspend_resume("restore_nvs_memory", false); >> } >> #endif >> diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c >> index 6776c59..b65a77a 100644 >> --- a/drivers/acpi/osl.c >> +++ b/drivers/acpi/osl.c >> @@ -44,6 +44,7 @@ >> #include >> #include >> #include >> +#include >> >> #include >> #include >> @@ -835,7 +836,9 @@ acpi_status acpi_os_remove_interrupt_handler(u32 irq, acpi_osd_handler handler) >> >> void acpi_os_sleep(u64 ms) >> { >> + trace_suspend_resume("acpi_os_sleep", true); >> msleep(ms); >> + trace_suspend_resume("acpi_os_sleep", false); >> } > > This function doesn't have anything to do with system suspend/resume. > It is for waiting. :-) > > The rest of the patch looks OK to me. > > Steven, what about the tracing angle? > Hi Todd, This might be a silly question, but the suspend_enter() function invokes ftrace_stop() before suspending the machine, and restarts ftrace after resume. And your patch seems to instrument code further down (deeper) in the suspend/resume path (such as disable_nonboot_cpus() for example). Doesn't the ftrace stop/start pose any problem for this patch? Regards, Srivatsa S. Bhat >> >> void acpi_os_stall(u32 us) >> diff --git a/drivers/base/syscore.c b/drivers/base/syscore.c >> index e8d11b6..c2fd5d4 100644 >> --- a/drivers/base/syscore.c >> +++ b/drivers/base/syscore.c >> @@ -10,6 +10,7 @@ >> #include >> #include >> #include >> +#include >> >> static LIST_HEAD(syscore_ops_list); >> static DEFINE_MUTEX(syscore_ops_lock); >> @@ -49,6 +50,7 @@ int syscore_suspend(void) >> struct syscore_ops *ops; >> int ret = 0; >> >> + trace_suspend_resume("syscore_suspend", true); >> pr_debug("Checking wakeup interrupts\n"); >> >> /* Return error code if there are any wakeup interrupts pending. */ >> @@ -70,6 +72,7 @@ int syscore_suspend(void) >> "Interrupts enabled after %pF\n", ops->suspend); >> } >> >> + trace_suspend_resume("syscore_suspend", false); >> return 0; >> >> err_out: >> @@ -92,6 +95,7 @@ void syscore_resume(void) >> { >> struct syscore_ops *ops; >> >> + trace_suspend_resume("syscore_resume", true); >> WARN_ONCE(!irqs_disabled(), >> "Interrupts enabled before system core resume.\n"); >> >> @@ -103,6 +107,7 @@ void syscore_resume(void) >> WARN_ONCE(!irqs_disabled(), >> "Interrupts enabled after %pF\n", ops->resume); >> } >> + trace_suspend_resume("syscore_resume", false); >> } >> EXPORT_SYMBOL_GPL(syscore_resume); >> #endif /* CONFIG_PM_SLEEP */ >> diff --git a/include/trace/events/power.h b/include/trace/events/power.h >> index 9a7e08d..1d9fd76 100644 >> --- a/include/trace/events/power.h >> +++ b/include/trace/events/power.h >> @@ -151,6 +151,25 @@ TRACE_EVENT(device_pm_report_time, >> __entry->ops_time, __entry->error) >> ); >> >> +TRACE_EVENT(suspend_resume, >> + >> + TP_PROTO(char *action, bool start), >> + >> + TP_ARGS(action, start), >> + >> + TP_STRUCT__entry( >> + __string(action, action) >> + __field(bool, start) >> + ), >> + >> + TP_fast_assign( >> + __assign_str(action, action); >> + __entry->start = start; >> + ), >> + >> + TP_printk("%s %s", __get_str(action), (__entry->start)?"begin":"end") >> +); >> + >> DECLARE_EVENT_CLASS(wakeup_source, >> >> TP_PROTO(const char *name, unsigned int state), >> diff --git a/kernel/cpu.c b/kernel/cpu.c >> index a9e710e..7aa30ec 100644 >> --- a/kernel/cpu.c >> +++ b/kernel/cpu.c >> @@ -20,6 +20,7 @@ >> #include >> #include >> #include >> +#include >> >> #include "smpboot.h" >> >> @@ -509,6 +510,7 @@ static cpumask_var_t frozen_cpus; >> int disable_nonboot_cpus(void) >> { >> int cpu, first_cpu, error = 0; >> + char buf[12]; >> >> cpu_maps_update_begin(); >> first_cpu = cpumask_first(cpu_online_mask); >> @@ -522,7 +524,10 @@ int disable_nonboot_cpus(void) >> for_each_online_cpu(cpu) { >> if (cpu == first_cpu) >> continue; >> + sprintf(buf, "CPU%u_OFF", cpu); >> + trace_suspend_resume(buf, true); >> error = _cpu_down(cpu, 1); >> + trace_suspend_resume(buf, false); >> if (!error) >> cpumask_set_cpu(cpu, frozen_cpus); >> else { >> @@ -554,6 +559,7 @@ void __weak arch_enable_nonboot_cpus_end(void) >> void __ref enable_nonboot_cpus(void) >> { >> int cpu, error; >> + char buf[10]; >> >> /* Allow everyone to use the CPU hotplug again */ >> cpu_maps_update_begin(); >> @@ -566,7 +572,10 @@ void __ref enable_nonboot_cpus(void) >> arch_enable_nonboot_cpus_begin(); >> >> for_each_cpu(cpu, frozen_cpus) { >> + sprintf(buf, "CPU%u_ON", cpu); >> + trace_suspend_resume(buf, true); >> error = _cpu_up(cpu, 1); >> + trace_suspend_resume(buf, false); >> if (!error) { >> printk(KERN_INFO "CPU%d is up\n", cpu); >> continue; >> diff --git a/kernel/power/process.c b/kernel/power/process.c >> index 06ec886..4aec2d9 100644 >> --- a/kernel/power/process.c >> +++ b/kernel/power/process.c >> @@ -17,6 +17,7 @@ >> #include >> #include >> #include >> +#include >> >> /* >> * Timeout for stopping processes >> @@ -175,6 +176,7 @@ void thaw_processes(void) >> struct task_struct *g, *p; >> struct task_struct *curr = current; >> >> + trace_suspend_resume("thaw_processes", true); >> if (pm_freezing) >> atomic_dec(&system_freezing_cnt); >> pm_freezing = false; >> @@ -201,6 +203,7 @@ void thaw_processes(void) >> >> schedule(); >> printk("done.\n"); >> + trace_suspend_resume("thaw_processes", false); >> } >> >> void thaw_kernel_threads(void) >> diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c >> index 8233cd4..c7e3e9f 100644 >> --- a/kernel/power/suspend.c >> +++ b/kernel/power/suspend.c >> @@ -147,7 +147,9 @@ static int suspend_prepare(suspend_state_t state) >> if (error) >> goto Finish; >> >> + trace_suspend_resume("freeze_processes", true); >> error = suspend_freeze_processes(); >> + trace_suspend_resume("freeze_processes", false); >> if (!error) >> return 0; >> >> @@ -337,9 +339,11 @@ static int enter_state(suspend_state_t state) >> if (state == PM_SUSPEND_FREEZE) >> freeze_begin(); >> >> + trace_suspend_resume("sync_filesystems", true); >> printk(KERN_INFO "PM: Syncing filesystems ... "); >> sys_sync(); >> printk("done.\n"); >> + trace_suspend_resume("sync_filesystems", false); >> >> pr_debug("PM: Preparing system for %s sleep\n", pm_states[state]); >> error = suspend_prepare(state);