[PATCH] PM: trace events for suspend/resume

[PATCH] PM: trace events for suspend/resume

[Todd E Brandt]

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);
 }
 
 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);

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

[Rafael J. Wysocki]

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?

>  
>  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);
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

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

[Todd E Brandt]

On Thu, May 22, 2014 at 02:01:44AM +0200, 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. :-)

Yea but it really helps you diagnose bad firmware during suspend/resume
(for instance when there's just an arbitrary sleep 100ms in the ACPI code).
But it doesn't matter for general performance testing, I'll remove it on v2
submit.

> 
> The rest of the patch looks OK to me.
> 
> Steven, what about the tracing angle?
> 
> >  
> >  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);
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> -- 
> I speak only for myself.
> Rafael J. Wysocki, Intel Open Source Technology Center.

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

[Srivatsa S. Bhat]

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?

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);

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

[Steven Rostedt]

On Thu, 2014-05-22 at 10:07 -0700, Todd E Brandt wrote:
>  
> > >  #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. :-)
> 
> Yea but it really helps you diagnose bad firmware during suspend/resume
> (for instance when there's just an arbitrary sleep 100ms in the ACPI code).
> But it doesn't matter for general performance testing, I'll remove it on v2
> submit.
> 
> > 
> > The rest of the patch looks OK to me.
> > 
> > Steven, what about the tracing angle?

It should be under an ACPI system (if there is one). Looks rather silly
to call it for suspend.

-- Steve

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

[Steven Rostedt]

On Fri, 2014-05-23 at 03:46 +0530, Srivatsa S. Bhat wrote:
> 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?

ftrace_start/stop() only affects the function tracer and not the
tracepoints. Those will still be recorded.

Note, we have an invested interest in getting rid of
ftrace_stop/start(). I rather mark all the suspend code with notrace or
find another method to protect suspend from function tracing. The
ftrace_stop/start() was more of a big hammer approach and I had plans
for getting rid of it in the future. That future is approaching fast :-)

-- Steve

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

[Rafael J. Wysocki]

On Thursday, May 22, 2014 06:41:45 PM Steven Rostedt wrote:
> On Thu, 2014-05-22 at 10:07 -0700, Todd E Brandt wrote:
> >  
> > > >  #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. :-)
> > 
> > Yea but it really helps you diagnose bad firmware during suspend/resume
> > (for instance when there's just an arbitrary sleep 100ms in the ACPI code).
> > But it doesn't matter for general performance testing, I'll remove it on v2
> > submit.
> > 
> > > 
> > > The rest of the patch looks OK to me.
> > > 
> > > Steven, what about the tracing angle?
> 
> It should be under an ACPI system (if there is one). Looks rather silly
> to call it for suspend.

I meant the rest of the patch, not this particular hunk. :-)

Rafael

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

[Todd E Brandt]

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);
> 
> 

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

[Srivatsa S. Bhat]

On 05/23/2014 04:14 AM, Steven Rostedt wrote:
> On Fri, 2014-05-23 at 03:46 +0530, Srivatsa S. Bhat wrote:
>> 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?
> 
> ftrace_start/stop() only affects the function tracer and not the
> tracepoints. Those will still be recorded.
> 

Ah, thank you for the clarification.

> Note, we have an invested interest in getting rid of
> ftrace_stop/start(). I rather mark all the suspend code with notrace or
> find another method to protect suspend from function tracing. The
> ftrace_stop/start() was more of a big hammer approach and I had plans
> for getting rid of it in the future. That future is approaching fast :-)
>

Ok.. :-)

 
Regards,
Srivatsa S. Bhat

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

[Srivatsa S. Bhat]

On 05/23/2014 05:20 AM, Todd E Brandt wrote:
> 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.
> 

Cool, thanks!

Regards,
Srivatsa S. Bhat

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

[Todd E Brandt]

Hi guys, I'm not hearing any objections to revamping the machine_suspend trace
event, so I'll resubmit a patch with it removed and supplanted with the
new suspend_resume event. I've also discovered that this first patch doesn't
completely cover freeze, so I'm moving a couple of the calls around.

On Fri, May 23, 2014 at 01:06:32AM +0200, Rafael J. Wysocki wrote:
> On Thursday, May 22, 2014 06:41:45 PM Steven Rostedt wrote:
> > On Thu, 2014-05-22 at 10:07 -0700, Todd E Brandt wrote:
> > >  
> > > > >  #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. :-)
> > > 
> > > Yea but it really helps you diagnose bad firmware during suspend/resume
> > > (for instance when there's just an arbitrary sleep 100ms in the ACPI code).
> > > But it doesn't matter for general performance testing, I'll remove it on v2
> > > submit.
> > > 
> > > > 
> > > > The rest of the patch looks OK to me.
> > > > 
> > > > Steven, what about the tracing angle?
> > 
> > It should be under an ACPI system (if there is one). Looks rather silly
> > to call it for suspend.
> 
> I meant the rest of the patch, not this particular hunk. :-)
> 
> Rafael
>