Re: [PATCH] PM: trace events for suspend/resume

[Todd E Brandt <todd.e.brandt@linux.intel.com>]

On Fri, May 23, 2014 at 03:46:54AM +0530, Srivatsa S. Bhat wrote:
> 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 <todd.e.brandt@intel.com>
> >> ---
> >>  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 <linux/mm.h>
> >>  #include <linux/slab.h>
> >>  #include <linux/acpi.h>
> >> +#include <trace/events/power.h>
> >>  
> >>  #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 <linux/list.h>
> >>  #include <linux/jiffies.h>
> >>  #include <linux/semaphore.h>
> >> +#include <trace/events/power.h>
> >>  
> >>  #include <asm/io.h>
> >>  #include <asm/uaccess.h>
> >> @@ -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?

I put a patch in the kernel a few months back that allowed ftrace to
function all the way through to disable/enable_nonboot_cpus. So the
actual trace code includes everything up to the actual CPU suspend/resume.
And also (as Steven mentioned), the tracepoint activity isn't controlled
via ftrace_start/ftrace_stop, those are for tracer based logging.

> 
> 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 <linux/mutex.h>
> >>  #include <linux/module.h>
> >>  #include <linux/interrupt.h>
> >> +#include <trace/events/power.h>
> >>  
> >>  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 <linux/gfp.h>
> >>  #include <linux/suspend.h>
> >>  #include <linux/lockdep.h>
> >> +#include <trace/events/power.h>
> >>  
> >>  #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 <linux/delay.h>
> >>  #include <linux/workqueue.h>
> >>  #include <linux/kmod.h>
> >> +#include <trace/events/power.h>
> >>  
> >>  /* 
> >>   * 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);
> 
>