[PATCH 0/6] PM: Asynchronous suspend and resume of devices

[PATCH 0/6] PM: Asynchronous suspend and resume of devices

[Rafael J. Wysocki]

Hi,

The following patches introduce a mechanism allowing us to execute device
drivers' suspend and resume callbacks asynchronously during system sleep
transitions, such as suspend to RAM.

The idea is explained in the changelogs of the first two patches.

Comments welcome.

Thanks,
Rafael

[PATCH 1/6] PM: Introduce PM links framework

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rjw@sisk.pl>

Introduce a framework for representing off-tree PM dependencies
between devices.

There are PM dependencies between devices that are not reflected by
the structure of the device tree.  In other words, as far as PM is
concerned, a device may depend on some other devices which are not
its children and none of which is its parent.

Every such dependency involves two devices, one of which is a
"master" and the other of which is a "slave", meaning that the
"slave" have to be suspended before the "master" and cannot be
woken up before it.  Thus every device can be given two lists of
"dependency objects", one for the dependencies where the device is
the "master" and the other for the dependencies where the device is
the "slave".  Then, each "dependency object" can be represented as

struct pm_link {
    struct device *master;
    struct list_head master_hook;
    struct device *slave;
    struct list_head slave_hook;
};

Add some synchronization, helpers for adding and removing
'struct pm_link' objects.

In addition to checking a device's parent, walk the list of its
"masters", in addition to walking the list of a device's children,
walk the list of its "slaves".

'struct pm_link' objects are created automatically for ACPI devices
and "regular" devices associated with them.  In the other cases such
objects will have to be added directly by platforms / bus types /
drivers etc.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/acpi/glue.c          |    3 
 drivers/base/core.c          |    4 
 drivers/base/power/Makefile  |    2 
 drivers/base/power/common.c  |  279 +++++++++++++++++++++++++++++++++++++++++++
 drivers/base/power/main.c    |   27 ----
 drivers/base/power/power.h   |   31 ++--
 drivers/base/power/runtime.c |    4 
 include/linux/pm.h           |    4 
 include/linux/pm_link.h      |   30 ++++
 9 files changed, 342 insertions(+), 42 deletions(-)

Index: linux-2.6/include/linux/pm.h
===================================================================
--- linux-2.6.orig/include/linux/pm.h
+++ linux-2.6/include/linux/pm.h
@@ -408,6 +408,9 @@ enum rpm_request {
 };
 
 struct dev_pm_info {
+	spinlock_t		lock;
+	struct list_head	master_links;
+	struct list_head	slave_links;
 	pm_message_t		power_state;
 	unsigned int		can_wakeup:1;
 	unsigned int		should_wakeup:1;
@@ -420,7 +423,6 @@ struct dev_pm_info {
 	unsigned long		timer_expires;
 	struct work_struct	work;
 	wait_queue_head_t	wait_queue;
-	spinlock_t		lock;
 	atomic_t		usage_count;
 	atomic_t		child_count;
 	unsigned int		disable_depth:3;
Index: linux-2.6/drivers/base/power/power.h
===================================================================
--- linux-2.6.orig/drivers/base/power/power.h
+++ linux-2.6/drivers/base/power/power.h
@@ -1,3 +1,7 @@
+extern void device_pm_init(struct device *dev);
+extern void device_pm_add(struct device *dev);
+extern void device_pm_remove(struct device *dev);
+
 #ifdef CONFIG_PM_RUNTIME
 
 extern void pm_runtime_init(struct device *dev);
@@ -23,7 +27,8 @@ static inline struct device *to_device(s
 	return container_of(entry, struct device, power.entry);
 }
 
-extern void device_pm_init(struct device *dev);
+extern void device_pm_list_add(struct device *dev);
+extern void device_pm_list_remove(struct device *dev);
 extern void device_pm_add(struct device *);
 extern void device_pm_remove(struct device *);
 extern void device_pm_move_before(struct device *, struct device *);
@@ -32,17 +37,8 @@ extern void device_pm_move_last(struct d
 
 #else /* !CONFIG_PM_SLEEP */
 
-static inline void device_pm_init(struct device *dev)
-{
-	pm_runtime_init(dev);
-}
-
-static inline void device_pm_remove(struct device *dev)
-{
-	pm_runtime_remove(dev);
-}
-
-static inline void device_pm_add(struct device *dev) {}
+static inline void device_pm_list_add(struct device *dev) {}
+static inline void device_pm_list_remove(struct device *dev) {}
 static inline void device_pm_move_before(struct device *deva,
 					 struct device *devb) {}
 static inline void device_pm_move_after(struct device *deva,
@@ -60,7 +56,11 @@ static inline void device_pm_move_last(s
 extern int dpm_sysfs_add(struct device *);
 extern void dpm_sysfs_remove(struct device *);
 
-#else /* CONFIG_PM */
+/* drivers/base/power/link.c */
+extern int pm_link_init(void);
+extern void pm_link_remove_all(struct device *dev);
+
+#else /* !CONFIG_PM */
 
 static inline int dpm_sysfs_add(struct device *dev)
 {
@@ -71,4 +71,7 @@ static inline void dpm_sysfs_remove(stru
 {
 }
 
-#endif
+static inline int pm_link_init(void) { return 0; }
+static inline void pm_link_remove_all(struct device *dev) {}
+
+#endif  /* !CONFIG_PM */
Index: linux-2.6/drivers/base/core.c
===================================================================
--- linux-2.6.orig/drivers/base/core.c
+++ linux-2.6/drivers/base/core.c
@@ -1252,9 +1252,13 @@ int __init devices_init(void)
 	sysfs_dev_char_kobj = kobject_create_and_add("char", dev_kobj);
 	if (!sysfs_dev_char_kobj)
 		goto char_kobj_err;
+	if (pm_link_init())
+		goto pm_link_err;
 
 	return 0;
 
+ pm_link_err:
+	kobject_put(sysfs_dev_char_kobj);
  char_kobj_err:
 	kobject_put(sysfs_dev_block_kobj);
  block_kobj_err:
Index: linux-2.6/include/linux/pm_link.h
===================================================================
--- /dev/null
+++ linux-2.6/include/linux/pm_link.h
@@ -0,0 +1,30 @@
+/*
+ * include/linux/pm_link.h - PM links manipulation core.
+ *
+ * Copyright (c) 2009 Rafael J. Wysocki <rjw@sisk.pl>, Novell Inc.
+ *
+ * This file is released under the GPLv2.
+ */
+
+#ifndef _LINUX_PM_LINK_H
+#define _LINUX_PM_LINK_H
+
+#include <linux/list.h>
+
+struct device;
+
+struct pm_link {
+	struct device *master;
+	struct list_head master_hook;
+	struct device *slave;
+	struct list_head slave_hook;
+};
+
+extern int pm_link_add(struct device *slave, struct device *master);
+extern void pm_link_remove(struct device *dev, struct device *master);
+extern int device_for_each_master(struct device *slave, void *data,
+			   int (*fn)(struct device *dev, void *data));
+extern int device_for_each_slave(struct device *master, void *data,
+			  int (*fn)(struct device *dev, void *data));
+
+#endif
Index: linux-2.6/drivers/base/power/Makefile
===================================================================
--- linux-2.6.orig/drivers/base/power/Makefile
+++ linux-2.6/drivers/base/power/Makefile
@@ -1,4 +1,4 @@
-obj-$(CONFIG_PM)	+= sysfs.o
+obj-$(CONFIG_PM)	+= sysfs.o common.o
 obj-$(CONFIG_PM_SLEEP)	+= main.o
 obj-$(CONFIG_PM_RUNTIME)	+= runtime.o
 obj-$(CONFIG_PM_TRACE_RTC)	+= trace.o
Index: linux-2.6/drivers/base/power/runtime.c
===================================================================
--- linux-2.6.orig/drivers/base/power/runtime.c
+++ linux-2.6/drivers/base/power/runtime.c
@@ -972,8 +972,6 @@ EXPORT_SYMBOL_GPL(pm_runtime_enable);
  */
 void pm_runtime_init(struct device *dev)
 {
-	spin_lock_init(&dev->power.lock);
-
 	dev->power.runtime_status = RPM_SUSPENDED;
 	dev->power.idle_notification = false;
 
@@ -993,8 +991,6 @@ void pm_runtime_init(struct device *dev)
 	dev->power.timer_expires = 0;
 	setup_timer(&dev->power.suspend_timer, pm_suspend_timer_fn,
 			(unsigned long)dev);
-
-	init_waitqueue_head(&dev->power.wait_queue);
 }
 
 /**
Index: linux-2.6/drivers/base/power/main.c
===================================================================
--- linux-2.6.orig/drivers/base/power/main.c
+++ linux-2.6/drivers/base/power/main.c
@@ -21,6 +21,7 @@
 #include <linux/kallsyms.h>
 #include <linux/mutex.h>
 #include <linux/pm.h>
+#include <linux/pm_link.h>
 #include <linux/pm_runtime.h>
 #include <linux/resume-trace.h>
 #include <linux/rwsem.h>
@@ -50,16 +51,6 @@ static DEFINE_MUTEX(dpm_list_mtx);
 static bool transition_started;
 
 /**
- * device_pm_init - Initialize the PM-related part of a device object.
- * @dev: Device object being initialized.
- */
-void device_pm_init(struct device *dev)
-{
-	dev->power.status = DPM_ON;
-	pm_runtime_init(dev);
-}
-
-/**
  * device_pm_lock - Lock the list of active devices used by the PM core.
  */
 void device_pm_lock(void)
@@ -76,14 +67,11 @@ void device_pm_unlock(void)
 }
 
 /**
- * device_pm_add - Add a device to the PM core's list of active devices.
+ * device_pm_list_add - Add a device to the PM core's list of active devices.
  * @dev: Device to add to the list.
  */
-void device_pm_add(struct device *dev)
+void device_pm_list_add(struct device *dev)
 {
-	pr_debug("PM: Adding info for %s:%s\n",
-		 dev->bus ? dev->bus->name : "No Bus",
-		 kobject_name(&dev->kobj));
 	mutex_lock(&dpm_list_mtx);
 	if (dev->parent) {
 		if (dev->parent->power.status >= DPM_SUSPENDING)
@@ -97,24 +85,19 @@ void device_pm_add(struct device *dev)
 		 */
 		dev_WARN(dev, "Parentless device registered during a PM transaction\n");
 	}
-
 	list_add_tail(&dev->power.entry, &dpm_list);
 	mutex_unlock(&dpm_list_mtx);
 }
 
 /**
- * device_pm_remove - Remove a device from the PM core's list of active devices.
+ * device_pm_list_remove - Remove a device from the PM core's list of devices.
  * @dev: Device to be removed from the list.
  */
-void device_pm_remove(struct device *dev)
+void device_pm_list_remove(struct device *dev)
 {
-	pr_debug("PM: Removing info for %s:%s\n",
-		 dev->bus ? dev->bus->name : "No Bus",
-		 kobject_name(&dev->kobj));
 	mutex_lock(&dpm_list_mtx);
 	list_del_init(&dev->power.entry);
 	mutex_unlock(&dpm_list_mtx);
-	pm_runtime_remove(dev);
 }
 
 /**
Index: linux-2.6/drivers/base/power/common.c
===================================================================
--- /dev/null
+++ linux-2.6/drivers/base/power/common.c
@@ -0,0 +1,279 @@
+/*
+ * drivers/base/power/common.c - device PM common functions.
+ *
+ * Copyright (c) 2009 Rafael J. Wysocki <rjw@sisk.pl>, Novell Inc.
+ *
+ * This file is released under the GPLv2.
+ */
+
+#include <linux/rculist.h>
+#include <linux/device.h>
+#include <linux/srcu.h>
+#include <linux/pm_link.h>
+
+#include "power.h"
+
+/**
+ * device_pm_init - Initialize the PM part of a device object.
+ * @dev: Device object being initialized.
+ */
+void device_pm_init(struct device *dev)
+{
+	dev->power.status = DPM_ON;
+	spin_lock_init(&dev->power.lock);
+	INIT_LIST_HEAD(&dev->power.master_links);
+	INIT_LIST_HEAD(&dev->power.slave_links);
+	pm_runtime_init(dev);
+}
+
+/**
+ * device_pm_add - Handle the PM part of a device added to device tree.
+ * @dev: Device object being added to device tree.
+ */
+void device_pm_add(struct device *dev)
+{
+	pr_debug("PM: Adding info for %s:%s\n",
+		 dev->bus ? dev->bus->name : "No Bus",
+		 kobject_name(&dev->kobj));
+	device_pm_list_add(dev);
+}
+
+/**
+ * device_pm_remove - Handle the PM part of a device removed from device tree.
+ * @dev: Device object being removed from device tree.
+ */
+void device_pm_remove(struct device *dev)
+{
+	pr_debug("PM: Removing info for %s:%s\n",
+		 dev->bus ? dev->bus->name : "No Bus",
+		 kobject_name(&dev->kobj));
+	device_pm_list_remove(dev);
+	pm_runtime_remove(dev);
+	pm_link_remove_all(dev);
+}
+
+/*
+ * PM links framework.
+ *
+ * There are PM dependencies between devices that are not reflected by the
+ * structure of the device tree.  In other words, as far as PM is concerned, a
+ * device may depend on some other devices which are not its children and none
+ * of which is its parent.
+ *
+ * Every such dependency involves two devices, one of which is a "master" and
+ * the other of which is a "slave", meaning that the "slave" have to be
+ * suspended before the "master" and cannot be woken up before it.  Thus every
+ * device can be given two lists of "dependency objects", one for the
+ * dependencies where the device is the "master" and the other for the
+ * dependencies where the device is the "slave".  Then, each "dependency object"
+ * can be represented as 'struct pm_link' as defined in include/linux/pm_link.h.
+ *
+ * The PM links of a device can help decide when the device should be suspended
+ * or resumed.  Namely, In addition to checking the device's parent, the PM core
+ * can walk the list of its "masters" and check their PM status.  Similarly, in
+ * addition to walking the list of a device's children, the PM core can walk the
+ * list of its "slaves".
+ */
+
+static struct srcu_struct pm_link_ss;
+static DEFINE_MUTEX(pm_link_mtx);
+
+/**
+ * pm_link_add - Create a PM link object connecting two devices.
+ * @slave: Device to be the slave in this link.
+ * @master: Device to be the master in this link.
+ */
+int pm_link_add(struct device *slave, struct device *master)
+{
+	struct pm_link *link;
+	int error = -ENODEV;
+
+	if (!get_device(master))
+		return error;
+
+	if (!get_device(slave))
+		goto err_slave;
+
+	link = kzalloc(sizeof(*link), GFP_KERNEL);
+	if (!link)
+		goto err_link;
+
+	dev_dbg(slave, "PM: Creating PM link to (master) %s %s\n",
+		dev_driver_string(master), dev_name(master));
+
+	link->master = master;
+	INIT_LIST_HEAD(&link->master_hook);
+	link->slave = slave;
+	INIT_LIST_HEAD(&link->slave_hook);
+
+	spin_lock_irq(&master->power.lock);
+	list_add_tail_rcu(&link->master_hook, &master->power.master_links);
+	spin_unlock_irq(&master->power.lock);
+
+	spin_lock_irq(&slave->power.lock);
+	list_add_tail_rcu(&link->slave_hook, &slave->power.slave_links);
+	spin_unlock_irq(&slave->power.lock);
+
+	return 0;
+
+ err_link:
+	error = -ENOMEM;
+	put_device(slave);
+
+ err_slave:
+	put_device(master);
+
+	return error;
+}
+EXPORT_SYMBOL_GPL(pm_link_add);
+
+/**
+ * __pm_link_remove - Remove a PM link object.
+ * @link: PM link object to remove
+ */
+static void __pm_link_remove(struct pm_link *link)
+{
+	struct device *master = link->master;
+	struct device *slave = link->slave;
+
+	dev_dbg(slave, "PM: Removing PM link to (master) %s %s\n",
+		dev_driver_string(master), dev_name(master));
+
+	spin_lock_irq(&master->power.lock);
+	list_del_rcu(&link->master_hook);
+	spin_unlock_irq(&master->power.lock);
+
+	spin_lock_irq(&slave->power.lock);
+	list_del_rcu(&link->slave_hook);
+	spin_unlock_irq(&slave->power.lock);
+
+	synchronize_srcu(&pm_link_ss);
+
+	kfree(link);
+
+	put_device(master);
+	put_device(slave);
+}
+
+/**
+ * pm_link_remove_all - Remove all PM link objects for given device.
+ * @dev: Device to handle.
+ */
+void pm_link_remove_all(struct device *dev)
+{
+	struct pm_link *link, *n;
+
+	mutex_lock(&pm_link_mtx);
+
+	list_for_each_entry_safe(link, n, &dev->power.master_links, master_hook)
+		__pm_link_remove(link);
+
+	list_for_each_entry_safe(link, n, &dev->power.slave_links, slave_hook)
+		__pm_link_remove(link);
+
+	mutex_unlock(&pm_link_mtx);
+}
+
+/**
+ * pm_link_remove - Remove a PM link object connecting two devices.
+ * @dev: Slave device of the PM link to remove.
+ * @master: Master device of the PM link to remove.
+ */
+void pm_link_remove(struct device *dev, struct device *master)
+{
+	struct pm_link *link, *n;
+
+	mutex_lock(&pm_link_mtx);
+
+	list_for_each_entry_safe(link, n, &dev->power.slave_links, slave_hook) {
+		if (link->master != master)
+			continue;
+
+		__pm_link_remove(link);
+		break;
+	}
+
+	mutex_unlock(&pm_link_mtx);
+}
+EXPORT_SYMBOL_GPL(pm_link_remove);
+
+/**
+ * device_for_each_master - Execute given function for each master of a device.
+ * @slave: Device whose masters to execute the function for.
+ * @data: Data pointer to pass to the function.
+ * @fn: Function to execute for each master of @slave.
+ *
+ * The function is executed for the parent of the device, if there is one, and
+ * for each device connected to it via a pm_link object where @slave is the
+ * "slave".
+ */
+int device_for_each_master(struct device *slave, void *data,
+			   int (*fn)(struct device *dev, void *data))
+{
+	struct pm_link *link;
+	int idx;
+	int error = 0;
+
+	if (slave->parent) {
+		error = fn(slave->parent, data);
+		if (error)
+			return error;
+	}
+
+	idx = srcu_read_lock(&pm_link_ss);
+
+	list_for_each_entry(link, &slave->power.slave_links, slave_hook) {
+		struct device *master = link->master;
+
+		error = fn(master, data);
+		if (error)
+			break;
+	}
+
+	srcu_read_unlock(&pm_link_ss, idx);
+
+	return error;
+}
+EXPORT_SYMBOL_GPL(device_for_each_master);
+
+/**
+ * device_for_each_slave - Execute given function for each slave of a device.
+ * @master: Device whose slaves to execute the function for.
+ * @data: Data pointer to pass to the function.
+ * @fn: Function to execute for each slave of @master.
+ *
+ * The function is executed for all children of the device, if there are any,
+ * and for each device connected to it via a pm_link object where @master is the
+ * "master".
+ */
+int device_for_each_slave(struct device *master, void *data,
+			  int (*fn)(struct device *dev, void *data))
+{
+	struct pm_link *link;
+	int idx;
+	int error;
+
+	error = device_for_each_child(master, data, fn);
+	if (error)
+		return error;
+
+	idx = srcu_read_lock(&pm_link_ss);
+
+	list_for_each_entry(link, &master->power.master_links, master_hook) {
+		struct device *slave = link->slave;
+
+		error = fn(slave, data);
+		if (error)
+			break;
+	}
+
+	srcu_read_unlock(&pm_link_ss, idx);
+
+	return error;
+}
+EXPORT_SYMBOL_GPL(device_for_each_slave);
+
+int __init pm_link_init(void)
+{
+	return init_srcu_struct(&pm_link_ss);
+}
Index: linux-2.6/drivers/acpi/glue.c
===================================================================
--- linux-2.6.orig/drivers/acpi/glue.c
+++ linux-2.6/drivers/acpi/glue.c
@@ -11,6 +11,7 @@
 #include <linux/device.h>
 #include <linux/rwsem.h>
 #include <linux/acpi.h>
+#include <linux/pm_link.h>
 
 #define ACPI_GLUE_DEBUG	0
 #if ACPI_GLUE_DEBUG
@@ -170,6 +171,7 @@ static int acpi_bind_one(struct device *
 			device_set_wakeup_enable(dev,
 						acpi_dev->wakeup.state.enabled);
 		}
+		pm_link_add(dev, &acpi_dev->dev);
 	}
 
 	return 0;
@@ -189,6 +191,7 @@ static int acpi_unbind_one(struct device
 					&acpi_dev)) {
 			sysfs_remove_link(&dev->kobj, "firmware_node");
 			sysfs_remove_link(&acpi_dev->dev.kobj, "physical_node");
+			pm_link_remove(dev, &acpi_dev->dev);
 		}
 
 		acpi_detach_data(dev->archdata.acpi_handle,

Re: [PATCH 1/6] PM: Introduce PM links framework

[Alan Stern]

On Wed, 26 Aug 2009, Rafael J. Wysocki wrote:

> From: Rafael J. Wysocki <rjw@sisk.pl>
> 
> Introduce a framework for representing off-tree PM dependencies
> between devices.
> 
> There are PM dependencies between devices that are not reflected by
> the structure of the device tree.  In other words, as far as PM is
> concerned, a device may depend on some other devices which are not
> its children and none of which is its parent.
> 
> Every such dependency involves two devices, one of which is a
> "master" and the other of which is a "slave", meaning that the
> "slave" have to be suspended before the "master" and cannot be
> woken up before it.  Thus every device can be given two lists of
> "dependency objects", one for the dependencies where the device is
> the "master" and the other for the dependencies where the device is
> the "slave".  Then, each "dependency object" can be represented as


> Index: linux-2.6/drivers/base/power/common.c
> ===================================================================
> --- /dev/null
> +++ linux-2.6/drivers/base/power/common.c

> +/**
> + * device_for_each_master - Execute given function for each master of a device.
> + * @slave: Device whose masters to execute the function for.
> + * @data: Data pointer to pass to the function.
> + * @fn: Function to execute for each master of @slave.
> + *
> + * The function is executed for the parent of the device, if there is one, and
> + * for each device connected to it via a pm_link object where @slave is the
> + * "slave".
> + */
> +int device_for_each_master(struct device *slave, void *data,
> +			   int (*fn)(struct device *dev, void *data))
> +{
> +	struct pm_link *link;
> +	int idx;
> +	int error = 0;
> +
> +	if (slave->parent) {
> +		error = fn(slave->parent, data);
> +		if (error)
> +			return error;
> +	}
> +
> +	idx = srcu_read_lock(&pm_link_ss);
> +
> +	list_for_each_entry(link, &slave->power.slave_links, slave_hook) {

This needs to use list_for_each_entry_rcu.  Likewise in
device_for_each_slave().

Alan Stern

Re: [PATCH 1/6] PM: Introduce PM links framework

[Rafael J. Wysocki]

On Friday 28 August 2009, Alan Stern wrote:
> On Wed, 26 Aug 2009, Rafael J. Wysocki wrote:
> 
> > From: Rafael J. Wysocki <rjw@sisk.pl>
> > 
> > Introduce a framework for representing off-tree PM dependencies
> > between devices.
> > 
> > There are PM dependencies between devices that are not reflected by
> > the structure of the device tree.  In other words, as far as PM is
> > concerned, a device may depend on some other devices which are not
> > its children and none of which is its parent.
> > 
> > Every such dependency involves two devices, one of which is a
> > "master" and the other of which is a "slave", meaning that the
> > "slave" have to be suspended before the "master" and cannot be
> > woken up before it.  Thus every device can be given two lists of
> > "dependency objects", one for the dependencies where the device is
> > the "master" and the other for the dependencies where the device is
> > the "slave".  Then, each "dependency object" can be represented as
> 
> 
> > Index: linux-2.6/drivers/base/power/common.c
> > ===================================================================
> > --- /dev/null
> > +++ linux-2.6/drivers/base/power/common.c
> 
> > +/**
> > + * device_for_each_master - Execute given function for each master of a device.
> > + * @slave: Device whose masters to execute the function for.
> > + * @data: Data pointer to pass to the function.
> > + * @fn: Function to execute for each master of @slave.
> > + *
> > + * The function is executed for the parent of the device, if there is one, and
> > + * for each device connected to it via a pm_link object where @slave is the
> > + * "slave".
> > + */
> > +int device_for_each_master(struct device *slave, void *data,
> > +			   int (*fn)(struct device *dev, void *data))
> > +{
> > +	struct pm_link *link;
> > +	int idx;
> > +	int error = 0;
> > +
> > +	if (slave->parent) {
> > +		error = fn(slave->parent, data);
> > +		if (error)
> > +			return error;
> > +	}
> > +
> > +	idx = srcu_read_lock(&pm_link_ss);
> > +
> > +	list_for_each_entry(link, &slave->power.slave_links, slave_hook) {
> 
> This needs to use list_for_each_entry_rcu.  Likewise in
> device_for_each_slave().

OK, I wasn't sure about that.

Thanks,
Rafael

[PATCH 2/6] PM: Asynchronous resume of devices

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rjw@sisk.pl>

Theoretically, the total time of system sleep transitions (suspend
to RAM, hibernation) can be reduced by running suspend and resume
callbacks of device drivers in parallel with each other.  However,
there are dependencies between devices such that, for example, we may
not be allowed to put one device into a low power state before
anohter one has been suspended (e.g. we cannot suspend a bridge
before suspending all devices behind it).  In particular, we're not
allowed to suspend the parent of a device before suspending the
device itself.  Analogously, we're not allowed to resume a device
before resuming its parent.

Thus, to make it possible to execute suspend and resume callbacks
provided by device drivers in parallel with each other, we need to
provide a synchronization mechanism preventing the dependencies
between devices from being violated.

The patch below allows some devices to be resumed asynchronously,
with the following rules:
(1) There is a wait queue head, dev->power.wait_queue, and an
    "operation complete" flag, dev->power.op_complete for each device
    object.
(2) All of the power.op_complete flags are reset before suspend as
    well as after each resume stage (dpm_resume_noirq(),
    dpm_resume()).
(3) If power.async_suspend is set for dev or for one of devices it
    depends on, the PM core waits for the "master" device's
    power.op_complete flag to be set before attempting to run the
    resume callbacks, appropriate for this particular stage of
    resume, for dev.
(4) dev->power.op_complete is set for each device after running its
    resume callbacks at each stage of resume (dpm_resume_noirq(),
    dpm_resume()) and all threads waiting in the devices wait
    queue are woken up.

With this mechanism in place, the drivers wanting their resume
callbacks to be executed asynchronously can set
dev->power.async_suspend for them, with the help of
device_enable_async_suspend().  In addition to that, the PM off-tree
dependencies between devices have to be represented by
'struct pm_link' objects introduced by the previous patch.

In this version of the patch the async threads started to execute
the resume callbacks of specific device don't exit immediately having
done that, but search dpm_list for devices whose PM dependencies have
already been satisfied and execute their callbacks without waiting.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/base/power/common.c |    5 
 drivers/base/power/main.c   |  327 ++++++++++++++++++++++++++++++++++++++++++--
 include/linux/device.h      |    6 
 include/linux/pm.h          |    6 
 4 files changed, 329 insertions(+), 15 deletions(-)

Index: linux-2.6/include/linux/pm.h
===================================================================
--- linux-2.6.orig/include/linux/pm.h
+++ linux-2.6/include/linux/pm.h
@@ -26,6 +26,7 @@
 #include <linux/spinlock.h>
 #include <linux/wait.h>
 #include <linux/timer.h>
+#include <linux/completion.h>
 
 /*
  * Callbacks for platform drivers to implement.
@@ -409,20 +410,23 @@ enum rpm_request {
 
 struct dev_pm_info {
 	spinlock_t		lock;
+	wait_queue_head_t	wait_queue;
 	struct list_head	master_links;
 	struct list_head	slave_links;
 	pm_message_t		power_state;
 	unsigned int		can_wakeup:1;
 	unsigned int		should_wakeup:1;
+	unsigned int		async_suspend:1;
 	enum dpm_state		status;		/* Owned by the PM core */
 #ifdef CONFIG_PM_SLEEP
 	struct list_head	entry;
+	unsigned int		op_started:1;
+	unsigned int		op_complete:1;
 #endif
 #ifdef CONFIG_PM_RUNTIME
 	struct timer_list	suspend_timer;
 	unsigned long		timer_expires;
 	struct work_struct	work;
-	wait_queue_head_t	wait_queue;
 	atomic_t		usage_count;
 	atomic_t		child_count;
 	unsigned int		disable_depth:3;
Index: linux-2.6/include/linux/device.h
===================================================================
--- linux-2.6.orig/include/linux/device.h
+++ linux-2.6/include/linux/device.h
@@ -472,6 +472,12 @@ static inline int device_is_registered(s
 	return dev->kobj.state_in_sysfs;
 }
 
+static inline void device_enable_async_suspend(struct device *dev, bool enable)
+{
+	if (dev->power.status == DPM_ON)
+		dev->power.async_suspend = enable;
+}
+
 void driver_init(void);
 
 /*
Index: linux-2.6/drivers/base/power/main.c
===================================================================
--- linux-2.6.orig/drivers/base/power/main.c
+++ linux-2.6/drivers/base/power/main.c
@@ -26,6 +26,8 @@
 #include <linux/resume-trace.h>
 #include <linux/rwsem.h>
 #include <linux/interrupt.h>
+#include <linux/async.h>
+#include <linux/completion.h>
 
 #include "../base.h"
 #include "power.h"
@@ -43,6 +45,7 @@
 LIST_HEAD(dpm_list);
 
 static DEFINE_MUTEX(dpm_list_mtx);
+static pm_message_t pm_transition;
 
 /*
  * Set once the preparation of devices for a PM transition has started, reset
@@ -145,6 +148,131 @@ void device_pm_move_last(struct device *
 }
 
 /**
+ * dpm_reset - Clear power.op_started and power.op_complete for given device.
+ * @dev: Device to handle.
+ */
+static void dpm_reset(struct device *dev)
+{
+	dev->power.op_started = false;
+	dev->power.op_complete = false;
+}
+
+/**
+ * dpm_reset_all - Call dpm_reset() for all devices.
+ */
+static void dpm_reset_all(void)
+{
+	struct device *dev;
+
+	list_for_each_entry(dev, &dpm_list, power.entry)
+		dpm_reset(dev);
+}
+
+/**
+ * dpm_synchronize_noirq - Wait for "late" or "early" PM callbacks to complete.
+ *
+ * Wait for the "late" or "early" suspend/resume callbacks of all devices to
+ * complete and clear power.op_started and power.op_complete for all devices.
+ */
+static void dpm_synchronize_noirq(void)
+{
+	async_synchronize_full();
+	dpm_reset_all();
+}
+
+/**
+ * dpm_synchronize_noirq - Wait for PM callbacks to complete.
+ *
+ * Wait for the "regular" suspend/resume callbacks of all devices to complete
+ * and clear power.op_started and power.op_complete for all devices.
+ */
+static void dpm_synchronize(void)
+{
+	async_synchronize_full();
+	mutex_lock(&dpm_list_mtx);
+	dpm_reset_all();
+	mutex_unlock(&dpm_list_mtx);
+}
+
+/**
+ * device_pm_wait - Wait for a PM operation to complete.
+ * @sub: "Slave" device.
+ * @dev: Device to wait for.
+ *
+ * Wait for a PM operation carried out for @dev to complete, unless both @sub
+ * and @dev have to be handled synchronously (in such a case they are going to
+ * be handled in the right order anyway thanks to the pm_list ordering).
+ */
+static void device_pm_wait(struct device *sub, struct device *dev)
+{
+	if (!dev)
+		return;
+
+	if (!(sub->power.async_suspend || dev->power.async_suspend))
+		return;
+
+	if (!dev->power.op_complete) {
+		dev_dbg(sub, "PM: Waiting for %s %s\n", dev_driver_string(dev),
+			dev_name(dev));
+		wait_event(dev->power.wait_queue, !!dev->power.op_complete);
+	}
+}
+
+/**
+ * device_pm_wait_fn - Wrapper for device_pm_wait().
+ * @dev: Device to wait for.
+ * @data: Pointer to the "slave" device object.
+ */
+static int device_pm_wait_fn(struct device *dev, void *data)
+{
+	device_pm_wait((struct device *)data, dev);
+	return 0;
+}
+
+/**
+ * device_pm_wait_for_masters - Wait for all masters of given device.
+ * @slave: Device to wait for the masters of.
+ */
+static void device_pm_wait_for_masters(struct device *slave)
+{
+	if (!pm_trace_enabled)
+		device_for_each_master(slave, slave, device_pm_wait_fn);
+}
+
+/**
+ * device_pm_check - Check the power.op_complete flag of given device.
+ * @dev: Device to check.
+ */
+static bool device_pm_check(struct device *dev)
+{
+	int ret = 0;
+
+	if (dev)
+		ret = !dev->power.op_complete;
+
+	return ret;
+}
+
+/**
+ * device_pm_check_fn - Wrapper for device_pm_check().
+ * @dev: Device to check.
+ * @data: Ignored.
+ */
+static int device_pm_check_fn(struct device *dev, void *data)
+{
+	return device_pm_check(dev);
+}
+
+/**
+ * device_pm_check_masters - Check power.op_complete for masters of a device.
+ * @slave: Device to check the masters of.
+ */
+static int device_pm_check_masters(struct device *slave)
+{
+	return device_for_each_master(slave, NULL, device_pm_check_fn);
+}
+
+/**
  * pm_op - Execute the PM operation appropriate for given PM event.
  * @dev: Device to handle.
  * @ops: PM operations to choose from.
@@ -269,6 +397,24 @@ static int pm_noirq_op(struct device *de
 	return error;
 }
 
+/**
+ * pm_op_started - Mark the beginning of a PM operation for given device.
+ * @dev: Device to handle.
+ */
+static bool pm_op_started(struct device *dev)
+{
+	bool ret = false;
+
+	spin_lock_irq(&dev->power.lock);
+	if (dev->power.op_started)
+		ret = true;
+	else
+		dev->power.op_started = true;
+	spin_unlock_irq(&dev->power.lock);
+
+	return ret;
+}
+
 static char *pm_verb(int event)
 {
 	switch (event) {
@@ -310,33 +456,102 @@ static void pm_dev_err(struct device *de
 /*------------------------- Resume routines -------------------------*/
 
 /**
- * device_resume_noirq - Execute an "early resume" callback for given device.
+ * __device_resume_noirq - Execute an "early resume" callback for given device.
  * @dev: Device to handle.
  * @state: PM transition of the system being carried out.
  *
  * The driver of @dev will not receive interrupts while this function is being
  * executed.
  */
-static int device_resume_noirq(struct device *dev, pm_message_t state)
+static int __device_resume_noirq(struct device *dev, pm_message_t state)
 {
 	int error = 0;
 
 	TRACE_DEVICE(dev);
 	TRACE_RESUME(0);
 
-	if (!dev->bus)
-		goto End;
-
-	if (dev->bus->pm) {
+	if (dev->bus && dev->bus->pm) {
 		pm_dev_dbg(dev, state, "EARLY ");
 		error = pm_noirq_op(dev, dev->bus->pm, state);
 	}
- End:
+
+	dev->power.op_complete = true;
+	wake_up_all(&dev->power.wait_queue);
+
 	TRACE_RESUME(error);
 	return error;
 }
 
 /**
+ * async_device_resume_noirq - Wrapper of __device_resume_noirq().
+ * @dev: Device to resume.
+ */
+static void async_device_resume_noirq(struct device *dev)
+{
+	int error;
+
+	pm_dev_dbg(dev, pm_transition, "async EARLY ");
+	error = __device_resume_noirq(dev, pm_transition);
+	if (error)
+		pm_dev_err(dev, pm_transition, " async EARLY", error);
+}
+
+/**
+ * async_resume_noirq - Execute "early" resume callbacks asynchronously.
+ * @data: Pointer to the first device to resume.
+ * @cookie: Ignored.
+ *
+ * The execution of this function is scheduled with async_schedule(), so it runs
+ * in its own kernel thread.  It first calls the "early" resume callback for the
+ * device passed to it as @data.  Next, it walks dpm_list looking for devices
+ * that can be resumed without waiting for their "masters".  If such a device is
+ * found, its "early" resume callback is run.
+ */
+static void async_resume_noirq(void *data, async_cookie_t cookie)
+{
+	struct device *dev = (struct device *)data;
+
+	device_pm_wait_for_masters(dev);
+	async_device_resume_noirq(dev);
+
+	list_for_each_entry_continue(dev, &dpm_list, power.entry) {
+		if (!dev->power.async_suspend || dev->power.status <= DPM_OFF)
+			continue;
+
+		if (device_pm_check_masters(dev))
+			continue;
+
+		if (pm_op_started(dev))
+			continue;
+
+		pm_dev_dbg(dev, pm_transition, "out of order EARLY ");
+		async_device_resume_noirq(dev);
+	}
+}
+
+/**
+ * device_resume_noirq - Execute or schedule "early" resume callback.
+ * @dev: Device to resume.
+ *
+ * If @dev can be resumed asynchronously, schedule the execution of
+ * async_resume_noirq() for it.  Otherwise, execute its "early" resume callback
+ * directly.
+ */
+static int device_resume_noirq(struct device *dev)
+{
+	if (pm_op_started(dev))
+		return 0;
+
+	if (dev->power.async_suspend && !pm_trace_enabled) {
+		async_schedule(async_resume_noirq, dev);
+		return 0;
+	}
+
+	device_pm_wait_for_masters(dev);
+	return __device_resume_noirq(dev, pm_transition);
+}
+
+/**
  * dpm_resume_noirq - Execute "early resume" callbacks for non-sysdev devices.
  * @state: PM transition of the system being carried out.
  *
@@ -349,26 +564,28 @@ void dpm_resume_noirq(pm_message_t state
 
 	mutex_lock(&dpm_list_mtx);
 	transition_started = false;
+	pm_transition = state;
 	list_for_each_entry(dev, &dpm_list, power.entry)
 		if (dev->power.status > DPM_OFF) {
 			int error;
 
 			dev->power.status = DPM_OFF;
-			error = device_resume_noirq(dev, state);
+			error = device_resume_noirq(dev);
 			if (error)
-				pm_dev_err(dev, state, " early", error);
+				pm_dev_err(dev, state, " EARLY", error);
 		}
+	dpm_synchronize_noirq();
 	mutex_unlock(&dpm_list_mtx);
 	resume_device_irqs();
 }
 EXPORT_SYMBOL_GPL(dpm_resume_noirq);
 
 /**
- * device_resume - Execute "resume" callbacks for given device.
+ * __device_resume - Execute "resume" callbacks for given device.
  * @dev: Device to handle.
  * @state: PM transition of the system being carried out.
  */
-static int device_resume(struct device *dev, pm_message_t state)
+static int __device_resume(struct device *dev, pm_message_t state)
 {
 	int error = 0;
 
@@ -409,12 +626,92 @@ static int device_resume(struct device *
 	}
  End:
 	up(&dev->sem);
+	dev->power.op_complete = true;
+	wake_up_all(&dev->power.wait_queue);
 
 	TRACE_RESUME(error);
 	return error;
 }
 
 /**
+ * async_device_resume - Wrapper of __device_resume().
+ * @dev: Device to resume.
+ */
+static void async_device_resume(struct device *dev)
+{
+	int error;
+
+	pm_dev_dbg(dev, pm_transition, "async ");
+	error = __device_resume(dev, pm_transition);
+	if (error)
+		pm_dev_err(dev, pm_transition, " async", error);
+}
+
+/**
+ * async_resume - Execute resume callbacks asynchronously.
+ * @data: Pointer to the first device to resume.
+ * @cookie: Ignored.
+ *
+ * The execution of this function is scheduled with async_schedule(), so it runs
+ * in its own kernel thread.  It first calls the resume callbacks for the device
+ * passed to it as @data.  Next, it walks dpm_list looking for devices that can
+ * be resumed without waiting for their "masters".  If such a device is found,
+ * its resume callbacks are run.
+ */
+static void async_resume(void *data, async_cookie_t cookie)
+{
+	struct device *dev = (struct device *)data;
+
+	device_pm_wait_for_masters(dev);
+
+ repeat:
+	async_device_resume(dev);
+	put_device(dev);
+
+	mutex_lock(&dpm_list_mtx);
+	if (dev->power.status < DPM_OFF)
+		dev = to_device(dpm_list.next);
+	list_for_each_entry_continue(dev, &dpm_list, power.entry) {
+		if (!dev->power.async_suspend || dev->power.status < DPM_OFF)
+			continue;
+
+		if (device_pm_check_masters(dev))
+			continue;
+
+		if (pm_op_started(dev))
+			continue;
+
+		get_device(dev);
+		mutex_unlock(&dpm_list_mtx);
+		pm_dev_dbg(dev, pm_transition, "out of order ");
+		goto repeat;
+	}
+	mutex_unlock(&dpm_list_mtx);
+}
+
+/**
+ * device_resume - Execute or schedule resume callbacks for given device.
+ * @dev: Device to resume.
+ *
+ * If @dev can be resumed asynchronously, schedule the execution of
+ * async_resume() for it.  Otherwise, execute its resume callbacks directly.
+ */
+static int device_resume(struct device *dev)
+{
+	if (pm_op_started(dev))
+		return 0;
+
+	if (dev->power.async_suspend && !pm_trace_enabled) {
+		get_device(dev);
+		async_schedule(async_resume, dev);
+		return 0;
+	}
+
+	device_pm_wait_for_masters(dev);
+	return __device_resume(dev, pm_transition);
+}
+
+/**
  * dpm_resume - Execute "resume" callbacks for non-sysdev devices.
  * @state: PM transition of the system being carried out.
  *
@@ -427,6 +724,7 @@ static void dpm_resume(pm_message_t stat
 
 	INIT_LIST_HEAD(&list);
 	mutex_lock(&dpm_list_mtx);
+	pm_transition = state;
 	while (!list_empty(&dpm_list)) {
 		struct device *dev = to_device(dpm_list.next);
 
@@ -437,7 +735,7 @@ static void dpm_resume(pm_message_t stat
 			dev->power.status = DPM_RESUMING;
 			mutex_unlock(&dpm_list_mtx);
 
-			error = device_resume(dev, state);
+			error = device_resume(dev);
 
 			mutex_lock(&dpm_list_mtx);
 			if (error)
@@ -452,6 +750,7 @@ static void dpm_resume(pm_message_t stat
 	}
 	list_splice(&list, &dpm_list);
 	mutex_unlock(&dpm_list_mtx);
+	dpm_synchronize();
 }
 
 /**
@@ -775,8 +1074,10 @@ static int dpm_prepare(pm_message_t stat
 			break;
 		}
 		dev->power.status = DPM_SUSPENDING;
-		if (!list_empty(&dev->power.entry))
+		if (!list_empty(&dev->power.entry)) {
 			list_move_tail(&dev->power.entry, &list);
+			dpm_reset(dev);
+		}
 		put_device(dev);
 	}
 	list_splice(&list, &dpm_list);
Index: linux-2.6/drivers/base/power/common.c
===================================================================
--- linux-2.6.orig/drivers/base/power/common.c
+++ linux-2.6/drivers/base/power/common.c
@@ -19,10 +19,11 @@
  */
 void device_pm_init(struct device *dev)
 {
-	dev->power.status = DPM_ON;
 	spin_lock_init(&dev->power.lock);
+	init_waitqueue_head(&dev->power.wait_queue);
 	INIT_LIST_HEAD(&dev->power.master_links);
 	INIT_LIST_HEAD(&dev->power.slave_links);
+	dev->power.status = DPM_ON;
 	pm_runtime_init(dev);
 }
 
@@ -117,6 +118,8 @@ int pm_link_add(struct device *slave, st
 	return 0;
 
  err_link:
+	master->power.async_suspend = false;
+	slave->power.async_suspend = false;
 	error = -ENOMEM;
 	put_device(slave);
 

Re: [PATCH 2/6] PM: Asynchronous resume of devices

[Alan Stern]

On Wed, 26 Aug 2009, Rafael J. Wysocki wrote:

> From: Rafael J. Wysocki <rjw@sisk.pl>
> 
> Theoretically, the total time of system sleep transitions (suspend
> to RAM, hibernation) can be reduced by running suspend and resume
> callbacks of device drivers in parallel with each other.  However,
> there are dependencies between devices such that, for example, we may
> not be allowed to put one device into a low power state before
> anohter one has been suspended (e.g. we cannot suspend a bridge
> before suspending all devices behind it).  In particular, we're not
> allowed to suspend the parent of a device before suspending the
> device itself.  Analogously, we're not allowed to resume a device
> before resuming its parent.

> In this version of the patch the async threads started to execute
> the resume callbacks of specific device don't exit immediately having
> done that, but search dpm_list for devices whose PM dependencies have
> already been satisfied and execute their callbacks without waiting.

Given this design, why bother to invoke device_resume() for the async 
devices?  Why not just start up a bunch of async threads, each of which 
calls async_resume() repeatedly until everything is finished?  (And 
rearrange async_resume() to scan the list first and do the actual 
resume second.)

The same goes for the noirq versions.

Alan Stern

Re: [PATCH 2/6] PM: Asynchronous resume of devices

[Rafael J. Wysocki]

On Friday 28 August 2009, Alan Stern wrote:
> On Wed, 26 Aug 2009, Rafael J. Wysocki wrote:
> 
> > From: Rafael J. Wysocki <rjw@sisk.pl>
> > 
> > Theoretically, the total time of system sleep transitions (suspend
> > to RAM, hibernation) can be reduced by running suspend and resume
> > callbacks of device drivers in parallel with each other.  However,
> > there are dependencies between devices such that, for example, we may
> > not be allowed to put one device into a low power state before
> > anohter one has been suspended (e.g. we cannot suspend a bridge
> > before suspending all devices behind it).  In particular, we're not
> > allowed to suspend the parent of a device before suspending the
> > device itself.  Analogously, we're not allowed to resume a device
> > before resuming its parent.
> 
> > In this version of the patch the async threads started to execute
> > the resume callbacks of specific device don't exit immediately having
> > done that, but search dpm_list for devices whose PM dependencies have
> > already been satisfied and execute their callbacks without waiting.
> 
> Given this design, why bother to invoke device_resume() for the async 
> devices?  Why not just start up a bunch of async threads, each of which 
> calls async_resume() repeatedly until everything is finished?  (And 
> rearrange async_resume() to scan the list first and do the actual 
> resume second.)
> 
> The same goes for the noirq versions.

I thought about that, but there are a few things to figure out:
- how many threads to start
- when to start them
- stop condition
I had a few ideas, but then I thought it would be simpler to start an async
thread when we know there's some async work to do (ie. there's an async
device to handle) and make each async thread browse the list just once (the
rationale is that we've just handled a device, so there's a chance there are
some devices with satisfied dependencies down the list).

Thanks,
Rafael

Re: [PATCH 2/6] PM: Asynchronous resume of devices

[Alan Stern]

On Fri, 28 Aug 2009, Rafael J. Wysocki wrote:

> > Given this design, why bother to invoke device_resume() for the async 
> > devices?  Why not just start up a bunch of async threads, each of which 
> > calls async_resume() repeatedly until everything is finished?  (And 
> > rearrange async_resume() to scan the list first and do the actual 
> > resume second.)
> > 
> > The same goes for the noirq versions.
> 
> I thought about that, but there are a few things to figure out:
> - how many threads to start

That's a tough question.  Right now you start roughly as many threads
as there are async devices.  That seems like overkill.

I would expect that a reasonably small number of threads would suffice 
to achieve most of the possible time savings.  Something on the order 
of 10 should work well.  If the majority of the time is spent 
handling N devices then N+1 threads would be enough.  Judging from some 
of the comments posted earlier, even 4 threads would give a big 
advantage.

> - when to start them

You might as well start them at the beginning of dpm_resume and 
dpm_resume_noirq.  That way they can overlap with the synchronous 
operations.

> - stop condition

When an error occurs or when op_started has been set for every 
remaining async device.

> I had a few ideas, but then I thought it would be simpler to start an async
> thread when we know there's some async work to do (ie. there's an async
> device to handle) and make each async thread browse the list just once (the
> rationale is that we've just handled a device, so there's a chance there are
> some devices with satisfied dependencies down the list).

It comes down to this: Should there be many threads, each of which 
browses the list only once, or should there be a few threads, each of 
which browses the list many times?

Alan Stern

Re: [PATCH 2/6] PM: Asynchronous resume of devices

[Rafael J. Wysocki]

On Friday 28 August 2009, Alan Stern wrote:
> On Fri, 28 Aug 2009, Rafael J. Wysocki wrote:
> 
> > > Given this design, why bother to invoke device_resume() for the async 
> > > devices?  Why not just start up a bunch of async threads, each of which 
> > > calls async_resume() repeatedly until everything is finished?  (And 
> > > rearrange async_resume() to scan the list first and do the actual 
> > > resume second.)
> > > 
> > > The same goes for the noirq versions.
> > 
> > I thought about that, but there are a few things to figure out:
> > - how many threads to start
> 
> That's a tough question.  Right now you start roughly as many threads
> as there are async devices.  That seems like overkill.

In fact they are substantially fewer than that, for the following reasons.

First, the async framework will not start more than MAX_THREADS threads,
which is 256 at the moment.  This number is less than the number of async
devices to handle on an average system.

Second, no new async threads are started while the main thread is handling the
sync devices , so the existing threads have a chance to do their job.  If
there's a "cluster" of sync devices in dpm_list, the number of async threads
running is likely to drop rapidly while those devices are being handled.
(BTW, if there were no sync devices, the whole thing would be much simpler,
but I don't think it's realistic to assume we'll be able to get rid of them any
time soon).

Finally, but not least importantly, async threads are not started for the
async devices that were previously handled "out of order" by the already
running async threads (or by async threads that have already finished).  My
testing shows that there are quite a few of them on the average.  For example,
on the HP nx6325 typically there are as many as 580 async devices handled "out
of order" during a _single_ suspend-resume cycle (including the "early" and
"late" phases), while only a few (below 10) devices are waited for by at least
one async thread.

I can try to monitor the number of asyn threads started if you're interested.

> I would expect that a reasonably small number of threads would suffice 
> to achieve most of the possible time savings.  Something on the order 
> of 10 should work well.  If the majority of the time is spent 
> handling N devices then N+1 threads would be enough.  Judging from some 
> of the comments posted earlier, even 4 threads would give a big 
> advantage.

That unfortunately is not the case with the set of async devices including
PCI, ACPI and serio devices only.  The average time savings are between 5% to
14%, depending on the system and the phase of the cycle (the relative savings
are typically greater for suspend).  Still, that amounts to .5 s in some cases.

> > - when to start them
> 
> You might as well start them at the beginning of dpm_resume and 
> dpm_resume_noirq.  That way they can overlap with the synchronous 
> operations.

In that case they would have to wait in the beginning, so I'd need a mechanism
to wake them up.

Alternatively, there could be a limit to the number of async threads started
within the current design, but I'd prefer to leave that to the async framework
(namely, if MAX_THREADS makes sense for boot, it's also likely to make sense
for PM).

> > - stop condition
> 
> When an error occurs or when op_started has been set for every 
> remaining async device.

Yeah, that's the easy one. :-)

> > I had a few ideas, but then I thought it would be simpler to start an async
> > thread when we know there's some async work to do (ie. there's an async
> > device to handle) and make each async thread browse the list just once (the
> > rationale is that we've just handled a device, so there's a chance there are
> > some devices with satisfied dependencies down the list).
> 
> It comes down to this: Should there be many threads, each of which 
> browses the list only once, or should there be a few threads, each of 
> which browses the list many times?

Well, quite obviously I prefer the many threads version. :-)

Thanks,
Rafael

Re: [PATCH 2/6] PM: Asynchronous resume of devices

[Rafael J. Wysocki]

On Saturday 29 August 2009, Rafael J. Wysocki wrote:
> On Friday 28 August 2009, Alan Stern wrote:
> > On Fri, 28 Aug 2009, Rafael J. Wysocki wrote:
> > 
> > > > Given this design, why bother to invoke device_resume() for the async 
> > > > devices?  Why not just start up a bunch of async threads, each of which 
> > > > calls async_resume() repeatedly until everything is finished?  (And 
> > > > rearrange async_resume() to scan the list first and do the actual 
> > > > resume second.)
> > > > 
> > > > The same goes for the noirq versions.
> > > 
> > > I thought about that, but there are a few things to figure out:
> > > - how many threads to start
> > 
> > That's a tough question.  Right now you start roughly as many threads
> > as there are async devices.  That seems like overkill.
> 
> In fact they are substantially fewer than that, for the following reasons.
> 
> First, the async framework will not start more than MAX_THREADS threads,
> which is 256 at the moment.  This number is less than the number of async
> devices to handle on an average system.
> 
> Second, no new async threads are started while the main thread is handling the
> sync devices , so the existing threads have a chance to do their job.  If
> there's a "cluster" of sync devices in dpm_list, the number of async threads
> running is likely to drop rapidly while those devices are being handled.
> (BTW, if there were no sync devices, the whole thing would be much simpler,
> but I don't think it's realistic to assume we'll be able to get rid of them any
> time soon).
> 
> Finally, but not least importantly, async threads are not started for the
> async devices that were previously handled "out of order" by the already
> running async threads (or by async threads that have already finished).  My
> testing shows that there are quite a few of them on the average.  For example,
> on the HP nx6325 typically there are as many as 580 async devices handled "out
> of order" during a _single_ suspend-resume cycle (including the "early" and
> "late" phases), while only a few (below 10) devices are waited for by at least
> one async thread.
> 
> I can try to monitor the number of asyn threads started if you're interested.

I did that out of curiousity.  To be precise, I applied the appended patch on
top of the previous ones and the maximum number of scheduled async operations
I've got so far is 14 (strangely enough, during "late" suspend).

So, that's not like there are hundreds of async threads being started.

Thanks,
Rafael


---
 drivers/base/power/main.c |    9 +++++++++
 1 file changed, 9 insertions(+)

Index: linux-2.6/drivers/base/power/main.c
===================================================================
--- linux-2.6.orig/drivers/base/power/main.c
+++ linux-2.6/drivers/base/power/main.c
@@ -53,6 +53,8 @@ static pm_message_t pm_transition;
  */
 static bool transition_started;
 
+static unsigned int nr_async;
+
 /**
  * device_pm_lock - Lock the list of active devices used by the PM core.
  */
@@ -166,6 +168,8 @@ static void dpm_reset_all(void)
 
 	list_for_each_entry(dev, &dpm_list, power.entry)
 		dpm_reset(dev);
+	printk(KERN_INFO "PM: Scheduled %d async operations\n", nr_async);
+	nr_async = 0;
 }
 
 /**
@@ -563,6 +567,7 @@ static int device_resume_noirq(struct de
 
 	if (pm_async_enabled && !pm_trace_enabled && dev->power.async_suspend) {
 		async_schedule(async_resume_noirq, dev);
+		nr_async++;
 		return 0;
 	}
 
@@ -723,6 +728,7 @@ static int device_resume(struct device *
 	if (pm_async_enabled && !pm_trace_enabled && dev->power.async_suspend) {
 		get_device(dev);
 		async_schedule(async_resume, dev);
+		nr_async++;
 		return 0;
 	}
 
@@ -968,6 +974,7 @@ static int device_suspend_noirq(struct d
 
 	if (pm_async_enabled && dev->power.async_suspend) {
 		async_schedule(async_suspend_noirq, dev);
+		nr_async++;
 		return 0;
 	}
 
@@ -1143,6 +1150,7 @@ static int device_suspend(struct device 
 	if (pm_async_enabled && dev->power.async_suspend) {
 		get_device(dev);
 		async_schedule(async_suspend, dev);
+		nr_async++;
 		return 0;
 	}
 
@@ -1247,6 +1255,7 @@ static int dpm_prepare(pm_message_t stat
 	mutex_lock(&dpm_list_mtx);
 	transition_started = true;
 	atomic_set(&async_error, 0);
+	nr_async = 0;
 	while (!list_empty(&dpm_list)) {
 		struct device *dev = to_device(dpm_list.next);
 

Re: [PATCH 2/6] PM: Asynchronous resume of devices

[Alan Stern]

On Sat, 29 Aug 2009, Rafael J. Wysocki wrote:

> On Friday 28 August 2009, Alan Stern wrote:
> > On Fri, 28 Aug 2009, Rafael J. Wysocki wrote:
> > 
> > > > Given this design, why bother to invoke device_resume() for the async 
> > > > devices?  Why not just start up a bunch of async threads, each of which 
> > > > calls async_resume() repeatedly until everything is finished?  (And 
> > > > rearrange async_resume() to scan the list first and do the actual 
> > > > resume second.)
> > > > 
> > > > The same goes for the noirq versions.
> > > 
> > > I thought about that, but there are a few things to figure out:
> > > - how many threads to start
> > 
> > That's a tough question.  Right now you start roughly as many threads
> > as there are async devices.  That seems like overkill.
> 
> In fact they are substantially fewer than that, for the following reasons.
> 
> First, the async framework will not start more than MAX_THREADS threads,
> which is 256 at the moment.  This number is less than the number of async
> devices to handle on an average system.

Okay, but MAX_THREADS isn't under your control.  Remember also that 
each thread takes up some memory, and during hibernation we are in a 
memory-constrained situation.

> Second, no new async threads are started while the main thread is handling the
> sync devices , so the existing threads have a chance to do their job.  If
> there's a "cluster" of sync devices in dpm_list, the number of async threads
> running is likely to drop rapidly while those devices are being handled.
> (BTW, if there were no sync devices, the whole thing would be much simpler,
> but I don't think it's realistic to assume we'll be able to get rid of them any
> time soon).

Perhaps not, but it would be interesting to see what happens if every 
device is async.  Maybe you can try it and get a meaningful result.

> Finally, but not least importantly, async threads are not started for the
> async devices that were previously handled "out of order" by the already
> running async threads (or by async threads that have already finished).  My
> testing shows that there are quite a few of them on the average.  For example,
> on the HP nx6325 typically there are as many as 580 async devices handled "out
> of order" during a _single_ suspend-resume cycle (including the "early" and
> "late" phases), while only a few (below 10) devices are waited for by at least
> one async thread.

That is a difficult sort of thing to know in advance.  It ought to be 
highly influenced by the percentage of async devices; that's another 
reason for wanting to know what happens when every device is async.

> > I would expect that a reasonably small number of threads would suffice 
> > to achieve most of the possible time savings.  Something on the order 
> > of 10 should work well.  If the majority of the time is spent 
> > handling N devices then N+1 threads would be enough.  Judging from some 
> > of the comments posted earlier, even 4 threads would give a big 
> > advantage.
> 
> That unfortunately is not the case with the set of async devices including
> PCI, ACPI and serio devices only.  The average time savings are between 5% to
> 14%, depending on the system and the phase of the cycle (the relative savings
> are typically greater for suspend).  Still, that amounts to .5 s in some cases.

Without context it's hard to be sure, but I don't think your numbers 
contradict what I said.  If you get between 5% and 14% time savings 
with 14 threads, then you might get between 4% and 10% savings with 
only 4 threads.

I must agree, 14 threads isn't a lot.  But at the moment that number is 
random, not under your control.

> > > - when to start them
> > 
> > You might as well start them at the beginning of dpm_resume and 
> > dpm_resume_noirq.  That way they can overlap with the synchronous 
> > operations.
> 
> In that case they would have to wait in the beginning, so I'd need a mechanism
> to wake them up.

You already have two such mechanisms: dpm_list_mtx and the embedded 
wait_queue_heads.  Although in the scheme I'm proposing, no async 
threads would ever have to wait on a per-device waitqueue.  A 
system-wide waitqueue might work out better (for use when a thread 
reaches the end of the list and then waits before starting over at the 
beginning).

> Alternatively, there could be a limit to the number of async threads started
> within the current design, but I'd prefer to leave that to the async framework
> (namely, if MAX_THREADS makes sense for boot, it's also likely to make sense
> for PM).

Strictly speaking, a new thread should be started only when needed.  
That is, only when all the existing threads are busy running a 
callback.  It shouldn't be too hard to keep track of when that happens.

> > It comes down to this: Should there be many threads, each of which 
> > browses the list only once, or should there be a few threads, each of 
> > which browses the list many times?
> 
> Well, quite obviously I prefer the many threads version. :-)

Okay, clearly it's a matter of taste.  To me the many-threads version 
seems less elegant and less well controlled.

Alan Stern

Re: [PATCH 2/6] PM: Asynchronous resume of devices

[Rafael J. Wysocki]

On Saturday 29 August 2009, Alan Stern wrote:
> On Sat, 29 Aug 2009, Rafael J. Wysocki wrote:
> 
> > On Friday 28 August 2009, Alan Stern wrote:
> > > On Fri, 28 Aug 2009, Rafael J. Wysocki wrote:
> > > 
> > > > > Given this design, why bother to invoke device_resume() for the async 
> > > > > devices?  Why not just start up a bunch of async threads, each of which 
> > > > > calls async_resume() repeatedly until everything is finished?  (And 
> > > > > rearrange async_resume() to scan the list first and do the actual 
> > > > > resume second.)
> > > > > 
> > > > > The same goes for the noirq versions.
> > > > 
> > > > I thought about that, but there are a few things to figure out:
> > > > - how many threads to start
> > > 
> > > That's a tough question.  Right now you start roughly as many threads
> > > as there are async devices.  That seems like overkill.
> > 
> > In fact they are substantially fewer than that, for the following reasons.
> > 
> > First, the async framework will not start more than MAX_THREADS threads,
> > which is 256 at the moment.  This number is less than the number of async
> > devices to handle on an average system.
> 
> Okay, but MAX_THREADS isn't under your control.  Remember also that 
> each thread takes up some memory, and during hibernation we are in a 
> memory-constrained situation.

We keep some extra free memory for things like this.  It's not likely to be
exhausted by the async threads alone.

> > Second, no new async threads are started while the main thread is handling the
> > sync devices , so the existing threads have a chance to do their job.  If
> > there's a "cluster" of sync devices in dpm_list, the number of async threads
> > running is likely to drop rapidly while those devices are being handled.
> > (BTW, if there were no sync devices, the whole thing would be much simpler,
> > but I don't think it's realistic to assume we'll be able to get rid of them any
> > time soon).
> 
> Perhaps not, but it would be interesting to see what happens if every 
> device is async.  Maybe you can try it and get a meaningful result.

I could if it didn't crash.  Perhaps I can with init=/bin/bash.

> > Finally, but not least importantly, async threads are not started for the
> > async devices that were previously handled "out of order" by the already
> > running async threads (or by async threads that have already finished).  My
> > testing shows that there are quite a few of them on the average.  For example,
> > on the HP nx6325 typically there are as many as 580 async devices handled "out
> > of order" during a _single_ suspend-resume cycle (including the "early" and
> > "late" phases), while only a few (below 10) devices are waited for by at least
> > one async thread.
> 
> That is a difficult sort of thing to know in advance.  It ought to be 
> highly influenced by the percentage of async devices; that's another 
> reason for wanting to know what happens when every device is async.

There are a few factors beyond our direct control influencing this, like for
example how much time it takes to handle each individual device (that may
vary from .5 s to microseconds AFAICS).

In addition to this not only the percentage, but also the distribution of sync
devices in dpm_list has an effect.  For example, the situations where every
second device is sync and where the first half of devices are async and the
other is sync would lead to different kinds of behavior.

> > > I would expect that a reasonably small number of threads would suffice 
> > > to achieve most of the possible time savings.  Something on the order 
> > > of 10 should work well.  If the majority of the time is spent 
> > > handling N devices then N+1 threads would be enough.  Judging from some 
> > > of the comments posted earlier, even 4 threads would give a big 
> > > advantage.
> > 
> > That unfortunately is not the case with the set of async devices including
> > PCI, ACPI and serio devices only.  The average time savings are between 5% to
> > 14%, depending on the system and the phase of the cycle (the relative savings
> > are typically greater for suspend).  Still, that amounts to .5 s in some cases.
> 
> Without context it's hard to be sure, but I don't think your numbers 
> contradict what I said.  If you get between 5% and 14% time savings 
> with 14 threads, then you might get between 4% and 10% savings with 
> only 4 threads.

I only wanted to say that the advantage is not really that "big". :-)

> I must agree, 14 threads isn't a lot.  But at the moment that number is 
> random, not under your control.

It's not directly controlled, but there are some interactions between the
async threads, the main threads and the async framework that don't allow this
number to grow too much.

IMO it sometimes is better to allow things to work themselves out, as long as
they don't explode, than to try to keep everything under strict control.  YMMV.

> > > > - when to start them
> > > 
> > > You might as well start them at the beginning of dpm_resume and 
> > > dpm_resume_noirq.  That way they can overlap with the synchronous 
> > > operations.
> > 
> > In that case they would have to wait in the beginning, so I'd need a mechanism
> > to wake them up.
> 
> You already have two such mechanisms: dpm_list_mtx and the embedded 
> wait_queue_heads.  Although in the scheme I'm proposing, no async 
> threads would ever have to wait on a per-device waitqueue.  A 
> system-wide waitqueue might work out better (for use when a thread 
> reaches the end of the list and then waits before starting over at the 
> beginning).

However, sync devices may depend on the async ones and they need the
per-device wait queues for this purpose.

> > Alternatively, there could be a limit to the number of async threads started
> > within the current design, but I'd prefer to leave that to the async framework
> > (namely, if MAX_THREADS makes sense for boot, it's also likely to make sense
> > for PM).
> 
> Strictly speaking, a new thread should be started only when needed.  
> That is, only when all the existing threads are busy running a 
> callback.  It shouldn't be too hard to keep track of when that happens.

The async framework does that for us. :-)

Thanks,
Rafael

Re: [PATCH 2/6] PM: Asynchronous resume of devices

[Rafael J. Wysocki]

On Saturday 29 August 2009, Rafael J. Wysocki wrote:
> On Saturday 29 August 2009, Alan Stern wrote:
...
> > Strictly speaking, a new thread should be started only when needed.  
> > That is, only when all the existing threads are busy running a 
> > callback.  It shouldn't be too hard to keep track of when that happens.
> 
> The async framework does that for us. :-)

So, when I said "async threads", I should have rather said "async functions",
because one async thread can execute multiple functions scheduled with
async_schedule().

Taking that into consideration, it all works like this.  When there's a new
async device to handle and there are no async threads available, a new thread
is started and it runs the async function that handles the device and
"opportunistically" searches dpm_list (once) for other device that are ready
for handling.  After this function has returned, the thread goes to sleep and
it only exits after being idle for 1 s.  In turn, when there's a new async
device to handle and there are async threads available, on of them gets the
async function to run.

I don't really think we can do much better than this in any other approach.

Thanks,
Rafael

Re: [PATCH 2/6] PM: Asynchronous resume of devices

[Alan Stern]

On Sat, 29 Aug 2009, Rafael J. Wysocki wrote:

> On Saturday 29 August 2009, Rafael J. Wysocki wrote:
> > On Saturday 29 August 2009, Alan Stern wrote:
> ...
> > > Strictly speaking, a new thread should be started only when needed.  
> > > That is, only when all the existing threads are busy running a 
> > > callback.  It shouldn't be too hard to keep track of when that happens.
> > 
> > The async framework does that for us. :-)
> 
> So, when I said "async threads", I should have rather said "async functions",
> because one async thread can execute multiple functions scheduled with
> async_schedule().
> 
> Taking that into consideration, it all works like this.  When there's a new
> async device to handle and there are no async threads available, a new thread
> is started and it runs the async function that handles the device and
> "opportunistically" searches dpm_list (once) for other device that are ready
> for handling.  After this function has returned, the thread goes to sleep and
> it only exits after being idle for 1 s.  In turn, when there's a new async
> device to handle and there are async threads available, on of them gets the
> async function to run.
> 
> I don't really think we can do much better than this in any other approach.

Since it obviously works and is relatively simple, I guess we ought to 
adopt it.

Alan Stern

Re: [PATCH 2/6] PM: Asynchronous resume of devices

[Alan Stern]

On Sat, 29 Aug 2009, Rafael J. Wysocki wrote:

> I only wanted to say that the advantage is not really that "big". :-)
> 
> > I must agree, 14 threads isn't a lot.  But at the moment that number is 
> > random, not under your control.
> 
> It's not directly controlled, but there are some interactions between the
> async threads, the main threads and the async framework that don't allow this
> number to grow too much.
> 
> IMO it sometimes is better to allow things to work themselves out, as long as
> they don't explode, than to try to keep everything under strict control.  YMMV.

For testing purposes it would be nice to have a one-line summary for
each device containing a thread ID, start timestamp, end timestamp, and
elapsed time.  With that information you could evaluate the amount of
parallelism and determine where the bottlenecks are.  It would give a
much more detailed picture of the entire process than the total time of
your recent patch 9.

Alan Stern

Re: [PATCH 2/6] PM: Asynchronous resume of devices

[Rafael J. Wysocki]

On Sunday 30 August 2009, Alan Stern wrote:
> On Sat, 29 Aug 2009, Rafael J. Wysocki wrote:
> 
> > I only wanted to say that the advantage is not really that "big". :-)
> > 
> > > I must agree, 14 threads isn't a lot.  But at the moment that number is 
> > > random, not under your control.
> > 
> > It's not directly controlled, but there are some interactions between the
> > async threads, the main threads and the async framework that don't allow this
> > number to grow too much.
> > 
> > IMO it sometimes is better to allow things to work themselves out, as long as
> > they don't explode, than to try to keep everything under strict control.  YMMV.
> 
> For testing purposes it would be nice to have a one-line summary for
> each device containing a thread ID, start timestamp, end timestamp, and
> elapsed time.  With that information you could evaluate the amount of
> parallelism and determine where the bottlenecks are.  It would give a
> much more detailed picture of the entire process than the total time of
> your recent patch 9.

Of course it would.  I think I'll implement it.

The purpose of patch 9 is basically to allow one to see how much time is
spent on the handling of devices overall and to compare that with the time
spent on the other operation during suspend-resume.

Thanks,
Rafael

[PATCH 10] PM: Measure suspend and resume times for individual devices (was: Re: [PATCH 2/6] PM: Asynchronous resume of devices)

[Rafael J. Wysocki]

On Sunday 30 August 2009, Rafael J. Wysocki wrote:
> On Sunday 30 August 2009, Alan Stern wrote:
> > On Sat, 29 Aug 2009, Rafael J. Wysocki wrote:
> > 
> > > I only wanted to say that the advantage is not really that "big". :-)
> > > 
> > > > I must agree, 14 threads isn't a lot.  But at the moment that number is 
> > > > random, not under your control.
> > > 
> > > It's not directly controlled, but there are some interactions between the
> > > async threads, the main threads and the async framework that don't allow this
> > > number to grow too much.
> > > 
> > > IMO it sometimes is better to allow things to work themselves out, as long as
> > > they don't explode, than to try to keep everything under strict control.  YMMV.
> > 
> > For testing purposes it would be nice to have a one-line summary for
> > each device containing a thread ID, start timestamp, end timestamp, and
> > elapsed time.  With that information you could evaluate the amount of
> > parallelism and determine where the bottlenecks are.  It would give a
> > much more detailed picture of the entire process than the total time of
> > your recent patch 9.
> 
> Of course it would.  I think I'll implement it.

OK, below is a patch for that.  It only prints the time elapsed, because the
timestamps themselves can be obtained from the usual kernel timestamping.

It's on top of all the previous patches.

Thanks,
Rafael

---
From: Rafael J. Wysocki <rjw@sisk.pl>
Subject: PM: Measure suspend and resume times for individual devices

If verbose PM debugging is enabled, measure and print the time of
suspending and resuming of individual devices.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/base/power/main.c |   51 +++++++++++++++++++++++++++++++++++++++++-----
 kernel/power/swsusp.c     |    2 -
 2 files changed, 47 insertions(+), 6 deletions(-)

Index: linux-2.6/drivers/base/power/main.c
===================================================================
--- linux-2.6.orig/drivers/base/power/main.c
+++ linux-2.6/drivers/base/power/main.c
@@ -442,11 +442,11 @@ static bool pm_op_started(struct device 
  */
 int pm_time_elapsed(struct timeval *start, struct timeval *stop)
 {
-	s64 elapsed_centisecs64;
+	s64 elapsed_msecs64;
 
-	elapsed_centisecs64 = timeval_to_ns(stop) - timeval_to_ns(start);
-	do_div(elapsed_centisecs64, NSEC_PER_SEC / 100);
-	return elapsed_centisecs64;
+	elapsed_msecs64 = timeval_to_ns(stop) - timeval_to_ns(start);
+	do_div(elapsed_msecs64, NSEC_PER_SEC / 1000);
+	return elapsed_msecs64;
 }
 
 static char *pm_verb(int event)
@@ -476,7 +476,7 @@ static char *pm_verb(int event)
 static void dpm_show_time(struct timeval *start, struct timeval *stop,
 			  pm_message_t state, const char *info)
 {
-	int centisecs = pm_time_elapsed(start, stop);
+	int centisecs = pm_time_elapsed(start, stop) / 10;
 
 	printk(KERN_INFO "PM: %s%s%s of devices complete in %d.%02d seconds\n",
 		info ? info : "", info ? " " : "", pm_verb(state.event),
@@ -497,6 +497,33 @@ static void pm_dev_err(struct device *de
 		kobject_name(&dev->kobj), pm_verb(state.event), info, error);
 }
 
+#ifdef DEBUG
+static void device_show_time(struct timeval *start, struct device *dev,
+			     pm_message_t state, char *info)
+{
+	struct timeval stop;
+	int msecs;
+
+	do_gettimeofday(&stop);
+	msecs = pm_time_elapsed(start, &stop);
+	dev_dbg(dev, "PID %d: %s%s%s complete in %d.%03d seconds\n",
+		task_pid_nr(current), info ? info : "", info ? " " : "",
+		pm_verb(state.event), msecs / 1000, msecs % 1000);
+}
+
+#define TIMER_DECLARE(timer)	struct timeval timer
+#define TIMER_START(timer)	do { \
+		do_gettimeofday(&timer); \
+	} while (0)
+#define TIMER_STOP(timer, dev, state, info)	do { \
+		device_show_time(&timer, dev, state, info); \
+	} while (0)
+#else /* !DEBUG */
+#define TIMER_DECLARE(timer)
+#define TIMER_START(timer)	do { } while (0)
+#define TIMER_STOP(timer, dev, state, info)	do { } while (0)
+#endif /* !DEBUG */
+
 /*------------------------- Resume routines -------------------------*/
 
 /**
@@ -510,7 +537,9 @@ static void pm_dev_err(struct device *de
 static int __device_resume_noirq(struct device *dev, pm_message_t state)
 {
 	int error = 0;
+	TIMER_DECLARE(timer);
 
+	TIMER_START(timer);
 	TRACE_DEVICE(dev);
 	TRACE_RESUME(0);
 
@@ -523,6 +552,7 @@ static int __device_resume_noirq(struct 
 	wake_up_all(&dev->power.wait_queue);
 
 	TRACE_RESUME(error);
+	TIMER_STOP(timer, dev, state, "EARLY");
 	return error;
 }
 
@@ -639,7 +669,9 @@ EXPORT_SYMBOL_GPL(dpm_resume_noirq);
 static int __device_resume(struct device *dev, pm_message_t state)
 {
 	int error = 0;
+	TIMER_DECLARE(timer);
 
+	TIMER_START(timer);
 	TRACE_DEVICE(dev);
 	TRACE_RESUME(0);
 
@@ -681,6 +713,7 @@ static int __device_resume(struct device
 	wake_up_all(&dev->power.wait_queue);
 
 	TRACE_RESUME(error);
+	TIMER_STOP(timer, dev, state, NULL);
 	return error;
 }
 
@@ -923,6 +956,9 @@ static pm_message_t resume_event(pm_mess
 static int __device_suspend_noirq(struct device *dev, pm_message_t state)
 {
 	int error = 0;
+	TIMER_DECLARE(timer);
+
+	TIMER_START(timer);
 
 	if (dev->bus && dev->bus->pm) {
 		pm_dev_dbg(dev, state, "LATE ");
@@ -932,6 +968,7 @@ static int __device_suspend_noirq(struct
 	dev->power.op_complete = true;
 	wake_up_all(&dev->power.wait_queue);
 
+	TIMER_STOP(timer, dev, state, "LATE");
 	return error;
 }
 
@@ -1063,6 +1100,9 @@ EXPORT_SYMBOL_GPL(dpm_suspend_noirq);
 static int __device_suspend(struct device *dev, pm_message_t state)
 {
 	int error = 0;
+	TIMER_DECLARE(timer);
+
+	TIMER_START(timer);
 
 	down(&dev->sem);
 
@@ -1103,6 +1143,7 @@ static int __device_suspend(struct devic
 	dev->power.op_complete = true;
 	wake_up_all(&dev->power.wait_queue);
 
+	TIMER_STOP(timer, dev, state, NULL);
 	return error;
 }
 
Index: linux-2.6/kernel/power/swsusp.c
===================================================================
--- linux-2.6.orig/kernel/power/swsusp.c
+++ linux-2.6/kernel/power/swsusp.c
@@ -169,7 +169,7 @@ int swsusp_swap_in_use(void)
 void swsusp_show_speed(struct timeval *start, struct timeval *stop,
 			unsigned nr_pages, char *msg)
 {
-	int centisecs = pm_time_elapsed(start, stop);
+	int centisecs = pm_time_elapsed(start, stop) / 10;
 	int k;
 	int kps;
 

Re: [PATCH 10] PM: Measure suspend and resume times for individual devices (was: Re: [PATCH 2/6] PM: Asynchronous resume of devices)

[Ingo Molnar]

* Rafael J. Wysocki <rjw@sisk.pl> wrote:

> On Sunday 30 August 2009, Rafael J. Wysocki wrote:
> > On Sunday 30 August 2009, Alan Stern wrote:
> > > On Sat, 29 Aug 2009, Rafael J. Wysocki wrote:
> > > 
> > > > I only wanted to say that the advantage is not really that "big". :-)
> > > > 
> > > > > I must agree, 14 threads isn't a lot.  But at the moment that number is 
> > > > > random, not under your control.
> > > > 
> > > > It's not directly controlled, but there are some interactions between the
> > > > async threads, the main threads and the async framework that don't allow this
> > > > number to grow too much.
> > > > 
> > > > IMO it sometimes is better to allow things to work themselves out, as long as
> > > > they don't explode, than to try to keep everything under strict control.  YMMV.
> > > 
> > > For testing purposes it would be nice to have a one-line summary for
> > > each device containing a thread ID, start timestamp, end timestamp, and
> > > elapsed time.  With that information you could evaluate the amount of
> > > parallelism and determine where the bottlenecks are.  It would give a
> > > much more detailed picture of the entire process than the total time of
> > > your recent patch 9.
> > 
> > Of course it would.  I think I'll implement it.
> 
> OK, below is a patch for that.  It only prints the time elapsed, because the
> timestamps themselves can be obtained from the usual kernel timestamping.
> 
> It's on top of all the previous patches.
> 
> Thanks,
> Rafael
> 
> ---
> From: Rafael J. Wysocki <rjw@sisk.pl>
> Subject: PM: Measure suspend and resume times for individual devices
> 
> If verbose PM debugging is enabled, measure and print the time of
> suspending and resuming of individual devices.
> 
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> ---
>  drivers/base/power/main.c |   51 +++++++++++++++++++++++++++++++++++++++++-----
>  kernel/power/swsusp.c     |    2 -
>  2 files changed, 47 insertions(+), 6 deletions(-)
> 
> Index: linux-2.6/drivers/base/power/main.c
> ===================================================================
> --- linux-2.6.orig/drivers/base/power/main.c
> +++ linux-2.6/drivers/base/power/main.c
> @@ -442,11 +442,11 @@ static bool pm_op_started(struct device 
>   */
>  int pm_time_elapsed(struct timeval *start, struct timeval *stop)
>  {
> -	s64 elapsed_centisecs64;
> +	s64 elapsed_msecs64;
>  
> -	elapsed_centisecs64 = timeval_to_ns(stop) - timeval_to_ns(start);
> -	do_div(elapsed_centisecs64, NSEC_PER_SEC / 100);
> -	return elapsed_centisecs64;
> +	elapsed_msecs64 = timeval_to_ns(stop) - timeval_to_ns(start);
> +	do_div(elapsed_msecs64, NSEC_PER_SEC / 1000);
> +	return elapsed_msecs64;
>  }
>  
>  static char *pm_verb(int event)
> @@ -476,7 +476,7 @@ static char *pm_verb(int event)
>  static void dpm_show_time(struct timeval *start, struct timeval *stop,
>  			  pm_message_t state, const char *info)
>  {
> -	int centisecs = pm_time_elapsed(start, stop);
> +	int centisecs = pm_time_elapsed(start, stop) / 10;
>  
>  	printk(KERN_INFO "PM: %s%s%s of devices complete in %d.%02d seconds\n",
>  		info ? info : "", info ? " " : "", pm_verb(state.event),
> @@ -497,6 +497,33 @@ static void pm_dev_err(struct device *de
>  		kobject_name(&dev->kobj), pm_verb(state.event), info, error);
>  }
>  
> +#ifdef DEBUG
> +static void device_show_time(struct timeval *start, struct device *dev,
> +			     pm_message_t state, char *info)
> +{
> +	struct timeval stop;
> +	int msecs;
> +
> +	do_gettimeofday(&stop);
> +	msecs = pm_time_elapsed(start, &stop);
> +	dev_dbg(dev, "PID %d: %s%s%s complete in %d.%03d seconds\n",
> +		task_pid_nr(current), info ? info : "", info ? " " : "",
> +		pm_verb(state.event), msecs / 1000, msecs % 1000);
> +}
> +
> +#define TIMER_DECLARE(timer)	struct timeval timer
> +#define TIMER_START(timer)	do { \
> +		do_gettimeofday(&timer); \
> +	} while (0)
> +#define TIMER_STOP(timer, dev, state, info)	do { \
> +		device_show_time(&timer, dev, state, info); \
> +	} while (0)
> +#else /* !DEBUG */
> +#define TIMER_DECLARE(timer)
> +#define TIMER_START(timer)	do { } while (0)
> +#define TIMER_STOP(timer, dev, state, info)	do { } while (0)
> +#endif /* !DEBUG */
> +
>  /*------------------------- Resume routines -------------------------*/
>  
>  /**
> @@ -510,7 +537,9 @@ static void pm_dev_err(struct device *de
>  static int __device_resume_noirq(struct device *dev, pm_message_t state)
>  {
>  	int error = 0;
> +	TIMER_DECLARE(timer);
>  
> +	TIMER_START(timer);
>  	TRACE_DEVICE(dev);
>  	TRACE_RESUME(0);
>  
> @@ -523,6 +552,7 @@ static int __device_resume_noirq(struct 
>  	wake_up_all(&dev->power.wait_queue);
>  
>  	TRACE_RESUME(error);
> +	TIMER_STOP(timer, dev, state, "EARLY");
>  	return error;

Hm, these CPP macros are rather ugly. Why is there a need for the 
TIMER_DECLARE() wrapper - if a proper inline function is used 
there's no need for that. There's other all-capitals macros in that 
code implementing code (and not constants) - is that really 
justified/clean?

	Ingo

Re: [PATCH 10] PM: Measure suspend and resume times for individual devices (was: Re: [PATCH 2/6] PM: Asynchronous resume of devices)

[Rafael J. Wysocki]

On Monday 31 August 2009, Ingo Molnar wrote:
> 
> * Rafael J. Wysocki <rjw@sisk.pl> wrote:
> 
> > On Sunday 30 August 2009, Rafael J. Wysocki wrote:
> > > On Sunday 30 August 2009, Alan Stern wrote:
> > > > On Sat, 29 Aug 2009, Rafael J. Wysocki wrote:
> > > > 
> > > > > I only wanted to say that the advantage is not really that "big". :-)
> > > > > 
> > > > > > I must agree, 14 threads isn't a lot.  But at the moment that number is 
> > > > > > random, not under your control.
> > > > > 
> > > > > It's not directly controlled, but there are some interactions between the
> > > > > async threads, the main threads and the async framework that don't allow this
> > > > > number to grow too much.
> > > > > 
> > > > > IMO it sometimes is better to allow things to work themselves out, as long as
> > > > > they don't explode, than to try to keep everything under strict control.  YMMV.
> > > > 
> > > > For testing purposes it would be nice to have a one-line summary for
> > > > each device containing a thread ID, start timestamp, end timestamp, and
> > > > elapsed time.  With that information you could evaluate the amount of
> > > > parallelism and determine where the bottlenecks are.  It would give a
> > > > much more detailed picture of the entire process than the total time of
> > > > your recent patch 9.
> > > 
> > > Of course it would.  I think I'll implement it.
> > 
> > OK, below is a patch for that.  It only prints the time elapsed, because the
> > timestamps themselves can be obtained from the usual kernel timestamping.
> > 
> > It's on top of all the previous patches.
> > 
> > Thanks,
> > Rafael
> > 
> > ---
> > From: Rafael J. Wysocki <rjw@sisk.pl>
> > Subject: PM: Measure suspend and resume times for individual devices
> > 
> > If verbose PM debugging is enabled, measure and print the time of
> > suspending and resuming of individual devices.
> > 
> > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> > ---
> >  drivers/base/power/main.c |   51 +++++++++++++++++++++++++++++++++++++++++-----
> >  kernel/power/swsusp.c     |    2 -
> >  2 files changed, 47 insertions(+), 6 deletions(-)
> > 
> > Index: linux-2.6/drivers/base/power/main.c
> > ===================================================================
> > --- linux-2.6.orig/drivers/base/power/main.c
> > +++ linux-2.6/drivers/base/power/main.c
> > @@ -442,11 +442,11 @@ static bool pm_op_started(struct device 
> >   */
> >  int pm_time_elapsed(struct timeval *start, struct timeval *stop)
> >  {
> > -	s64 elapsed_centisecs64;
> > +	s64 elapsed_msecs64;
> >  
> > -	elapsed_centisecs64 = timeval_to_ns(stop) - timeval_to_ns(start);
> > -	do_div(elapsed_centisecs64, NSEC_PER_SEC / 100);
> > -	return elapsed_centisecs64;
> > +	elapsed_msecs64 = timeval_to_ns(stop) - timeval_to_ns(start);
> > +	do_div(elapsed_msecs64, NSEC_PER_SEC / 1000);
> > +	return elapsed_msecs64;
> >  }
> >  
> >  static char *pm_verb(int event)
> > @@ -476,7 +476,7 @@ static char *pm_verb(int event)
> >  static void dpm_show_time(struct timeval *start, struct timeval *stop,
> >  			  pm_message_t state, const char *info)
> >  {
> > -	int centisecs = pm_time_elapsed(start, stop);
> > +	int centisecs = pm_time_elapsed(start, stop) / 10;
> >  
> >  	printk(KERN_INFO "PM: %s%s%s of devices complete in %d.%02d seconds\n",
> >  		info ? info : "", info ? " " : "", pm_verb(state.event),
> > @@ -497,6 +497,33 @@ static void pm_dev_err(struct device *de
> >  		kobject_name(&dev->kobj), pm_verb(state.event), info, error);
> >  }
> >  
> > +#ifdef DEBUG
> > +static void device_show_time(struct timeval *start, struct device *dev,
> > +			     pm_message_t state, char *info)
> > +{
> > +	struct timeval stop;
> > +	int msecs;
> > +
> > +	do_gettimeofday(&stop);
> > +	msecs = pm_time_elapsed(start, &stop);
> > +	dev_dbg(dev, "PID %d: %s%s%s complete in %d.%03d seconds\n",
> > +		task_pid_nr(current), info ? info : "", info ? " " : "",
> > +		pm_verb(state.event), msecs / 1000, msecs % 1000);
> > +}
> > +
> > +#define TIMER_DECLARE(timer)	struct timeval timer
> > +#define TIMER_START(timer)	do { \
> > +		do_gettimeofday(&timer); \
> > +	} while (0)
> > +#define TIMER_STOP(timer, dev, state, info)	do { \
> > +		device_show_time(&timer, dev, state, info); \
> > +	} while (0)
> > +#else /* !DEBUG */
> > +#define TIMER_DECLARE(timer)
> > +#define TIMER_START(timer)	do { } while (0)
> > +#define TIMER_STOP(timer, dev, state, info)	do { } while (0)
> > +#endif /* !DEBUG */
> > +
> >  /*------------------------- Resume routines -------------------------*/
> >  
> >  /**
> > @@ -510,7 +537,9 @@ static void pm_dev_err(struct device *de
> >  static int __device_resume_noirq(struct device *dev, pm_message_t state)
> >  {
> >  	int error = 0;
> > +	TIMER_DECLARE(timer);
> >  
> > +	TIMER_START(timer);
> >  	TRACE_DEVICE(dev);
> >  	TRACE_RESUME(0);
> >  
> > @@ -523,6 +552,7 @@ static int __device_resume_noirq(struct 
> >  	wake_up_all(&dev->power.wait_queue);
> >  
> >  	TRACE_RESUME(error);
> > +	TIMER_STOP(timer, dev, state, "EARLY");
> >  	return error;
> 
> Hm, these CPP macros are rather ugly. Why is there a need for the 
> TIMER_DECLARE() wrapper - if a proper inline function is used 
> there's no need for that.

I need a variable to be declared so that I can save the "start" timestamp
in it.  I don't need the variable if DEBUG is unset, though.

How would you do that without using a macro?  Or #ifdef #endif block that
would be uglier IMO (which is why I didn't do that)?

> There's other all-capitals macros in that code implementing code (and not
> constants) - is that really justified/clean?

Do you mean the TRACE_* macros?  Please ask Linus about that, they are from
him. :-)

Thanks,
Rafael

Re: [PATCH 10] PM: Measure suspend and resume times for individual devices (was: Re: [PATCH 2/6] PM: Asynchronous resume of devices)

[Ingo Molnar]

* Rafael J. Wysocki <rjw@sisk.pl> wrote:

> On Monday 31 August 2009, Ingo Molnar wrote:
> > 
> > * Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > 
> > > On Sunday 30 August 2009, Rafael J. Wysocki wrote:
> > > > On Sunday 30 August 2009, Alan Stern wrote:
> > > > > On Sat, 29 Aug 2009, Rafael J. Wysocki wrote:
> > > > > 
> > > > > > I only wanted to say that the advantage is not really that "big". :-)
> > > > > > 
> > > > > > > I must agree, 14 threads isn't a lot.  But at the moment that number is 
> > > > > > > random, not under your control.
> > > > > > 
> > > > > > It's not directly controlled, but there are some interactions between the
> > > > > > async threads, the main threads and the async framework that don't allow this
> > > > > > number to grow too much.
> > > > > > 
> > > > > > IMO it sometimes is better to allow things to work themselves out, as long as
> > > > > > they don't explode, than to try to keep everything under strict control.  YMMV.
> > > > > 
> > > > > For testing purposes it would be nice to have a one-line summary for
> > > > > each device containing a thread ID, start timestamp, end timestamp, and
> > > > > elapsed time.  With that information you could evaluate the amount of
> > > > > parallelism and determine where the bottlenecks are.  It would give a
> > > > > much more detailed picture of the entire process than the total time of
> > > > > your recent patch 9.
> > > > 
> > > > Of course it would.  I think I'll implement it.
> > > 
> > > OK, below is a patch for that.  It only prints the time elapsed, because the
> > > timestamps themselves can be obtained from the usual kernel timestamping.
> > > 
> > > It's on top of all the previous patches.
> > > 
> > > Thanks,
> > > Rafael
> > > 
> > > ---
> > > From: Rafael J. Wysocki <rjw@sisk.pl>
> > > Subject: PM: Measure suspend and resume times for individual devices
> > > 
> > > If verbose PM debugging is enabled, measure and print the time of
> > > suspending and resuming of individual devices.
> > > 
> > > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> > > ---
> > >  drivers/base/power/main.c |   51 +++++++++++++++++++++++++++++++++++++++++-----
> > >  kernel/power/swsusp.c     |    2 -
> > >  2 files changed, 47 insertions(+), 6 deletions(-)
> > > 
> > > Index: linux-2.6/drivers/base/power/main.c
> > > ===================================================================
> > > --- linux-2.6.orig/drivers/base/power/main.c
> > > +++ linux-2.6/drivers/base/power/main.c
> > > @@ -442,11 +442,11 @@ static bool pm_op_started(struct device 
> > >   */
> > >  int pm_time_elapsed(struct timeval *start, struct timeval *stop)
> > >  {
> > > -	s64 elapsed_centisecs64;
> > > +	s64 elapsed_msecs64;
> > >  
> > > -	elapsed_centisecs64 = timeval_to_ns(stop) - timeval_to_ns(start);
> > > -	do_div(elapsed_centisecs64, NSEC_PER_SEC / 100);
> > > -	return elapsed_centisecs64;
> > > +	elapsed_msecs64 = timeval_to_ns(stop) - timeval_to_ns(start);
> > > +	do_div(elapsed_msecs64, NSEC_PER_SEC / 1000);
> > > +	return elapsed_msecs64;
> > >  }
> > >  
> > >  static char *pm_verb(int event)
> > > @@ -476,7 +476,7 @@ static char *pm_verb(int event)
> > >  static void dpm_show_time(struct timeval *start, struct timeval *stop,
> > >  			  pm_message_t state, const char *info)
> > >  {
> > > -	int centisecs = pm_time_elapsed(start, stop);
> > > +	int centisecs = pm_time_elapsed(start, stop) / 10;
> > >  
> > >  	printk(KERN_INFO "PM: %s%s%s of devices complete in %d.%02d seconds\n",
> > >  		info ? info : "", info ? " " : "", pm_verb(state.event),
> > > @@ -497,6 +497,33 @@ static void pm_dev_err(struct device *de
> > >  		kobject_name(&dev->kobj), pm_verb(state.event), info, error);
> > >  }
> > >  
> > > +#ifdef DEBUG
> > > +static void device_show_time(struct timeval *start, struct device *dev,
> > > +			     pm_message_t state, char *info)
> > > +{
> > > +	struct timeval stop;
> > > +	int msecs;
> > > +
> > > +	do_gettimeofday(&stop);
> > > +	msecs = pm_time_elapsed(start, &stop);
> > > +	dev_dbg(dev, "PID %d: %s%s%s complete in %d.%03d seconds\n",
> > > +		task_pid_nr(current), info ? info : "", info ? " " : "",
> > > +		pm_verb(state.event), msecs / 1000, msecs % 1000);
> > > +}
> > > +
> > > +#define TIMER_DECLARE(timer)	struct timeval timer
> > > +#define TIMER_START(timer)	do { \
> > > +		do_gettimeofday(&timer); \
> > > +	} while (0)
> > > +#define TIMER_STOP(timer, dev, state, info)	do { \
> > > +		device_show_time(&timer, dev, state, info); \
> > > +	} while (0)
> > > +#else /* !DEBUG */
> > > +#define TIMER_DECLARE(timer)
> > > +#define TIMER_START(timer)	do { } while (0)
> > > +#define TIMER_STOP(timer, dev, state, info)	do { } while (0)
> > > +#endif /* !DEBUG */
> > > +
> > >  /*------------------------- Resume routines -------------------------*/
> > >  
> > >  /**
> > > @@ -510,7 +537,9 @@ static void pm_dev_err(struct device *de
> > >  static int __device_resume_noirq(struct device *dev, pm_message_t state)
> > >  {
> > >  	int error = 0;
> > > +	TIMER_DECLARE(timer);
> > >  
> > > +	TIMER_START(timer);
> > >  	TRACE_DEVICE(dev);
> > >  	TRACE_RESUME(0);
> > >  
> > > @@ -523,6 +552,7 @@ static int __device_resume_noirq(struct 
> > >  	wake_up_all(&dev->power.wait_queue);
> > >  
> > >  	TRACE_RESUME(error);
> > > +	TIMER_STOP(timer, dev, state, "EARLY");
> > >  	return error;
> > 
> > Hm, these CPP macros are rather ugly. Why is there a need for 
> > the TIMER_DECLARE() wrapper - if a proper inline function is 
> > used there's no need for that.
> 
> I need a variable to be declared so that I can save the "start" 
> timestamp in it.  I don't need the variable if DEBUG is unset, 
> though.
> 
> How would you do that without using a macro?  Or #ifdef #endif 
> block that would be uglier IMO (which is why I didn't do that)?

Well, why not use an inline function like i suggested? [which does 
nothing in the !enabled case] You can keep the local variable always 
defined.

> > There's other all-capitals macros in that code implementing code 
> > (and not constants) - is that really justified/clean?
> 
> Do you mean the TRACE_* macros?  Please ask Linus about that, they 
> are from him. :-)

hey, Linus is not immune to crap either :) IIRC the TRACE_*() stuff 
was a quick hack originally to debug some nasty suspend/resume hang. 
Combined with the DECLARE/START/STOP macros it definitely starts 
looking a bit ugly. If you find something cleaner looking i doubt 
Linus will complain. (no biggie if you keep it in place either)

	Ingo

Re: [PATCH 10] PM: Measure suspend and resume times for individual devices (was: Re: [PATCH 2/6] PM: Asynchronous resume of devices)

[Rafael J. Wysocki]

On Monday 31 August 2009, Ingo Molnar wrote:
> 
> * Rafael J. Wysocki <rjw@sisk.pl> wrote:
> 
> > On Monday 31 August 2009, Ingo Molnar wrote:
> > > 
> > > * Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > > 
> > > > On Sunday 30 August 2009, Rafael J. Wysocki wrote:
> > > > > On Sunday 30 August 2009, Alan Stern wrote:
> > > > > > On Sat, 29 Aug 2009, Rafael J. Wysocki wrote:
> > > > > > 
> > > > > > > I only wanted to say that the advantage is not really that "big". :-)
> > > > > > > 
> > > > > > > > I must agree, 14 threads isn't a lot.  But at the moment that number is 
> > > > > > > > random, not under your control.
> > > > > > > 
> > > > > > > It's not directly controlled, but there are some interactions between the
> > > > > > > async threads, the main threads and the async framework that don't allow this
> > > > > > > number to grow too much.
> > > > > > > 
> > > > > > > IMO it sometimes is better to allow things to work themselves out, as long as
> > > > > > > they don't explode, than to try to keep everything under strict control.  YMMV.
> > > > > > 
> > > > > > For testing purposes it would be nice to have a one-line summary for
> > > > > > each device containing a thread ID, start timestamp, end timestamp, and
> > > > > > elapsed time.  With that information you could evaluate the amount of
> > > > > > parallelism and determine where the bottlenecks are.  It would give a
> > > > > > much more detailed picture of the entire process than the total time of
> > > > > > your recent patch 9.
> > > > > 
> > > > > Of course it would.  I think I'll implement it.
> > > > 
> > > > OK, below is a patch for that.  It only prints the time elapsed, because the
> > > > timestamps themselves can be obtained from the usual kernel timestamping.
> > > > 
> > > > It's on top of all the previous patches.
> > > > 
> > > > Thanks,
> > > > Rafael
> > > > 
> > > > ---
> > > > From: Rafael J. Wysocki <rjw@sisk.pl>
> > > > Subject: PM: Measure suspend and resume times for individual devices
> > > > 
> > > > If verbose PM debugging is enabled, measure and print the time of
> > > > suspending and resuming of individual devices.
> > > > 
> > > > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> > > > ---
> > > >  drivers/base/power/main.c |   51 +++++++++++++++++++++++++++++++++++++++++-----
> > > >  kernel/power/swsusp.c     |    2 -
> > > >  2 files changed, 47 insertions(+), 6 deletions(-)
> > > > 
> > > > Index: linux-2.6/drivers/base/power/main.c
> > > > ===================================================================
> > > > --- linux-2.6.orig/drivers/base/power/main.c
> > > > +++ linux-2.6/drivers/base/power/main.c
> > > > @@ -442,11 +442,11 @@ static bool pm_op_started(struct device 
> > > >   */
> > > >  int pm_time_elapsed(struct timeval *start, struct timeval *stop)
> > > >  {
> > > > -	s64 elapsed_centisecs64;
> > > > +	s64 elapsed_msecs64;
> > > >  
> > > > -	elapsed_centisecs64 = timeval_to_ns(stop) - timeval_to_ns(start);
> > > > -	do_div(elapsed_centisecs64, NSEC_PER_SEC / 100);
> > > > -	return elapsed_centisecs64;
> > > > +	elapsed_msecs64 = timeval_to_ns(stop) - timeval_to_ns(start);
> > > > +	do_div(elapsed_msecs64, NSEC_PER_SEC / 1000);
> > > > +	return elapsed_msecs64;
> > > >  }
> > > >  
> > > >  static char *pm_verb(int event)
> > > > @@ -476,7 +476,7 @@ static char *pm_verb(int event)
> > > >  static void dpm_show_time(struct timeval *start, struct timeval *stop,
> > > >  			  pm_message_t state, const char *info)
> > > >  {
> > > > -	int centisecs = pm_time_elapsed(start, stop);
> > > > +	int centisecs = pm_time_elapsed(start, stop) / 10;
> > > >  
> > > >  	printk(KERN_INFO "PM: %s%s%s of devices complete in %d.%02d seconds\n",
> > > >  		info ? info : "", info ? " " : "", pm_verb(state.event),
> > > > @@ -497,6 +497,33 @@ static void pm_dev_err(struct device *de
> > > >  		kobject_name(&dev->kobj), pm_verb(state.event), info, error);
> > > >  }
> > > >  
> > > > +#ifdef DEBUG
> > > > +static void device_show_time(struct timeval *start, struct device *dev,
> > > > +			     pm_message_t state, char *info)
> > > > +{
> > > > +	struct timeval stop;
> > > > +	int msecs;
> > > > +
> > > > +	do_gettimeofday(&stop);
> > > > +	msecs = pm_time_elapsed(start, &stop);
> > > > +	dev_dbg(dev, "PID %d: %s%s%s complete in %d.%03d seconds\n",
> > > > +		task_pid_nr(current), info ? info : "", info ? " " : "",
> > > > +		pm_verb(state.event), msecs / 1000, msecs % 1000);
> > > > +}
> > > > +
> > > > +#define TIMER_DECLARE(timer)	struct timeval timer
> > > > +#define TIMER_START(timer)	do { \
> > > > +		do_gettimeofday(&timer); \
> > > > +	} while (0)
> > > > +#define TIMER_STOP(timer, dev, state, info)	do { \
> > > > +		device_show_time(&timer, dev, state, info); \
> > > > +	} while (0)
> > > > +#else /* !DEBUG */
> > > > +#define TIMER_DECLARE(timer)
> > > > +#define TIMER_START(timer)	do { } while (0)
> > > > +#define TIMER_STOP(timer, dev, state, info)	do { } while (0)
> > > > +#endif /* !DEBUG */
> > > > +
> > > >  /*------------------------- Resume routines -------------------------*/
> > > >  
> > > >  /**
> > > > @@ -510,7 +537,9 @@ static void pm_dev_err(struct device *de
> > > >  static int __device_resume_noirq(struct device *dev, pm_message_t state)
> > > >  {
> > > >  	int error = 0;
> > > > +	TIMER_DECLARE(timer);
> > > >  
> > > > +	TIMER_START(timer);
> > > >  	TRACE_DEVICE(dev);
> > > >  	TRACE_RESUME(0);
> > > >  
> > > > @@ -523,6 +552,7 @@ static int __device_resume_noirq(struct 
> > > >  	wake_up_all(&dev->power.wait_queue);
> > > >  
> > > >  	TRACE_RESUME(error);
> > > > +	TIMER_STOP(timer, dev, state, "EARLY");
> > > >  	return error;
> > > 
> > > Hm, these CPP macros are rather ugly. Why is there a need for 
> > > the TIMER_DECLARE() wrapper - if a proper inline function is 
> > > used there's no need for that.
> > 
> > I need a variable to be declared so that I can save the "start" 
> > timestamp in it.  I don't need the variable if DEBUG is unset, 
> > though.
> > 
> > How would you do that without using a macro?  Or #ifdef #endif 
> > block that would be uglier IMO (which is why I didn't do that)?
> 
> Well, why not use an inline function like i suggested? [which does 
> nothing in the !enabled case] You can keep the local variable always 
> defined.

Well, I used the macros _exactly_ because I didn't want to keep the local
variable always defined.

Now, if you think that adding several useless bytes to the stack frame is OK,
perhaps it can be always defined.  However, IMnsHO that would be
(a) confusing (why define a variable you don't use) and (b) wasteful.

Perhaps the names of the macros should be directly related to debugging?

Best,
Rafael

[PATCH 10 update] PM: Measure suspend and resume times for individual devices

[Rafael J. Wysocki]

On Monday 31 August 2009, Rafael J. Wysocki wrote:
> On Monday 31 August 2009, Ingo Molnar wrote:
...
> > > > Hm, these CPP macros are rather ugly. Why is there a need for 
> > > > the TIMER_DECLARE() wrapper - if a proper inline function is 
> > > > used there's no need for that.
> > > 
> > > I need a variable to be declared so that I can save the "start" 
> > > timestamp in it.  I don't need the variable if DEBUG is unset, 
> > > though.
> > > 
> > > How would you do that without using a macro?  Or #ifdef #endif 
> > > block that would be uglier IMO (which is why I didn't do that)?
> > 
> > Well, why not use an inline function like i suggested? [which does 
> > nothing in the !enabled case] You can keep the local variable always 
> > defined.
> 
> Well, I used the macros _exactly_ because I didn't want to keep the local
> variable always defined.
> 
> Now, if you think that adding several useless bytes to the stack frame is OK,
> perhaps it can be always defined.  However, IMnsHO that would be
> (a) confusing (why define a variable you don't use) and (b) wasteful.
> 
> Perhaps the names of the macros should be directly related to debugging?

Something like in the patch below, maybe?

Best,
Rafael

---
From: Rafael J. Wysocki <rjw@sisk.pl>
Subject: PM: Measure suspend and resume times for individual devices

If verbose PM debugging is enabled, measure and print the time of
suspending and resuming of individual devices.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/base/power/main.c |   51 +++++++++++++++++++++++++++++++++++++++++-----
 kernel/power/swsusp.c     |    2 -
 2 files changed, 47 insertions(+), 6 deletions(-)

Index: linux-2.6/drivers/base/power/main.c
===================================================================
--- linux-2.6.orig/drivers/base/power/main.c
+++ linux-2.6/drivers/base/power/main.c
@@ -442,11 +442,11 @@ static bool pm_op_started(struct device 
  */
 int pm_time_elapsed(struct timeval *start, struct timeval *stop)
 {
-	s64 elapsed_centisecs64;
+	s64 elapsed_msecs64;
 
-	elapsed_centisecs64 = timeval_to_ns(stop) - timeval_to_ns(start);
-	do_div(elapsed_centisecs64, NSEC_PER_SEC / 100);
-	return elapsed_centisecs64;
+	elapsed_msecs64 = timeval_to_ns(stop) - timeval_to_ns(start);
+	do_div(elapsed_msecs64, NSEC_PER_SEC / 1000);
+	return elapsed_msecs64;
 }
 
 static char *pm_verb(int event)
@@ -476,7 +476,7 @@ static char *pm_verb(int event)
 static void dpm_show_time(struct timeval *start, struct timeval *stop,
 			  pm_message_t state, const char *info)
 {
-	int centisecs = pm_time_elapsed(start, stop);
+	int centisecs = pm_time_elapsed(start, stop) / 10;
 
 	printk(KERN_INFO "PM: %s%s%s of devices complete in %d.%02d seconds\n",
 		info ? info : "", info ? " " : "", pm_verb(state.event),
@@ -497,6 +497,33 @@ static void pm_dev_err(struct device *de
 		kobject_name(&dev->kobj), pm_verb(state.event), info, error);
 }
 
+#ifdef DEBUG
+static void device_show_time(struct timeval *start, struct device *dev,
+			     pm_message_t state, char *info)
+{
+	struct timeval stop;
+	int msecs;
+
+	do_gettimeofday(&stop);
+	msecs = pm_time_elapsed(start, &stop);
+	dev_dbg(dev, "PID %d: %s%s%s complete in %d.%03d seconds\n",
+		task_pid_nr(current), info ? info : "", info ? " " : "",
+		pm_verb(state.event), msecs / 1000, msecs % 1000);
+}
+
+#define DEBUG_TIMEVAL(tv)	struct timeval tv
+#define DEBUG_TIME_START(tv)	do { \
+		do_gettimeofday(&tv); \
+	} while (0)
+#define DEBUG_TIME_SHOW(tv, dev, state, info)	do { \
+		device_show_time(&tv, dev, state, info); \
+	} while (0)
+#else /* !DEBUG */
+#define DEBUG_TIMEVAL(tv)
+#define DEBUG_TIME_START(tv)	do { } while (0)
+#define DEBUG_TIME_SHOW(tv, dev, state, info)	do { } while (0)
+#endif /* !DEBUG */
+
 /*------------------------- Resume routines -------------------------*/
 
 /**
@@ -510,7 +537,9 @@ static void pm_dev_err(struct device *de
 static int __device_resume_noirq(struct device *dev, pm_message_t state)
 {
 	int error = 0;
+	DEBUG_TIMEVAL(start);
 
+	DEBUG_TIME_START(start);
 	TRACE_DEVICE(dev);
 	TRACE_RESUME(0);
 
@@ -523,6 +552,7 @@ static int __device_resume_noirq(struct 
 	wake_up_all(&dev->power.wait_queue);
 
 	TRACE_RESUME(error);
+	DEBUG_TIME_SHOW(start, dev, state, "EARLY");
 	return error;
 }
 
@@ -639,7 +669,9 @@ EXPORT_SYMBOL_GPL(dpm_resume_noirq);
 static int __device_resume(struct device *dev, pm_message_t state)
 {
 	int error = 0;
+	DEBUG_TIMEVAL(start);
 
+	DEBUG_TIME_START(start);
 	TRACE_DEVICE(dev);
 	TRACE_RESUME(0);
 
@@ -681,6 +713,7 @@ static int __device_resume(struct device
 	wake_up_all(&dev->power.wait_queue);
 
 	TRACE_RESUME(error);
+	DEBUG_TIME_SHOW(start, dev, state, NULL);
 	return error;
 }
 
@@ -923,6 +956,9 @@ static pm_message_t resume_event(pm_mess
 static int __device_suspend_noirq(struct device *dev, pm_message_t state)
 {
 	int error = 0;
+	DEBUG_TIMEVAL(start);
+
+	DEBUG_TIME_START(start);
 
 	if (dev->bus && dev->bus->pm) {
 		pm_dev_dbg(dev, state, "LATE ");
@@ -932,6 +968,7 @@ static int __device_suspend_noirq(struct
 	dev->power.op_complete = true;
 	wake_up_all(&dev->power.wait_queue);
 
+	DEBUG_TIME_SHOW(start, dev, state, "LATE");
 	return error;
 }
 
@@ -1063,6 +1100,9 @@ EXPORT_SYMBOL_GPL(dpm_suspend_noirq);
 static int __device_suspend(struct device *dev, pm_message_t state)
 {
 	int error = 0;
+	DEBUG_TIMEVAL(start);
+
+	DEBUG_TIME_START(start);
 
 	down(&dev->sem);
 
@@ -1103,6 +1143,7 @@ static int __device_suspend(struct devic
 	dev->power.op_complete = true;
 	wake_up_all(&dev->power.wait_queue);
 
+	DEBUG_TIME_SHOW(start, dev, state, NULL);
 	return error;
 }
 
Index: linux-2.6/kernel/power/swsusp.c
===================================================================
--- linux-2.6.orig/kernel/power/swsusp.c
+++ linux-2.6/kernel/power/swsusp.c
@@ -169,7 +169,7 @@ int swsusp_swap_in_use(void)
 void swsusp_show_speed(struct timeval *start, struct timeval *stop,
 			unsigned nr_pages, char *msg)
 {
-	int centisecs = pm_time_elapsed(start, stop);
+	int centisecs = pm_time_elapsed(start, stop) / 10;
 	int k;
 	int kps;
 

Re: [PATCH 10] PM: Measure suspend and resume times for individual devices (was: Re: [PATCH 2/6] PM: Asynchronous resume of devices)

[Ingo Molnar]

* Rafael J. Wysocki <rjw@sisk.pl> wrote:

> On Monday 31 August 2009, Ingo Molnar wrote:
> > 
> > * Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > 
> > > On Monday 31 August 2009, Ingo Molnar wrote:
> > > > 
> > > > * Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > > > 
> > > > > On Sunday 30 August 2009, Rafael J. Wysocki wrote:
> > > > > > On Sunday 30 August 2009, Alan Stern wrote:
> > > > > > > On Sat, 29 Aug 2009, Rafael J. Wysocki wrote:
> > > > > > > 
> > > > > > > > I only wanted to say that the advantage is not really that "big". :-)
> > > > > > > > 
> > > > > > > > > I must agree, 14 threads isn't a lot.  But at the moment that number is 
> > > > > > > > > random, not under your control.
> > > > > > > > 
> > > > > > > > It's not directly controlled, but there are some interactions between the
> > > > > > > > async threads, the main threads and the async framework that don't allow this
> > > > > > > > number to grow too much.
> > > > > > > > 
> > > > > > > > IMO it sometimes is better to allow things to work themselves out, as long as
> > > > > > > > they don't explode, than to try to keep everything under strict control.  YMMV.
> > > > > > > 
> > > > > > > For testing purposes it would be nice to have a one-line summary for
> > > > > > > each device containing a thread ID, start timestamp, end timestamp, and
> > > > > > > elapsed time.  With that information you could evaluate the amount of
> > > > > > > parallelism and determine where the bottlenecks are.  It would give a
> > > > > > > much more detailed picture of the entire process than the total time of
> > > > > > > your recent patch 9.
> > > > > > 
> > > > > > Of course it would.  I think I'll implement it.
> > > > > 
> > > > > OK, below is a patch for that.  It only prints the time elapsed, because the
> > > > > timestamps themselves can be obtained from the usual kernel timestamping.
> > > > > 
> > > > > It's on top of all the previous patches.
> > > > > 
> > > > > Thanks,
> > > > > Rafael
> > > > > 
> > > > > ---
> > > > > From: Rafael J. Wysocki <rjw@sisk.pl>
> > > > > Subject: PM: Measure suspend and resume times for individual devices
> > > > > 
> > > > > If verbose PM debugging is enabled, measure and print the time of
> > > > > suspending and resuming of individual devices.
> > > > > 
> > > > > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> > > > > ---
> > > > >  drivers/base/power/main.c |   51 +++++++++++++++++++++++++++++++++++++++++-----
> > > > >  kernel/power/swsusp.c     |    2 -
> > > > >  2 files changed, 47 insertions(+), 6 deletions(-)
> > > > > 
> > > > > Index: linux-2.6/drivers/base/power/main.c
> > > > > ===================================================================
> > > > > --- linux-2.6.orig/drivers/base/power/main.c
> > > > > +++ linux-2.6/drivers/base/power/main.c
> > > > > @@ -442,11 +442,11 @@ static bool pm_op_started(struct device 
> > > > >   */
> > > > >  int pm_time_elapsed(struct timeval *start, struct timeval *stop)
> > > > >  {
> > > > > -	s64 elapsed_centisecs64;
> > > > > +	s64 elapsed_msecs64;
> > > > >  
> > > > > -	elapsed_centisecs64 = timeval_to_ns(stop) - timeval_to_ns(start);
> > > > > -	do_div(elapsed_centisecs64, NSEC_PER_SEC / 100);
> > > > > -	return elapsed_centisecs64;
> > > > > +	elapsed_msecs64 = timeval_to_ns(stop) - timeval_to_ns(start);
> > > > > +	do_div(elapsed_msecs64, NSEC_PER_SEC / 1000);
> > > > > +	return elapsed_msecs64;
> > > > >  }
> > > > >  
> > > > >  static char *pm_verb(int event)
> > > > > @@ -476,7 +476,7 @@ static char *pm_verb(int event)
> > > > >  static void dpm_show_time(struct timeval *start, struct timeval *stop,
> > > > >  			  pm_message_t state, const char *info)
> > > > >  {
> > > > > -	int centisecs = pm_time_elapsed(start, stop);
> > > > > +	int centisecs = pm_time_elapsed(start, stop) / 10;
> > > > >  
> > > > >  	printk(KERN_INFO "PM: %s%s%s of devices complete in %d.%02d seconds\n",
> > > > >  		info ? info : "", info ? " " : "", pm_verb(state.event),
> > > > > @@ -497,6 +497,33 @@ static void pm_dev_err(struct device *de
> > > > >  		kobject_name(&dev->kobj), pm_verb(state.event), info, error);
> > > > >  }
> > > > >  
> > > > > +#ifdef DEBUG
> > > > > +static void device_show_time(struct timeval *start, struct device *dev,
> > > > > +			     pm_message_t state, char *info)
> > > > > +{
> > > > > +	struct timeval stop;
> > > > > +	int msecs;
> > > > > +
> > > > > +	do_gettimeofday(&stop);
> > > > > +	msecs = pm_time_elapsed(start, &stop);
> > > > > +	dev_dbg(dev, "PID %d: %s%s%s complete in %d.%03d seconds\n",
> > > > > +		task_pid_nr(current), info ? info : "", info ? " " : "",
> > > > > +		pm_verb(state.event), msecs / 1000, msecs % 1000);
> > > > > +}
> > > > > +
> > > > > +#define TIMER_DECLARE(timer)	struct timeval timer
> > > > > +#define TIMER_START(timer)	do { \
> > > > > +		do_gettimeofday(&timer); \
> > > > > +	} while (0)
> > > > > +#define TIMER_STOP(timer, dev, state, info)	do { \
> > > > > +		device_show_time(&timer, dev, state, info); \
> > > > > +	} while (0)
> > > > > +#else /* !DEBUG */
> > > > > +#define TIMER_DECLARE(timer)
> > > > > +#define TIMER_START(timer)	do { } while (0)
> > > > > +#define TIMER_STOP(timer, dev, state, info)	do { } while (0)
> > > > > +#endif /* !DEBUG */
> > > > > +
> > > > >  /*------------------------- Resume routines -------------------------*/
> > > > >  
> > > > >  /**
> > > > > @@ -510,7 +537,9 @@ static void pm_dev_err(struct device *de
> > > > >  static int __device_resume_noirq(struct device *dev, pm_message_t state)
> > > > >  {
> > > > >  	int error = 0;
> > > > > +	TIMER_DECLARE(timer);
> > > > >  
> > > > > +	TIMER_START(timer);
> > > > >  	TRACE_DEVICE(dev);
> > > > >  	TRACE_RESUME(0);
> > > > >  
> > > > > @@ -523,6 +552,7 @@ static int __device_resume_noirq(struct 
> > > > >  	wake_up_all(&dev->power.wait_queue);
> > > > >  
> > > > >  	TRACE_RESUME(error);
> > > > > +	TIMER_STOP(timer, dev, state, "EARLY");
> > > > >  	return error;
> > > > 
> > > > Hm, these CPP macros are rather ugly. Why is there a need for 
> > > > the TIMER_DECLARE() wrapper - if a proper inline function is 
> > > > used there's no need for that.
> > > 
> > > I need a variable to be declared so that I can save the "start" 
> > > timestamp in it.  I don't need the variable if DEBUG is unset, 
> > > though.
> > > 
> > > How would you do that without using a macro?  Or #ifdef #endif 
> > > block that would be uglier IMO (which is why I didn't do that)?
> > 
> > Well, why not use an inline function like i suggested? [which does 
> > nothing in the !enabled case] You can keep the local variable always 
> > defined.
> 
> Well, I used the macros _exactly_ because I didn't want to keep 
> the local variable always defined.
> 
> Now, if you think that adding several useless bytes to the stack 
> frame is OK, perhaps it can be always defined.  However, IMnsHO 
> that would be (a) confusing (why define a variable you don't use) 
> and (b) wasteful.

Well the compiler will eliminate them, doesnt it?

	Ingo

Re: [PATCH 10] PM: Measure suspend and resume times for individual devices (was: Re: [PATCH 2/6] PM: Asynchronous resume of devices)

[Rafael J. Wysocki]

On Friday 04 September 2009, Ingo Molnar wrote:
> 
> * Rafael J. Wysocki <rjw@sisk.pl> wrote:
> 
> > On Monday 31 August 2009, Ingo Molnar wrote:
> > > 
> > > * Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > > 
> > > > On Monday 31 August 2009, Ingo Molnar wrote:
> > > > > 
> > > > > * Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > > > > 
> > > > > > On Sunday 30 August 2009, Rafael J. Wysocki wrote:
> > > > > > > On Sunday 30 August 2009, Alan Stern wrote:
> > > > > > > > On Sat, 29 Aug 2009, Rafael J. Wysocki wrote:
> > > > > > > > 
> > > > > > > > > I only wanted to say that the advantage is not really that "big". :-)
> > > > > > > > > 
> > > > > > > > > > I must agree, 14 threads isn't a lot.  But at the moment that number is 
> > > > > > > > > > random, not under your control.
> > > > > > > > > 
> > > > > > > > > It's not directly controlled, but there are some interactions between the
> > > > > > > > > async threads, the main threads and the async framework that don't allow this
> > > > > > > > > number to grow too much.
> > > > > > > > > 
> > > > > > > > > IMO it sometimes is better to allow things to work themselves out, as long as
> > > > > > > > > they don't explode, than to try to keep everything under strict control.  YMMV.
> > > > > > > > 
> > > > > > > > For testing purposes it would be nice to have a one-line summary for
> > > > > > > > each device containing a thread ID, start timestamp, end timestamp, and
> > > > > > > > elapsed time.  With that information you could evaluate the amount of
> > > > > > > > parallelism and determine where the bottlenecks are.  It would give a
> > > > > > > > much more detailed picture of the entire process than the total time of
> > > > > > > > your recent patch 9.
> > > > > > > 
> > > > > > > Of course it would.  I think I'll implement it.
> > > > > > 
> > > > > > OK, below is a patch for that.  It only prints the time elapsed, because the
> > > > > > timestamps themselves can be obtained from the usual kernel timestamping.
> > > > > > 
> > > > > > It's on top of all the previous patches.
> > > > > > 
> > > > > > Thanks,
> > > > > > Rafael
> > > > > > 
> > > > > > ---
> > > > > > From: Rafael J. Wysocki <rjw@sisk.pl>
> > > > > > Subject: PM: Measure suspend and resume times for individual devices
> > > > > > 
> > > > > > If verbose PM debugging is enabled, measure and print the time of
> > > > > > suspending and resuming of individual devices.
> > > > > > 
> > > > > > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> > > > > > ---
> > > > > >  drivers/base/power/main.c |   51 +++++++++++++++++++++++++++++++++++++++++-----
> > > > > >  kernel/power/swsusp.c     |    2 -
> > > > > >  2 files changed, 47 insertions(+), 6 deletions(-)
> > > > > > 
> > > > > > Index: linux-2.6/drivers/base/power/main.c
> > > > > > ===================================================================
> > > > > > --- linux-2.6.orig/drivers/base/power/main.c
> > > > > > +++ linux-2.6/drivers/base/power/main.c
> > > > > > @@ -442,11 +442,11 @@ static bool pm_op_started(struct device 
> > > > > >   */
> > > > > >  int pm_time_elapsed(struct timeval *start, struct timeval *stop)
> > > > > >  {
> > > > > > -	s64 elapsed_centisecs64;
> > > > > > +	s64 elapsed_msecs64;
> > > > > >  
> > > > > > -	elapsed_centisecs64 = timeval_to_ns(stop) - timeval_to_ns(start);
> > > > > > -	do_div(elapsed_centisecs64, NSEC_PER_SEC / 100);
> > > > > > -	return elapsed_centisecs64;
> > > > > > +	elapsed_msecs64 = timeval_to_ns(stop) - timeval_to_ns(start);
> > > > > > +	do_div(elapsed_msecs64, NSEC_PER_SEC / 1000);
> > > > > > +	return elapsed_msecs64;
> > > > > >  }
> > > > > >  
> > > > > >  static char *pm_verb(int event)
> > > > > > @@ -476,7 +476,7 @@ static char *pm_verb(int event)
> > > > > >  static void dpm_show_time(struct timeval *start, struct timeval *stop,
> > > > > >  			  pm_message_t state, const char *info)
> > > > > >  {
> > > > > > -	int centisecs = pm_time_elapsed(start, stop);
> > > > > > +	int centisecs = pm_time_elapsed(start, stop) / 10;
> > > > > >  
> > > > > >  	printk(KERN_INFO "PM: %s%s%s of devices complete in %d.%02d seconds\n",
> > > > > >  		info ? info : "", info ? " " : "", pm_verb(state.event),
> > > > > > @@ -497,6 +497,33 @@ static void pm_dev_err(struct device *de
> > > > > >  		kobject_name(&dev->kobj), pm_verb(state.event), info, error);
> > > > > >  }
> > > > > >  
> > > > > > +#ifdef DEBUG
> > > > > > +static void device_show_time(struct timeval *start, struct device *dev,
> > > > > > +			     pm_message_t state, char *info)
> > > > > > +{
> > > > > > +	struct timeval stop;
> > > > > > +	int msecs;
> > > > > > +
> > > > > > +	do_gettimeofday(&stop);
> > > > > > +	msecs = pm_time_elapsed(start, &stop);
> > > > > > +	dev_dbg(dev, "PID %d: %s%s%s complete in %d.%03d seconds\n",
> > > > > > +		task_pid_nr(current), info ? info : "", info ? " " : "",
> > > > > > +		pm_verb(state.event), msecs / 1000, msecs % 1000);
> > > > > > +}
> > > > > > +
> > > > > > +#define TIMER_DECLARE(timer)	struct timeval timer
> > > > > > +#define TIMER_START(timer)	do { \
> > > > > > +		do_gettimeofday(&timer); \
> > > > > > +	} while (0)
> > > > > > +#define TIMER_STOP(timer, dev, state, info)	do { \
> > > > > > +		device_show_time(&timer, dev, state, info); \
> > > > > > +	} while (0)
> > > > > > +#else /* !DEBUG */
> > > > > > +#define TIMER_DECLARE(timer)
> > > > > > +#define TIMER_START(timer)	do { } while (0)
> > > > > > +#define TIMER_STOP(timer, dev, state, info)	do { } while (0)
> > > > > > +#endif /* !DEBUG */
> > > > > > +
> > > > > >  /*------------------------- Resume routines -------------------------*/
> > > > > >  
> > > > > >  /**
> > > > > > @@ -510,7 +537,9 @@ static void pm_dev_err(struct device *de
> > > > > >  static int __device_resume_noirq(struct device *dev, pm_message_t state)
> > > > > >  {
> > > > > >  	int error = 0;
> > > > > > +	TIMER_DECLARE(timer);
> > > > > >  
> > > > > > +	TIMER_START(timer);
> > > > > >  	TRACE_DEVICE(dev);
> > > > > >  	TRACE_RESUME(0);
> > > > > >  
> > > > > > @@ -523,6 +552,7 @@ static int __device_resume_noirq(struct 
> > > > > >  	wake_up_all(&dev->power.wait_queue);
> > > > > >  
> > > > > >  	TRACE_RESUME(error);
> > > > > > +	TIMER_STOP(timer, dev, state, "EARLY");
> > > > > >  	return error;
> > > > > 
> > > > > Hm, these CPP macros are rather ugly. Why is there a need for 
> > > > > the TIMER_DECLARE() wrapper - if a proper inline function is 
> > > > > used there's no need for that.
> > > > 
> > > > I need a variable to be declared so that I can save the "start" 
> > > > timestamp in it.  I don't need the variable if DEBUG is unset, 
> > > > though.
> > > > 
> > > > How would you do that without using a macro?  Or #ifdef #endif 
> > > > block that would be uglier IMO (which is why I didn't do that)?
> > > 
> > > Well, why not use an inline function like i suggested? [which does 
> > > nothing in the !enabled case] You can keep the local variable always 
> > > defined.
> > 
> > Well, I used the macros _exactly_ because I didn't want to keep 
> > the local variable always defined.
> > 
> > Now, if you think that adding several useless bytes to the stack 
> > frame is OK, perhaps it can be always defined.  However, IMnsHO 
> > that would be (a) confusing (why define a variable you don't use) 
> > and (b) wasteful.
> 
> Well the compiler will eliminate them, doesnt it?

If the compiler is guaranteed to eliminate them (without generating a "variable
defined but not used" warning for that matter), then they can be always
defined.

Thanks,
Rafael

Re: [PATCH 10] PM: Measure suspend and resume times for individual devices (was: Re: [PATCH 2/6] PM: Asynchronous resume of devices)

[Ingo Molnar]

* Rafael J. Wysocki <rjw@sisk.pl> wrote:

> On Friday 04 September 2009, Ingo Molnar wrote:
> > 
> > * Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > 
> > > On Monday 31 August 2009, Ingo Molnar wrote:
> > > > 
> > > > * Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > > > 
> > > > > On Monday 31 August 2009, Ingo Molnar wrote:
> > > > > > 
> > > > > > * Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > > > > > 
> > > > > > > On Sunday 30 August 2009, Rafael J. Wysocki wrote:
> > > > > > > > On Sunday 30 August 2009, Alan Stern wrote:
> > > > > > > > > On Sat, 29 Aug 2009, Rafael J. Wysocki wrote:
> > > > > > > > > 
> > > > > > > > > > I only wanted to say that the advantage is not really that "big". :-)
> > > > > > > > > > 
> > > > > > > > > > > I must agree, 14 threads isn't a lot.  But at the moment that number is 
> > > > > > > > > > > random, not under your control.
> > > > > > > > > > 
> > > > > > > > > > It's not directly controlled, but there are some interactions between the
> > > > > > > > > > async threads, the main threads and the async framework that don't allow this
> > > > > > > > > > number to grow too much.
> > > > > > > > > > 
> > > > > > > > > > IMO it sometimes is better to allow things to work themselves out, as long as
> > > > > > > > > > they don't explode, than to try to keep everything under strict control.  YMMV.
> > > > > > > > > 
> > > > > > > > > For testing purposes it would be nice to have a one-line summary for
> > > > > > > > > each device containing a thread ID, start timestamp, end timestamp, and
> > > > > > > > > elapsed time.  With that information you could evaluate the amount of
> > > > > > > > > parallelism and determine where the bottlenecks are.  It would give a
> > > > > > > > > much more detailed picture of the entire process than the total time of
> > > > > > > > > your recent patch 9.
> > > > > > > > 
> > > > > > > > Of course it would.  I think I'll implement it.
> > > > > > > 
> > > > > > > OK, below is a patch for that.  It only prints the time elapsed, because the
> > > > > > > timestamps themselves can be obtained from the usual kernel timestamping.
> > > > > > > 
> > > > > > > It's on top of all the previous patches.
> > > > > > > 
> > > > > > > Thanks,
> > > > > > > Rafael
> > > > > > > 
> > > > > > > ---
> > > > > > > From: Rafael J. Wysocki <rjw@sisk.pl>
> > > > > > > Subject: PM: Measure suspend and resume times for individual devices
> > > > > > > 
> > > > > > > If verbose PM debugging is enabled, measure and print the time of
> > > > > > > suspending and resuming of individual devices.
> > > > > > > 
> > > > > > > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> > > > > > > ---
> > > > > > >  drivers/base/power/main.c |   51 +++++++++++++++++++++++++++++++++++++++++-----
> > > > > > >  kernel/power/swsusp.c     |    2 -
> > > > > > >  2 files changed, 47 insertions(+), 6 deletions(-)
> > > > > > > 
> > > > > > > Index: linux-2.6/drivers/base/power/main.c
> > > > > > > ===================================================================
> > > > > > > --- linux-2.6.orig/drivers/base/power/main.c
> > > > > > > +++ linux-2.6/drivers/base/power/main.c
> > > > > > > @@ -442,11 +442,11 @@ static bool pm_op_started(struct device 
> > > > > > >   */
> > > > > > >  int pm_time_elapsed(struct timeval *start, struct timeval *stop)
> > > > > > >  {
> > > > > > > -	s64 elapsed_centisecs64;
> > > > > > > +	s64 elapsed_msecs64;
> > > > > > >  
> > > > > > > -	elapsed_centisecs64 = timeval_to_ns(stop) - timeval_to_ns(start);
> > > > > > > -	do_div(elapsed_centisecs64, NSEC_PER_SEC / 100);
> > > > > > > -	return elapsed_centisecs64;
> > > > > > > +	elapsed_msecs64 = timeval_to_ns(stop) - timeval_to_ns(start);
> > > > > > > +	do_div(elapsed_msecs64, NSEC_PER_SEC / 1000);
> > > > > > > +	return elapsed_msecs64;
> > > > > > >  }
> > > > > > >  
> > > > > > >  static char *pm_verb(int event)
> > > > > > > @@ -476,7 +476,7 @@ static char *pm_verb(int event)
> > > > > > >  static void dpm_show_time(struct timeval *start, struct timeval *stop,
> > > > > > >  			  pm_message_t state, const char *info)
> > > > > > >  {
> > > > > > > -	int centisecs = pm_time_elapsed(start, stop);
> > > > > > > +	int centisecs = pm_time_elapsed(start, stop) / 10;
> > > > > > >  
> > > > > > >  	printk(KERN_INFO "PM: %s%s%s of devices complete in %d.%02d seconds\n",
> > > > > > >  		info ? info : "", info ? " " : "", pm_verb(state.event),
> > > > > > > @@ -497,6 +497,33 @@ static void pm_dev_err(struct device *de
> > > > > > >  		kobject_name(&dev->kobj), pm_verb(state.event), info, error);
> > > > > > >  }
> > > > > > >  
> > > > > > > +#ifdef DEBUG
> > > > > > > +static void device_show_time(struct timeval *start, struct device *dev,
> > > > > > > +			     pm_message_t state, char *info)
> > > > > > > +{
> > > > > > > +	struct timeval stop;
> > > > > > > +	int msecs;
> > > > > > > +
> > > > > > > +	do_gettimeofday(&stop);
> > > > > > > +	msecs = pm_time_elapsed(start, &stop);
> > > > > > > +	dev_dbg(dev, "PID %d: %s%s%s complete in %d.%03d seconds\n",
> > > > > > > +		task_pid_nr(current), info ? info : "", info ? " " : "",
> > > > > > > +		pm_verb(state.event), msecs / 1000, msecs % 1000);
> > > > > > > +}
> > > > > > > +
> > > > > > > +#define TIMER_DECLARE(timer)	struct timeval timer
> > > > > > > +#define TIMER_START(timer)	do { \
> > > > > > > +		do_gettimeofday(&timer); \
> > > > > > > +	} while (0)
> > > > > > > +#define TIMER_STOP(timer, dev, state, info)	do { \
> > > > > > > +		device_show_time(&timer, dev, state, info); \
> > > > > > > +	} while (0)
> > > > > > > +#else /* !DEBUG */
> > > > > > > +#define TIMER_DECLARE(timer)
> > > > > > > +#define TIMER_START(timer)	do { } while (0)
> > > > > > > +#define TIMER_STOP(timer, dev, state, info)	do { } while (0)
> > > > > > > +#endif /* !DEBUG */
> > > > > > > +
> > > > > > >  /*------------------------- Resume routines -------------------------*/
> > > > > > >  
> > > > > > >  /**
> > > > > > > @@ -510,7 +537,9 @@ static void pm_dev_err(struct device *de
> > > > > > >  static int __device_resume_noirq(struct device *dev, pm_message_t state)
> > > > > > >  {
> > > > > > >  	int error = 0;
> > > > > > > +	TIMER_DECLARE(timer);
> > > > > > >  
> > > > > > > +	TIMER_START(timer);
> > > > > > >  	TRACE_DEVICE(dev);
> > > > > > >  	TRACE_RESUME(0);
> > > > > > >  
> > > > > > > @@ -523,6 +552,7 @@ static int __device_resume_noirq(struct 
> > > > > > >  	wake_up_all(&dev->power.wait_queue);
> > > > > > >  
> > > > > > >  	TRACE_RESUME(error);
> > > > > > > +	TIMER_STOP(timer, dev, state, "EARLY");
> > > > > > >  	return error;
> > > > > > 
> > > > > > Hm, these CPP macros are rather ugly. Why is there a need for 
> > > > > > the TIMER_DECLARE() wrapper - if a proper inline function is 
> > > > > > used there's no need for that.
> > > > > 
> > > > > I need a variable to be declared so that I can save the "start" 
> > > > > timestamp in it.  I don't need the variable if DEBUG is unset, 
> > > > > though.
> > > > > 
> > > > > How would you do that without using a macro?  Or #ifdef #endif 
> > > > > block that would be uglier IMO (which is why I didn't do that)?
> > > > 
> > > > Well, why not use an inline function like i suggested? [which does 
> > > > nothing in the !enabled case] You can keep the local variable always 
> > > > defined.
> > > 
> > > Well, I used the macros _exactly_ because I didn't want to keep 
> > > the local variable always defined.
> > > 
> > > Now, if you think that adding several useless bytes to the stack 
> > > frame is OK, perhaps it can be always defined.  However, IMnsHO 
> > > that would be (a) confusing (why define a variable you don't use) 
> > > and (b) wasteful.
> > 
> > Well the compiler will eliminate them, doesnt it?
> 
> If the compiler is guaranteed to eliminate them (without 
> generating a "variable defined but not used" warning for that 
> matter), then they can be always defined.

Why should such a warning be generated if what makes use of it is an 
inline function?

(it would be generated if it's a macro.)

	Ingo

[PATCH 10 update 2x] PM: Measure suspend and resume times for individual devices

[Rafael J. Wysocki]

On Friday 04 September 2009, Ingo Molnar wrote:
> 
> * Rafael J. Wysocki <rjw@sisk.pl> wrote:
> 
> > On Friday 04 September 2009, Ingo Molnar wrote:
> > > 
> > > * Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > > 
> > > > On Monday 31 August 2009, Ingo Molnar wrote:
> > > > > 
> > > > > * Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > > > > 
> > > > > > On Monday 31 August 2009, Ingo Molnar wrote:
> > > > > > > 
> > > > > > > * Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > > > > > > 
> > > > > > > > On Sunday 30 August 2009, Rafael J. Wysocki wrote:
> > > > > > > > > On Sunday 30 August 2009, Alan Stern wrote:
> > > > > > > > > > On Sat, 29 Aug 2009, Rafael J. Wysocki wrote:
> > > > > > > > > > 
> > > > > > > > > > > I only wanted to say that the advantage is not really that "big". :-)
> > > > > > > > > > > 
> > > > > > > > > > > > I must agree, 14 threads isn't a lot.  But at the moment that number is 
> > > > > > > > > > > > random, not under your control.
> > > > > > > > > > > 
> > > > > > > > > > > It's not directly controlled, but there are some interactions between the
> > > > > > > > > > > async threads, the main threads and the async framework that don't allow this
> > > > > > > > > > > number to grow too much.
> > > > > > > > > > > 
> > > > > > > > > > > IMO it sometimes is better to allow things to work themselves out, as long as
> > > > > > > > > > > they don't explode, than to try to keep everything under strict control.  YMMV.
> > > > > > > > > > 
> > > > > > > > > > For testing purposes it would be nice to have a one-line summary for
> > > > > > > > > > each device containing a thread ID, start timestamp, end timestamp, and
> > > > > > > > > > elapsed time.  With that information you could evaluate the amount of
> > > > > > > > > > parallelism and determine where the bottlenecks are.  It would give a
> > > > > > > > > > much more detailed picture of the entire process than the total time of
> > > > > > > > > > your recent patch 9.
> > > > > > > > > 
> > > > > > > > > Of course it would.  I think I'll implement it.
> > > > > > > > 
> > > > > > > > OK, below is a patch for that.  It only prints the time elapsed, because the
> > > > > > > > timestamps themselves can be obtained from the usual kernel timestamping.
> > > > > > > > 
> > > > > > > > It's on top of all the previous patches.
> > > > > > > > 
> > > > > > > > Thanks,
> > > > > > > > Rafael
> > > > > > > > 
> > > > > > > > ---
> > > > > > > > From: Rafael J. Wysocki <rjw@sisk.pl>
> > > > > > > > Subject: PM: Measure suspend and resume times for individual devices
> > > > > > > > 
> > > > > > > > If verbose PM debugging is enabled, measure and print the time of
> > > > > > > > suspending and resuming of individual devices.
> > > > > > > > 
> > > > > > > > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> > > > > > > > ---
> > > > > > > >  drivers/base/power/main.c |   51 +++++++++++++++++++++++++++++++++++++++++-----
> > > > > > > >  kernel/power/swsusp.c     |    2 -
> > > > > > > >  2 files changed, 47 insertions(+), 6 deletions(-)
> > > > > > > > 
> > > > > > > > Index: linux-2.6/drivers/base/power/main.c
> > > > > > > > ===================================================================
> > > > > > > > --- linux-2.6.orig/drivers/base/power/main.c
> > > > > > > > +++ linux-2.6/drivers/base/power/main.c
> > > > > > > > @@ -442,11 +442,11 @@ static bool pm_op_started(struct device 
> > > > > > > >   */
> > > > > > > >  int pm_time_elapsed(struct timeval *start, struct timeval *stop)
> > > > > > > >  {
> > > > > > > > -	s64 elapsed_centisecs64;
> > > > > > > > +	s64 elapsed_msecs64;
> > > > > > > >  
> > > > > > > > -	elapsed_centisecs64 = timeval_to_ns(stop) - timeval_to_ns(start);
> > > > > > > > -	do_div(elapsed_centisecs64, NSEC_PER_SEC / 100);
> > > > > > > > -	return elapsed_centisecs64;
> > > > > > > > +	elapsed_msecs64 = timeval_to_ns(stop) - timeval_to_ns(start);
> > > > > > > > +	do_div(elapsed_msecs64, NSEC_PER_SEC / 1000);
> > > > > > > > +	return elapsed_msecs64;
> > > > > > > >  }
> > > > > > > >  
> > > > > > > >  static char *pm_verb(int event)
> > > > > > > > @@ -476,7 +476,7 @@ static char *pm_verb(int event)
> > > > > > > >  static void dpm_show_time(struct timeval *start, struct timeval *stop,
> > > > > > > >  			  pm_message_t state, const char *info)
> > > > > > > >  {
> > > > > > > > -	int centisecs = pm_time_elapsed(start, stop);
> > > > > > > > +	int centisecs = pm_time_elapsed(start, stop) / 10;
> > > > > > > >  
> > > > > > > >  	printk(KERN_INFO "PM: %s%s%s of devices complete in %d.%02d seconds\n",
> > > > > > > >  		info ? info : "", info ? " " : "", pm_verb(state.event),
> > > > > > > > @@ -497,6 +497,33 @@ static void pm_dev_err(struct device *de
> > > > > > > >  		kobject_name(&dev->kobj), pm_verb(state.event), info, error);
> > > > > > > >  }
> > > > > > > >  
> > > > > > > > +#ifdef DEBUG
> > > > > > > > +static void device_show_time(struct timeval *start, struct device *dev,
> > > > > > > > +			     pm_message_t state, char *info)
> > > > > > > > +{
> > > > > > > > +	struct timeval stop;
> > > > > > > > +	int msecs;
> > > > > > > > +
> > > > > > > > +	do_gettimeofday(&stop);
> > > > > > > > +	msecs = pm_time_elapsed(start, &stop);
> > > > > > > > +	dev_dbg(dev, "PID %d: %s%s%s complete in %d.%03d seconds\n",
> > > > > > > > +		task_pid_nr(current), info ? info : "", info ? " " : "",
> > > > > > > > +		pm_verb(state.event), msecs / 1000, msecs % 1000);
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > +#define TIMER_DECLARE(timer)	struct timeval timer
> > > > > > > > +#define TIMER_START(timer)	do { \
> > > > > > > > +		do_gettimeofday(&timer); \
> > > > > > > > +	} while (0)
> > > > > > > > +#define TIMER_STOP(timer, dev, state, info)	do { \
> > > > > > > > +		device_show_time(&timer, dev, state, info); \
> > > > > > > > +	} while (0)
> > > > > > > > +#else /* !DEBUG */
> > > > > > > > +#define TIMER_DECLARE(timer)
> > > > > > > > +#define TIMER_START(timer)	do { } while (0)
> > > > > > > > +#define TIMER_STOP(timer, dev, state, info)	do { } while (0)
> > > > > > > > +#endif /* !DEBUG */
> > > > > > > > +
> > > > > > > >  /*------------------------- Resume routines -------------------------*/
> > > > > > > >  
> > > > > > > >  /**
> > > > > > > > @@ -510,7 +537,9 @@ static void pm_dev_err(struct device *de
> > > > > > > >  static int __device_resume_noirq(struct device *dev, pm_message_t state)
> > > > > > > >  {
> > > > > > > >  	int error = 0;
> > > > > > > > +	TIMER_DECLARE(timer);
> > > > > > > >  
> > > > > > > > +	TIMER_START(timer);
> > > > > > > >  	TRACE_DEVICE(dev);
> > > > > > > >  	TRACE_RESUME(0);
> > > > > > > >  
> > > > > > > > @@ -523,6 +552,7 @@ static int __device_resume_noirq(struct 
> > > > > > > >  	wake_up_all(&dev->power.wait_queue);
> > > > > > > >  
> > > > > > > >  	TRACE_RESUME(error);
> > > > > > > > +	TIMER_STOP(timer, dev, state, "EARLY");
> > > > > > > >  	return error;
> > > > > > > 
> > > > > > > Hm, these CPP macros are rather ugly. Why is there a need for 
> > > > > > > the TIMER_DECLARE() wrapper - if a proper inline function is 
> > > > > > > used there's no need for that.
> > > > > > 
> > > > > > I need a variable to be declared so that I can save the "start" 
> > > > > > timestamp in it.  I don't need the variable if DEBUG is unset, 
> > > > > > though.
> > > > > > 
> > > > > > How would you do that without using a macro?  Or #ifdef #endif 
> > > > > > block that would be uglier IMO (which is why I didn't do that)?
> > > > > 
> > > > > Well, why not use an inline function like i suggested? [which does 
> > > > > nothing in the !enabled case] You can keep the local variable always 
> > > > > defined.
> > > > 
> > > > Well, I used the macros _exactly_ because I didn't want to keep 
> > > > the local variable always defined.
> > > > 
> > > > Now, if you think that adding several useless bytes to the stack 
> > > > frame is OK, perhaps it can be always defined.  However, IMnsHO 
> > > > that would be (a) confusing (why define a variable you don't use) 
> > > > and (b) wasteful.
> > > 
> > > Well the compiler will eliminate them, doesnt it?
> > 
> > If the compiler is guaranteed to eliminate them (without 
> > generating a "variable defined but not used" warning for that 
> > matter), then they can be always defined.
> 
> Why should such a warning be generated if what makes use of it is an 
> inline function?
> 
> (it would be generated if it's a macro.)

OK, updated patch is appended.

Thanks,
Rafael

---
From: Rafael J. Wysocki <rjw@sisk.pl>
Subject: PM: Measure suspend and resume times for individual devices

If verbose PM debugging is enabled, measure and print the time of
suspending and resuming of individual devices.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/base/power/main.c |   50 +++++++++++++++++++++++++++++++++++++++++-----
 kernel/power/swsusp.c     |    2 -
 2 files changed, 46 insertions(+), 6 deletions(-)

Index: linux-2.6/drivers/base/power/main.c
===================================================================
--- linux-2.6.orig/drivers/base/power/main.c
+++ linux-2.6/drivers/base/power/main.c
@@ -442,11 +442,11 @@ static bool pm_op_started(struct device 
  */
 int pm_time_elapsed(struct timeval *start, struct timeval *stop)
 {
-	s64 elapsed_centisecs64;
+	s64 elapsed_msecs64;
 
-	elapsed_centisecs64 = timeval_to_ns(stop) - timeval_to_ns(start);
-	do_div(elapsed_centisecs64, NSEC_PER_SEC / 100);
-	return elapsed_centisecs64;
+	elapsed_msecs64 = timeval_to_ns(stop) - timeval_to_ns(start);
+	do_div(elapsed_msecs64, NSEC_PER_SEC / 1000);
+	return elapsed_msecs64;
 }
 
 static char *pm_verb(int event)
@@ -476,7 +476,7 @@ static char *pm_verb(int event)
 static void dpm_show_time(struct timeval *start, struct timeval *stop,
 			  pm_message_t state, const char *info)
 {
-	int centisecs = pm_time_elapsed(start, stop);
+	int centisecs = pm_time_elapsed(start, stop) / 10;
 
 	printk(KERN_INFO "PM: %s%s%s of devices complete in %d.%02d seconds\n",
 		info ? info : "", info ? " " : "", pm_verb(state.event),
@@ -497,6 +497,32 @@ static void pm_dev_err(struct device *de
 		kobject_name(&dev->kobj), pm_verb(state.event), info, error);
 }
 
+#ifdef DEBUG
+static void dbg_get_time(struct timeval *start)
+{
+	do_gettimeofday(start);
+}
+
+static void dbg_show_time(struct timeval *start, struct device *dev,
+			  pm_message_t state, char *info)
+{
+	struct timeval stop;
+	int msecs;
+
+	do_gettimeofday(&stop);
+	msecs = pm_time_elapsed(start, &stop);
+	dev_dbg(dev, "PID %d: %s%s%s complete in %d.%03d seconds\n",
+		task_pid_nr(current), info ? info : "", info ? " " : "",
+		pm_verb(state.event), msecs / 1000, msecs % 1000);
+}
+
+#else /* !DEBUG */
+static void dbg_get_time(struct timeval *start) {}
+static void dbg_show_time(struct timeval *start, struct device *dev,
+			  pm_message_t state, char *info) {}
+
+#endif /* !DEBUG */
+
 /*------------------------- Resume routines -------------------------*/
 
 /**
@@ -510,7 +536,9 @@ static void pm_dev_err(struct device *de
 static int __device_resume_noirq(struct device *dev, pm_message_t state)
 {
 	int error = 0;
+	struct timeval start;
 
+	dbg_get_time(&start);
 	TRACE_DEVICE(dev);
 	TRACE_RESUME(0);
 
@@ -523,6 +551,7 @@ static int __device_resume_noirq(struct 
 	wake_up_all(&dev->power.wait_queue);
 
 	TRACE_RESUME(error);
+	dbg_show_time(&start, dev, state, "EARLY");
 	return error;
 }
 
@@ -639,7 +668,9 @@ EXPORT_SYMBOL_GPL(dpm_resume_noirq);
 static int __device_resume(struct device *dev, pm_message_t state)
 {
 	int error = 0;
+	struct timeval start;
 
+	dbg_get_time(&start);
 	TRACE_DEVICE(dev);
 	TRACE_RESUME(0);
 
@@ -681,6 +712,7 @@ static int __device_resume(struct device
 	wake_up_all(&dev->power.wait_queue);
 
 	TRACE_RESUME(error);
+	dbg_show_time(&start, dev, state, NULL);
 	return error;
 }
 
@@ -923,6 +955,9 @@ static pm_message_t resume_event(pm_mess
 static int __device_suspend_noirq(struct device *dev, pm_message_t state)
 {
 	int error = 0;
+	struct timeval start;
+
+	dbg_get_time(&start);
 
 	if (dev->bus && dev->bus->pm) {
 		pm_dev_dbg(dev, state, "LATE ");
@@ -932,6 +967,7 @@ static int __device_suspend_noirq(struct
 	dev->power.op_complete = true;
 	wake_up_all(&dev->power.wait_queue);
 
+	dbg_show_time(&start, dev, state, "LATE");
 	return error;
 }
 
@@ -1063,6 +1099,9 @@ EXPORT_SYMBOL_GPL(dpm_suspend_noirq);
 static int __device_suspend(struct device *dev, pm_message_t state)
 {
 	int error = 0;
+	struct timeval start;
+
+	dbg_get_time(&start);
 
 	down(&dev->sem);
 
@@ -1103,6 +1142,7 @@ static int __device_suspend(struct devic
 	dev->power.op_complete = true;
 	wake_up_all(&dev->power.wait_queue);
 
+	dbg_show_time(&start, dev, state, NULL);
 	return error;
 }
 
Index: linux-2.6/kernel/power/swsusp.c
===================================================================
--- linux-2.6.orig/kernel/power/swsusp.c
+++ linux-2.6/kernel/power/swsusp.c
@@ -169,7 +169,7 @@ int swsusp_swap_in_use(void)
 void swsusp_show_speed(struct timeval *start, struct timeval *stop,
 			unsigned nr_pages, char *msg)
 {
-	int centisecs = pm_time_elapsed(start, stop);
+	int centisecs = pm_time_elapsed(start, stop) / 10;
 	int k;
 	int kps;
 

Re: [PATCH 10 update 2x] PM: Measure suspend and resume times for individual devices

[Ingo Molnar]

Thanks Rafael, the code looks a lot more natural IMHO.

this bit:

> +#ifdef DEBUG
> +static void dbg_get_time(struct timeval *start)
> +{
> +	do_gettimeofday(start);
> +}

is a plain wrapper over gettimeofday so you might want to inline 
it. (although GCC will do it too most of the time)

	Ingo

Re: [PATCH 10 update 2x] PM: Measure suspend and resume times for individual devices

[Rafael J. Wysocki]

On Sunday 06 September 2009, Ingo Molnar wrote:
> 
> Thanks Rafael, the code looks a lot more natural IMHO.
> 
> this bit:
> 
> > +#ifdef DEBUG
> > +static void dbg_get_time(struct timeval *start)
> > +{
> > +	do_gettimeofday(start);
> > +}
> 
> is a plain wrapper over gettimeofday so you might want to inline 
> it. (although GCC will do it too most of the time)

I have a rule of not using 'inline' except for in headers.  In this case,
however, it's almost like a header, so I'll follow your suggestion.

Thanks,
Rafael

Re: [PATCH 10] PM: Measure suspend and resume times for individual devices (was: Re: [PATCH 2/6] PM: Asynchronous resume of devices)

[Alan Stern]

On Sun, 30 Aug 2009, Rafael J. Wysocki wrote:

> > > For testing purposes it would be nice to have a one-line summary for
> > > each device containing a thread ID, start timestamp, end timestamp, and
> > > elapsed time.  With that information you could evaluate the amount of
> > > parallelism and determine where the bottlenecks are.  It would give a
> > > much more detailed picture of the entire process than the total time of
> > > your recent patch 9.
> > 
> > Of course it would.  I think I'll implement it.
> 
> OK, below is a patch for that.  It only prints the time elapsed, because the
> timestamps themselves can be obtained from the usual kernel timestamping.

Does that include the start timestamps?  I don't see them anywhere in
the patch.  Without the start timestamps we have no way to know how
much time was spent waiting for dpm_list_mtx or other resources as
opposed to actually carrying out the operation.

Alan Stern

Re: [PATCH 10] PM: Measure suspend and resume times for individual devices (was: Re: [PATCH 2/6] PM: Asynchronous resume of devices)

[Rafael J. Wysocki]

On Monday 31 August 2009, Alan Stern wrote:
> On Sun, 30 Aug 2009, Rafael J. Wysocki wrote:
> 
> > > > For testing purposes it would be nice to have a one-line summary for
> > > > each device containing a thread ID, start timestamp, end timestamp, and
> > > > elapsed time.  With that information you could evaluate the amount of
> > > > parallelism and determine where the bottlenecks are.  It would give a
> > > > much more detailed picture of the entire process than the total time of
> > > > your recent patch 9.
> > > 
> > > Of course it would.  I think I'll implement it.
> > 
> > OK, below is a patch for that.  It only prints the time elapsed, because the
> > timestamps themselves can be obtained from the usual kernel timestamping.
> 
> Does that include the start timestamps?  I don't see them anywhere in
> the patch.  Without the start timestamps we have no way to know how
> much time was spent waiting for dpm_list_mtx or other resources as
> opposed to actually carrying out the operation.

If the callback in question is actually defined, there will be additional debug
printouts before executing it from which we can get the start timestamps.

If the callback is not defined, the time elapsed will be 0 anyway, which is
kind of untinteresting.

Thanks,
Rafael

Re: [PATCH 2/6] PM: Asynchronous resume of devices

[Pavel Machek]

Hi!

> > > > > > The same goes for the noirq versions.
> > > > > 
> > > > > I thought about that, but there are a few things to figure out:
> > > > > - how many threads to start
> > > > 
> > > > That's a tough question.  Right now you start roughly as many threads
> > > > as there are async devices.  That seems like overkill.
> > > 
> > > In fact they are substantially fewer than that, for the following reasons.
> > > 
> > > First, the async framework will not start more than MAX_THREADS threads,
> > > which is 256 at the moment.  This number is less than the number of async
> > > devices to handle on an average system.
> > 
> > Okay, but MAX_THREADS isn't under your control.  Remember also that 
> > each thread takes up some memory, and during hibernation we are in a 
> > memory-constrained situation.
> 
> We keep some extra free memory for things like this.  It's not likely to be
> exhausted by the async threads alone.

What extra memory? You are creating quite a lot of threads. For 256 of
them, it would take cca 2MB... You recently removed code from s2disk
that freed 4MB of extra memory, so you are now essentially relying on
min_free_kbytes -- and even then is not reliable, user might s2disk
under memory pressure. min_free_kbytes is 1MB on my system.

								Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH 2/6] PM: Asynchronous resume of devices

[Rafael J. Wysocki]

On Sunday 30 August 2009, Pavel Machek wrote:
> Hi!
> 
> > > > > > > The same goes for the noirq versions.
> > > > > > 
> > > > > > I thought about that, but there are a few things to figure out:
> > > > > > - how many threads to start
> > > > > 
> > > > > That's a tough question.  Right now you start roughly as many threads
> > > > > as there are async devices.  That seems like overkill.
> > > > 
> > > > In fact they are substantially fewer than that, for the following reasons.
> > > > 
> > > > First, the async framework will not start more than MAX_THREADS threads,
> > > > which is 256 at the moment.  This number is less than the number of async
> > > > devices to handle on an average system.
> > > 
> > > Okay, but MAX_THREADS isn't under your control.  Remember also that 
> > > each thread takes up some memory, and during hibernation we are in a 
> > > memory-constrained situation.
> > 
> > We keep some extra free memory for things like this.  It's not likely to be
> > exhausted by the async threads alone.
> 
> What extra memory? You are creating quite a lot of threads. For 256 of
> them, it would take cca 2MB...

We never start that many threads and even if there's not enough memory to
start a new thread, the async framework will handle that for us.

> You recently removed code from s2disk that freed 4MB of extra memory,

That was removed from s2ram.  For STD we still have PAGES_FOR_IO and
SPARE_PAGES, nothing's changed there.

Thanks,
Rafael

[PATCH 3/6] PM: Asynchronous suspend of devices

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rjw@sisk.pl>

Extend the approach used in the previous patch to the suspend part of
the PM core.

Asynchronous suspend is slightly more complicated, because if any of
the suspend callbacks executed asynchronously returns error code, the
entire suspend has to be terminated and rolled back.  Apart from
this, it's completely analogous to the asynchronous resume.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/base/power/main.c |  225 +++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 214 insertions(+), 11 deletions(-)

Index: linux-2.6/drivers/base/power/main.c
===================================================================
--- linux-2.6.orig/drivers/base/power/main.c
+++ linux-2.6/drivers/base/power/main.c
@@ -240,6 +240,15 @@ static void device_pm_wait_for_masters(s
 }
 
 /**
+ * device_pm_wait_for_slaves - Wait for all slaves of given device.
+ * @slave: Device to wait for the slaves of.
+ */
+static void device_pm_wait_for_slaves(struct device *master)
+{
+	device_for_each_slave(master, master, device_pm_wait_fn);
+}
+
+/**
  * device_pm_check - Check the power.op_complete flag of given device.
  * @dev: Device to check.
  */
@@ -273,6 +282,15 @@ static int device_pm_check_masters(struc
 }
 
 /**
+ * device_pm_check_slaves - Check power.op_complete for slaves of a device.
+ * @slave: Device to check the slaves of.
+ */
+static int device_pm_check_slaves(struct device *master)
+{
+	return device_for_each_slave(master, NULL, device_pm_check_fn);
+}
+
+/**
  * pm_op - Execute the PM operation appropriate for given PM event.
  * @dev: Device to handle.
  * @ops: PM operations to choose from.
@@ -832,6 +850,8 @@ EXPORT_SYMBOL_GPL(dpm_resume_end);
 
 /*------------------------- Suspend routines -------------------------*/
 
+static atomic_t async_error;
+
 /**
  * resume_event - Return a "resume" message for given "suspend" sleep state.
  * @sleep_state: PM message representing a sleep state.
@@ -861,21 +881,100 @@ static pm_message_t resume_event(pm_mess
  * The driver of @dev will not receive interrupts while this function is being
  * executed.
  */
-static int device_suspend_noirq(struct device *dev, pm_message_t state)
+static int __device_suspend_noirq(struct device *dev, pm_message_t state)
 {
 	int error = 0;
 
-	if (!dev->bus)
-		return 0;
-
-	if (dev->bus->pm) {
+	if (dev->bus && dev->bus->pm) {
 		pm_dev_dbg(dev, state, "LATE ");
 		error = pm_noirq_op(dev, dev->bus->pm, state);
 	}
+
+	dev->power.op_complete = true;
+	wake_up_all(&dev->power.wait_queue);
+
 	return error;
 }
 
 /**
+ * async_device_suspend_noirq - Wrapper of __device_suspend_noirq().
+ * @dev: Device to suspend.
+ *
+ * Execute __device_suspend_noirq() for given device unless async_error is
+ * set and if that returns error code, copy it to async_error and change the
+ * PM status of @dev to DPM_OFF.
+ */
+static void async_device_suspend_noirq(struct device *dev)
+{
+	int error = atomic_read(&async_error);
+
+	if (error)
+		return;
+	pm_dev_dbg(dev, pm_transition, "async LATE ");
+	error = __device_suspend_noirq(dev, pm_transition);
+	if (!error)
+		return;
+	pm_dev_err(dev, pm_transition, " async LATE", error);
+	dev->power.status = DPM_OFF;
+	atomic_set(&async_error, error);
+}
+
+/**
+ * async_suspend_noirq - Execute "late" suspend callbacks asynchronously.
+ * @data: Pointer to the first device to suspend.
+ * @cookie: Ignored.
+ *
+ * The execution of this function is scheduled with async_schedule(), so it runs
+ * in its own kernel thread.  It first calls the "late" suspend callback for the
+ * device passed to it as @data.  Next, it walks dpm_list looking for devices
+ * that can be suspended without waiting for their "slaves".  If such a device
+ * is found, its "late" suspend callback is run.
+ */
+static void async_suspend_noirq(void *data, async_cookie_t cookie)
+{
+	struct device *dev = (struct device *)data;
+
+	device_pm_wait_for_slaves(dev);
+	async_device_suspend_noirq(dev);
+
+	list_for_each_entry_continue_reverse(dev, &dpm_list, power.entry) {
+		if (!dev->power.async_suspend)
+			continue;
+
+		if (device_pm_check_slaves(dev))
+			continue;
+
+		if (pm_op_started(dev))
+			continue;
+
+		pm_dev_dbg(dev, pm_transition, "out of order LATE ");
+		async_device_suspend_noirq(dev);
+	}
+}
+
+/**
+ * device_suspend_noirq - Execute or schedule "late" suspend callback.
+ * @dev: Device to suspend.
+ *
+ * If @dev can be resumed asynchronously, schedule the execution of
+ * async_suspend_noirq() for it.  Otherwise, execute its "late" suspend callback
+ * directly.
+ */
+static int device_suspend_noirq(struct device *dev)
+{
+	if (pm_op_started(dev))
+		return 0;
+
+	if (dev->power.async_suspend) {
+		async_schedule(async_suspend_noirq, dev);
+		return 0;
+	}
+
+	device_pm_wait_for_slaves(dev);
+	return __device_suspend_noirq(dev, pm_transition);
+}
+
+/**
  * dpm_suspend_noirq - Execute "late suspend" callbacks for non-sysdev devices.
  * @state: PM transition of the system being carried out.
  *
@@ -889,14 +988,20 @@ int dpm_suspend_noirq(pm_message_t state
 
 	suspend_device_irqs();
 	mutex_lock(&dpm_list_mtx);
+	pm_transition = state;
 	list_for_each_entry_reverse(dev, &dpm_list, power.entry) {
-		error = device_suspend_noirq(dev, state);
+		dev->power.status = DPM_OFF_IRQ;
+		error = device_suspend_noirq(dev);
 		if (error) {
-			pm_dev_err(dev, state, " late", error);
+			pm_dev_err(dev, state, " LATE", error);
+			dev->power.status = DPM_OFF;
 			break;
 		}
-		dev->power.status = DPM_OFF_IRQ;
+		error = atomic_read(&async_error);
+		if (error)
+			break;
 	}
+	dpm_synchronize_noirq();
 	mutex_unlock(&dpm_list_mtx);
 	if (error)
 		dpm_resume_noirq(resume_event(state));
@@ -909,7 +1014,7 @@ EXPORT_SYMBOL_GPL(dpm_suspend_noirq);
  * @dev: Device to handle.
  * @state: PM transition of the system being carried out.
  */
-static int device_suspend(struct device *dev, pm_message_t state)
+static int __device_suspend(struct device *dev, pm_message_t state)
 {
 	int error = 0;
 
@@ -949,11 +1054,102 @@ static int device_suspend(struct device 
 	}
  End:
 	up(&dev->sem);
+	dev->power.op_complete = true;
+	wake_up_all(&dev->power.wait_queue);
 
 	return error;
 }
 
 /**
+ * async_device_suspend - Wrapper of __device_suspend().
+ * @dev: Device to suspend.
+ *
+ * Execute __device_suspend() for given device unless async_error is set and if
+ * that returns error code, copy it to async_error and change the PM status of
+ * @dev to DPM_SUSPENDING.
+ */
+static void async_device_suspend(struct device *dev)
+{
+	int error = atomic_read(&async_error);
+
+	if (error)
+		return;
+	pm_dev_dbg(dev, pm_transition, "async ");
+	error = __device_suspend(dev, pm_transition);
+	if (!error)
+		return;
+	pm_dev_err(dev, pm_transition, " async", error);
+	mutex_lock(&dpm_list_mtx);
+	dev->power.status = DPM_SUSPENDING;
+	atomic_set(&async_error, error);
+	mutex_unlock(&dpm_list_mtx);
+}
+
+/**
+ * async_suspend - Execute suspend callbacks asynchronously.
+ * @data: Pointer to the first device to resume.
+ * @cookie: Ignored.
+ *
+ * The execution of this function is scheduled with async_schedule(), so it runs
+ * in its own kernel thread.  It first calls the suspend callbacks for the
+ * device passed to it as @data.  Next, it walks dpm_list looking for devices
+ * that can be suspended without waiting for their "slaves".  If such a device
+ * is found, its suspend callbacks are run.
+ */
+static void async_suspend(void *data, async_cookie_t cookie)
+{
+	struct device *dev = (struct device *)data;
+
+	device_pm_wait_for_slaves(dev);
+
+ repeat:
+	async_device_suspend(dev);
+	put_device(dev);
+
+	mutex_lock(&dpm_list_mtx);
+	if (dev->power.status >= DPM_OFF)
+		dev = to_device(dpm_list.prev);
+	list_for_each_entry_continue_reverse(dev, &dpm_list, power.entry) {
+		if (!dev->power.async_suspend)
+			continue;
+
+		if (device_pm_check_slaves(dev))
+			continue;
+
+		if (pm_op_started(dev))
+			continue;
+
+		get_device(dev);
+		mutex_unlock(&dpm_list_mtx);
+		pm_dev_dbg(dev, pm_transition, "out of order ");
+		goto repeat;
+	}
+	mutex_unlock(&dpm_list_mtx);
+}
+
+/**
+ * device_suspend - Execute or schedule suspend callbacks for given device.
+ * @dev: Device to suspend.
+ *
+ * If @dev can be suspended asynchronously, schedule the execution of
+ * async_suspend() for it.  Otherwise, execute its suspend callbacks directly.
+ */
+static int device_suspend(struct device *dev)
+{
+	if (pm_op_started(dev))
+		return 0;
+
+	if (dev->power.async_suspend) {
+		get_device(dev);
+		async_schedule(async_suspend, dev);
+		return 0;
+	}
+
+	device_pm_wait_for_slaves(dev);
+	return __device_suspend(dev, pm_transition);
+}
+
+/**
  * dpm_suspend - Execute "suspend" callbacks for all non-sysdev devices.
  * @state: PM transition of the system being carried out.
  */
@@ -964,27 +1160,33 @@ static int dpm_suspend(pm_message_t stat
 
 	INIT_LIST_HEAD(&list);
 	mutex_lock(&dpm_list_mtx);
+	pm_transition = state;
 	while (!list_empty(&dpm_list)) {
 		struct device *dev = to_device(dpm_list.prev);
 
 		get_device(dev);
+		dev->power.status = DPM_OFF;
 		mutex_unlock(&dpm_list_mtx);
 
-		error = device_suspend(dev, state);
+		error = device_suspend(dev);
 
 		mutex_lock(&dpm_list_mtx);
 		if (error) {
 			pm_dev_err(dev, state, "", error);
+			dev->power.status = DPM_SUSPENDING;
 			put_device(dev);
 			break;
 		}
-		dev->power.status = DPM_OFF;
 		if (!list_empty(&dev->power.entry))
 			list_move(&dev->power.entry, &list);
 		put_device(dev);
+		error = atomic_read(&async_error);
+		if (error)
+			break;
 	}
 	list_splice(&list, dpm_list.prev);
 	mutex_unlock(&dpm_list_mtx);
+	dpm_synchronize();
 	return error;
 }
 
@@ -1043,6 +1245,7 @@ static int dpm_prepare(pm_message_t stat
 	INIT_LIST_HEAD(&list);
 	mutex_lock(&dpm_list_mtx);
 	transition_started = true;
+	atomic_set(&async_error, 0);
 	while (!list_empty(&dpm_list)) {
 		struct device *dev = to_device(dpm_list.next);
 

[PATCH 4/6] PM: Allow PCI devices to suspend/resume asynchronously

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rjw@sisk.pl>

Set async_suspend for all PCI devices and PCIe port services, so that
they can be suspended and resumed asynchronously with other devices
they don't depend on in a known way (i.e. devices which are not their
parents or children and to which they are not connected via struct
pm_link objects).

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/pci/pci.c               |    1 +
 drivers/pci/pcie/portdrv_core.c |    1 +
 2 files changed, 2 insertions(+)

Index: linux-2.6/drivers/pci/pci.c
===================================================================
--- linux-2.6.orig/drivers/pci/pci.c
+++ linux-2.6/drivers/pci/pci.c
@@ -1360,6 +1360,7 @@ void pci_pm_init(struct pci_dev *dev)
 	int pm;
 	u16 pmc;
 
+	device_enable_async_suspend(&dev->dev, true);
 	dev->pm_cap = 0;
 
 	/* find PCI PM capability in list */
Index: linux-2.6/drivers/pci/pcie/portdrv_core.c
===================================================================
--- linux-2.6.orig/drivers/pci/pcie/portdrv_core.c
+++ linux-2.6/drivers/pci/pcie/portdrv_core.c
@@ -280,6 +280,7 @@ static void pcie_device_init(struct pci_
 	dev_set_name(device, "%s:pcie%02x",
 		 pci_name(parent), get_descriptor_id(port_type, service_type));
 	device->parent = &parent->dev;
+	device_enable_async_suspend(device, true);
 }
 
 /**

[PATCH 5/6] PM: Allow ACPI devices to suspend/resume asynchronously

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rjw@sisk.pl>

Set async_suspend for all ACPI devices, so that they can be suspended
and resumed asynchronously with other devices they don't depend on in
a known way (i.e. devices which are not their parents or children and
to which they are not connected via struct pm_link objects).

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/acpi/scan.c |    1 +
 1 file changed, 1 insertion(+)

Index: linux-2.6/drivers/acpi/scan.c
===================================================================
--- linux-2.6.orig/drivers/acpi/scan.c
+++ linux-2.6/drivers/acpi/scan.c
@@ -539,6 +539,7 @@ static int acpi_device_register(struct a
 		       dev_name(&device->dev));
 
 	device->removal_type = ACPI_BUS_REMOVAL_NORMAL;
+	device_enable_async_suspend(&device->dev, true);
 	return 0;
 end:
 	mutex_lock(&acpi_device_lock);

[PATCH 6/6] PM: Allow serio input devices to suspend/resume asynchronously

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rjw@sisk.pl>

Set async_suspend for all serio input devices, so that they can be
suspended and resumed asynchronously with other devices they don't
depend on in a known way (i.e. devices which are not their parents
or children and to which they are not connected via struct pm_link
objects).

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/input/serio/serio.c |    1 +
 1 file changed, 1 insertion(+)

Index: linux-2.6/drivers/input/serio/serio.c
===================================================================
--- linux-2.6.orig/drivers/input/serio/serio.c
+++ linux-2.6/drivers/input/serio/serio.c
@@ -576,6 +576,7 @@ static void serio_add_port(struct serio 
 			printk(KERN_ERR
 				"serio: sysfs_create_group() failed for %s (%s), error: %d\n",
 				serio->phys, serio->name, error);
+		device_enable_async_suspend(&serio->dev, true);
 	}
 }
 

Re: [PATCH 6/6] PM: Allow serio input devices to suspend/resume asynchronously

[Dmitry Torokhov]

On Wed, Aug 26, 2009 at 10:24:19PM +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rjw@sisk.pl>
> 
> Set async_suspend for all serio input devices, so that they can be
> suspended and resumed asynchronously with other devices they don't
> depend on in a known way (i.e. devices which are not their parents
> or children and to which they are not connected via struct pm_link
> objects).
> 

As I mentioned not yet, please - i8042 may be unhappy if you pound
all its ports at once. I should have a solution for it though.

-- 
Dmitry

Re: [PATCH 6/6] PM: Allow serio input devices to suspend/resume asynchronously

[Rafael J. Wysocki]

On Thursday 27 August 2009, Dmitry Torokhov wrote:
> On Wed, Aug 26, 2009 at 10:24:19PM +0200, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rjw@sisk.pl>
> > 
> > Set async_suspend for all serio input devices, so that they can be
> > suspended and resumed asynchronously with other devices they don't
> > depend on in a known way (i.e. devices which are not their parents
> > or children and to which they are not connected via struct pm_link
> > objects).
> > 
> 
> As I mentioned not yet, please - i8042 may be unhappy if you pound
> all its ports at once. I should have a solution for it though.

OK, I'll drop this patch.

Thanks,
Rafael

Re: [PATCH 0/6] PM: Asynchronous suspend and resume of devices

[Rafael J. Wysocki]

On Wednesday 26 August 2009, Rafael J. Wysocki wrote:
> Hi,
> 
> The following patches introduce a mechanism allowing us to execute device
> drivers' suspend and resume callbacks asynchronously during system sleep
> transitions, such as suspend to RAM.
> 
> The idea is explained in the changelogs of the first two patches.
> 
> Comments welcome.

Forgot to say, the patchset is on top of the linux-next branch of suspend-2.6.

Thanks,
Rafael

Re: [PATCH 0/6] PM: Asynchronous suspend and resume of devices

[Rafael J. Wysocki]

On Thursday 27 August 2009, Rafael J. Wysocki wrote:
> On Wednesday 26 August 2009, Rafael J. Wysocki wrote:
> > Hi,
> > 
> > The following patches introduce a mechanism allowing us to execute device
> > drivers' suspend and resume callbacks asynchronously during system sleep
> > transitions, such as suspend to RAM.
> > 
> > The idea is explained in the changelogs of the first two patches.
> > 
> > Comments welcome.
> 
> Forgot to say, the patchset is on top of the linux-next branch of suspend-2.6.

I have two more patches in this series.  The first one introduces a sysfs
switch allowing the user space to block the async suspend/resume.  The second
one gives the user space access to the power.async_suspend flag of devices
through sysfs.

The patches will follow in the next messages.

Thanks,
Rafael

[PATCH 7] PM: Add a switch for disabling/enabling asynchronous suspend/resume

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rjw@sisk.pl>

Add sysfs attribute kernel/power/pm_async allowing the user space to
disable and enable async suspend/resume of devices.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/base/power/main.c  |   13 +++++++------
 drivers/base/power/power.h |    6 +++---
 kernel/power/main.c        |   28 +++++++++++++++++++++++++++-
 3 files changed, 37 insertions(+), 10 deletions(-)

Index: linux-2.6/kernel/power/main.c
===================================================================
--- linux-2.6.orig/kernel/power/main.c
+++ linux-2.6/kernel/power/main.c
@@ -44,6 +44,29 @@ int pm_notifier_call_chain(unsigned long
 			== NOTIFY_BAD) ? -EINVAL : 0;
 }
 
+/* If set, devices may be suspended and resumed asynchronously. */
+int pm_async_enabled = 1;
+
+static ssize_t pm_async_show(struct kobject *kobj, struct kobj_attribute *attr,
+			     char *buf)
+{
+	return sprintf(buf, "%d\n", pm_async_enabled);
+}
+
+static ssize_t pm_async_store(struct kobject *kobj, struct kobj_attribute *attr,
+			      const char *buf, size_t n)
+{
+	int val;
+
+	if (sscanf(buf, "%d", &val) == 1) {
+		pm_async_enabled = !!val;
+		return n;
+	}
+	return -EINVAL;
+}
+
+power_attr(pm_async);
+
 #ifdef CONFIG_PM_DEBUG
 int pm_test_level = TEST_NONE;
 
@@ -208,9 +231,12 @@ static struct attribute * g[] = {
 #ifdef CONFIG_PM_TRACE
 	&pm_trace_attr.attr,
 #endif
-#if defined(CONFIG_PM_SLEEP) && defined(CONFIG_PM_DEBUG)
+#ifdef CONFIG_PM_SLEEP
+	&pm_async_attr.attr,
+#ifdef CONFIG_PM_DEBUG
 	&pm_test_attr.attr,
 #endif
+#endif
 	NULL,
 };
 
Index: linux-2.6/drivers/base/power/main.c
===================================================================
--- linux-2.6.orig/drivers/base/power/main.c
+++ linux-2.6/drivers/base/power/main.c
@@ -235,7 +235,7 @@ static int device_pm_wait_fn(struct devi
  */
 static void device_pm_wait_for_masters(struct device *slave)
 {
-	if (!pm_trace_enabled)
+	if (pm_async_enabled && !pm_trace_enabled)
 		device_for_each_master(slave, slave, device_pm_wait_fn);
 }
 
@@ -245,7 +245,8 @@ static void device_pm_wait_for_masters(s
  */
 static void device_pm_wait_for_slaves(struct device *master)
 {
-	device_for_each_slave(master, master, device_pm_wait_fn);
+	if (pm_async_enabled)
+		device_for_each_slave(master, master, device_pm_wait_fn);
 }
 
 /**
@@ -560,7 +561,7 @@ static int device_resume_noirq(struct de
 	if (pm_op_started(dev))
 		return 0;
 
-	if (dev->power.async_suspend && !pm_trace_enabled) {
+	if (pm_async_enabled && !pm_trace_enabled && dev->power.async_suspend) {
 		async_schedule(async_resume_noirq, dev);
 		return 0;
 	}
@@ -719,7 +720,7 @@ static int device_resume(struct device *
 	if (pm_op_started(dev))
 		return 0;
 
-	if (dev->power.async_suspend && !pm_trace_enabled) {
+	if (pm_async_enabled && !pm_trace_enabled && dev->power.async_suspend) {
 		get_device(dev);
 		async_schedule(async_resume, dev);
 		return 0;
@@ -965,7 +966,7 @@ static int device_suspend_noirq(struct d
 	if (pm_op_started(dev))
 		return 0;
 
-	if (dev->power.async_suspend) {
+	if (pm_async_enabled && dev->power.async_suspend) {
 		async_schedule(async_suspend_noirq, dev);
 		return 0;
 	}
@@ -1139,7 +1140,7 @@ static int device_suspend(struct device 
 	if (pm_op_started(dev))
 		return 0;
 
-	if (dev->power.async_suspend) {
+	if (pm_async_enabled && dev->power.async_suspend) {
 		get_device(dev);
 		async_schedule(async_suspend, dev);
 		return 0;
Index: linux-2.6/drivers/base/power/power.h
===================================================================
--- linux-2.6.orig/drivers/base/power/power.h
+++ linux-2.6/drivers/base/power/power.h
@@ -16,10 +16,10 @@ static inline void pm_runtime_remove(str
 
 #ifdef CONFIG_PM_SLEEP
 
-/*
- * main.c
- */
+/* kernel/power/main.c */
+extern int pm_async_enabled;
 
+/* drivers/base/power/main.c */
 extern struct list_head dpm_list;	/* The active device list */
 
 static inline struct device *to_device(struct list_head *entry)

Re: [PATCH 7] PM: Add a switch for disabling/enabling asynchronous suspend/resume

[Dmitry Torokhov]

On Thu, Aug 27, 2009 at 09:19:46PM +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rjw@sisk.pl>
> 
> Add sysfs attribute kernel/power/pm_async allowing the user space to
> disable and enable async suspend/resume of devices.
> 
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> ---
>  drivers/base/power/main.c  |   13 +++++++------
>  drivers/base/power/power.h |    6 +++---
>  kernel/power/main.c        |   28 +++++++++++++++++++++++++++-
>  3 files changed, 37 insertions(+), 10 deletions(-)
> 
> Index: linux-2.6/kernel/power/main.c
> ===================================================================
> --- linux-2.6.orig/kernel/power/main.c
> +++ linux-2.6/kernel/power/main.c
> @@ -44,6 +44,29 @@ int pm_notifier_call_chain(unsigned long
>  			== NOTIFY_BAD) ? -EINVAL : 0;
>  }
>  
> +/* If set, devices may be suspended and resumed asynchronously. */
> +int pm_async_enabled = 1;
> +
> +static ssize_t pm_async_show(struct kobject *kobj, struct kobj_attribute *attr,
> +			     char *buf)
> +{
> +	return sprintf(buf, "%d\n", pm_async_enabled);
> +}
> +
> +static ssize_t pm_async_store(struct kobject *kobj, struct kobj_attribute *attr,
> +			      const char *buf, size_t n)
> +{
> +	int val;
> +
> +	if (sscanf(buf, "%d", &val) == 1) {

I prefer more vigorous restrictions on the sysfs interfaces. Maybe:

	if (strict_strtoul(buf, 10, &val) || val > 1)
		return -EINVAL;

	pm_async_enabled = val;

?

-- 
Dmitry

[PATCH 7 updated] PM: Add a switch for disabling/enabling asynchronous suspend/resume

[Rafael J. Wysocki]

On Thursday 27 August 2009, Dmitry Torokhov wrote:
> On Thu, Aug 27, 2009 at 09:19:46PM +0200, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rjw@sisk.pl>
> > 
> > Add sysfs attribute kernel/power/pm_async allowing the user space to
> > disable and enable async suspend/resume of devices.
> > 
> > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> > ---
> >  drivers/base/power/main.c  |   13 +++++++------
> >  drivers/base/power/power.h |    6 +++---
> >  kernel/power/main.c        |   28 +++++++++++++++++++++++++++-
> >  3 files changed, 37 insertions(+), 10 deletions(-)
> > 
> > Index: linux-2.6/kernel/power/main.c
> > ===================================================================
> > --- linux-2.6.orig/kernel/power/main.c
> > +++ linux-2.6/kernel/power/main.c
> > @@ -44,6 +44,29 @@ int pm_notifier_call_chain(unsigned long
> >  			== NOTIFY_BAD) ? -EINVAL : 0;
> >  }
> >  
> > +/* If set, devices may be suspended and resumed asynchronously. */
> > +int pm_async_enabled = 1;
> > +
> > +static ssize_t pm_async_show(struct kobject *kobj, struct kobj_attribute *attr,
> > +			     char *buf)
> > +{
> > +	return sprintf(buf, "%d\n", pm_async_enabled);
> > +}
> > +
> > +static ssize_t pm_async_store(struct kobject *kobj, struct kobj_attribute *attr,
> > +			      const char *buf, size_t n)
> > +{
> > +	int val;
> > +
> > +	if (sscanf(buf, "%d", &val) == 1) {
> 
> I prefer more vigorous restrictions on the sysfs interfaces. Maybe:
> 
> 	if (strict_strtoul(buf, 10, &val) || val > 1)
> 		return -EINVAL;
> 
> 	pm_async_enabled = val;
> 
> ?

OK

Updated patch below.

Thanks,
Rafael

---
From: Rafael J. Wysocki <rjw@sisk.pl>
Subject: PM: Add a switch for disabling/enabling asynchronous suspend/resume

Add sysfs attribute kernel/power/pm_async allowing the user space to
disable and enable async suspend/resume of devices.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/base/power/main.c  |   13 +++++++------
 drivers/base/power/power.h |    6 +++---
 kernel/power/main.c        |   31 ++++++++++++++++++++++++++++++-
 3 files changed, 40 insertions(+), 10 deletions(-)

Index: linux-2.6/kernel/power/main.c
===================================================================
--- linux-2.6.orig/kernel/power/main.c
+++ linux-2.6/kernel/power/main.c
@@ -44,6 +44,32 @@ int pm_notifier_call_chain(unsigned long
 			== NOTIFY_BAD) ? -EINVAL : 0;
 }
 
+/* If set, devices may be suspended and resumed asynchronously. */
+int pm_async_enabled = 1;
+
+static ssize_t pm_async_show(struct kobject *kobj, struct kobj_attribute *attr,
+			     char *buf)
+{
+	return sprintf(buf, "%d\n", pm_async_enabled);
+}
+
+static ssize_t pm_async_store(struct kobject *kobj, struct kobj_attribute *attr,
+			      const char *buf, size_t n)
+{
+	unsigned long val;
+
+	if (strict_strtoul(buf, 10, &val))
+		return -EINVAL;
+
+	if (val > 1)
+		return -EINVAL;
+
+	pm_async_enabled = val;
+	return n;
+}
+
+power_attr(pm_async);
+
 #ifdef CONFIG_PM_DEBUG
 int pm_test_level = TEST_NONE;
 
@@ -208,9 +234,12 @@ static struct attribute * g[] = {
 #ifdef CONFIG_PM_TRACE
 	&pm_trace_attr.attr,
 #endif
-#if defined(CONFIG_PM_SLEEP) && defined(CONFIG_PM_DEBUG)
+#ifdef CONFIG_PM_SLEEP
+	&pm_async_attr.attr,
+#ifdef CONFIG_PM_DEBUG
 	&pm_test_attr.attr,
 #endif
+#endif
 	NULL,
 };
 
Index: linux-2.6/drivers/base/power/main.c
===================================================================
--- linux-2.6.orig/drivers/base/power/main.c
+++ linux-2.6/drivers/base/power/main.c
@@ -235,7 +235,7 @@ static int device_pm_wait_fn(struct devi
  */
 static void device_pm_wait_for_masters(struct device *slave)
 {
-	if (!pm_trace_enabled)
+	if (pm_async_enabled && !pm_trace_enabled)
 		device_for_each_master(slave, slave, device_pm_wait_fn);
 }
 
@@ -245,7 +245,8 @@ static void device_pm_wait_for_masters(s
  */
 static void device_pm_wait_for_slaves(struct device *master)
 {
-	device_for_each_slave(master, master, device_pm_wait_fn);
+	if (pm_async_enabled)
+		device_for_each_slave(master, master, device_pm_wait_fn);
 }
 
 /**
@@ -560,7 +561,7 @@ static int device_resume_noirq(struct de
 	if (pm_op_started(dev))
 		return 0;
 
-	if (dev->power.async_suspend && !pm_trace_enabled) {
+	if (pm_async_enabled && !pm_trace_enabled && dev->power.async_suspend) {
 		async_schedule(async_resume_noirq, dev);
 		return 0;
 	}
@@ -719,7 +720,7 @@ static int device_resume(struct device *
 	if (pm_op_started(dev))
 		return 0;
 
-	if (dev->power.async_suspend && !pm_trace_enabled) {
+	if (pm_async_enabled && !pm_trace_enabled && dev->power.async_suspend) {
 		get_device(dev);
 		async_schedule(async_resume, dev);
 		return 0;
@@ -965,7 +966,7 @@ static int device_suspend_noirq(struct d
 	if (pm_op_started(dev))
 		return 0;
 
-	if (dev->power.async_suspend) {
+	if (pm_async_enabled && dev->power.async_suspend) {
 		async_schedule(async_suspend_noirq, dev);
 		return 0;
 	}
@@ -1139,7 +1140,7 @@ static int device_suspend(struct device 
 	if (pm_op_started(dev))
 		return 0;
 
-	if (dev->power.async_suspend) {
+	if (pm_async_enabled && dev->power.async_suspend) {
 		get_device(dev);
 		async_schedule(async_suspend, dev);
 		return 0;
Index: linux-2.6/drivers/base/power/power.h
===================================================================
--- linux-2.6.orig/drivers/base/power/power.h
+++ linux-2.6/drivers/base/power/power.h
@@ -16,10 +16,10 @@ static inline void pm_runtime_remove(str
 
 #ifdef CONFIG_PM_SLEEP
 
-/*
- * main.c
- */
+/* kernel/power/main.c */
+extern int pm_async_enabled;
 
+/* drivers/base/power/main.c */
 extern struct list_head dpm_list;	/* The active device list */
 
 static inline struct device *to_device(struct list_head *entry)

Re: [PATCH 7 updated] PM: Add a switch for disabling/enabling asynchronous suspend/resume

[Dmitry Torokhov]

On Fri, Aug 28, 2009 at 12:22:40AM +0200, Rafael J. Wysocki wrote:
> +/* kernel/power/main.c */
> +extern int pm_async_enabled;
>  

Sorry, forgot about this in the first mail - why don't we make it a
bool?

-- 
Dmitry

Re: [PATCH 7 updated] PM: Add a switch for disabling/enabling asynchronous suspend/resume

[Rafael J. Wysocki]

On Friday 28 August 2009, Dmitry Torokhov wrote:
> On Fri, Aug 28, 2009 at 12:22:40AM +0200, Rafael J. Wysocki wrote:
> > +/* kernel/power/main.c */
> > +extern int pm_async_enabled;
> >  
> 
> Sorry, forgot about this in the first mail - why don't we make it a
> bool?

I didn't do that because of the

sprintf(buf, "%d\n", pm_async_enabled)

in pm_async_show(), where it clearly is treated as an int.

Thanks,
Rafael

[PATCH 8] PM: Allow user space to change the power.async_suspend flag of devices

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rjw@sisk.pl>

Add sysfs attribute power/async for every device allowing the user
space to access the device's power.async_suspend flag and modify it,
if necessary.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/base/power/sysfs.c |   47 +++++++++++++++++++++++++++++++++++++++++++++
 include/linux/device.h     |    5 ++++
 2 files changed, 52 insertions(+)

Index: linux-2.6/drivers/base/power/sysfs.c
===================================================================
--- linux-2.6.orig/drivers/base/power/sysfs.c
+++ linux-2.6/drivers/base/power/sysfs.c
@@ -38,6 +38,22 @@
  *	wakeup events internally (unless they are disabled), keeping
  *	their hardware in low power modes whenever they're unused.  This
  *	saves runtime power, without requiring system-wide sleep states.
+ *
+ *	async - Report/change current async suspend setting for the device
+ *
+ *	If set, the PM core will attempt to suspend and resume the device during
+ *	system power transitions (e.g. suspend to RAM, hibernation) in parallel
+ *	with other devices it doesn't appear to depend on (to the PM core's
+ *	knowledge).
+ *
+ *	 + "enabled\n" to permit the asynchronous suspend/resume of the device
+ *	 + "disabled\n" to forbid it
+ *
+ *	NOTE: It generally is unsafe to permit the asynchronous suspend/resume
+ *	of a device unless it is certain that all of the PM dependencies of the
+ *	device are known to the PM core.  However, for some devices this
+ *	attribute is set to "enabled" by the kernel and in that cases it should
+ *	be safe to leave the default value.
  */
 
 static const char enabled[] = "enabled";
@@ -77,9 +93,40 @@ wake_store(struct device * dev, struct d
 
 static DEVICE_ATTR(wakeup, 0644, wake_show, wake_store);
 
+#ifdef CONFIG_PM_SLEEP
+static ssize_t async_show(struct device *dev, struct device_attribute *attr,
+			  char *buf)
+{
+	return sprintf(buf, "%s\n",
+			device_async_suspend_enabled(dev) ? enabled : disabled);
+}
+
+static ssize_t async_store(struct device *dev, struct device_attribute *attr,
+			   const char *buf, size_t n)
+{
+	char *cp;
+	int len = n;
+
+	cp = memchr(buf, '\n', n);
+	if (cp)
+		len = cp - buf;
+	if (len == sizeof enabled - 1 && strncmp(buf, enabled, len) == 0)
+		device_enable_async_suspend(dev, true);
+	else if (len == sizeof disabled - 1 && strncmp(buf, disabled, len) == 0)
+		device_enable_async_suspend(dev, false);
+	else
+		return -EINVAL;
+	return n;
+}
+
+static DEVICE_ATTR(async, 0644, async_show, async_store);
+#endif /* CONFIG_PM_SLEEP */
 
 static struct attribute * power_attrs[] = {
 	&dev_attr_wakeup.attr,
+#ifdef CONFIG_PM_SLEEP
+	&dev_attr_async.attr,
+#endif
 	NULL,
 };
 static struct attribute_group pm_attr_group = {
Index: linux-2.6/include/linux/device.h
===================================================================
--- linux-2.6.orig/include/linux/device.h
+++ linux-2.6/include/linux/device.h
@@ -478,6 +478,11 @@ static inline void device_enable_async_s
 		dev->power.async_suspend = enable;
 }
 
+static inline bool device_async_suspend_enabled(struct device *dev)
+{
+	return !!dev->power.async_suspend;
+}
+
 void driver_init(void);
 
 /*

Re: [PATCH 8] PM: Allow user space to change the power.async_suspend flag of devices

[Dmitry Torokhov]

Hi Rafael,

On Thu, Aug 27, 2009 at 09:20:55PM +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rjw@sisk.pl>
> 
> Add sysfs attribute power/async for every device allowing the user
> space to access the device's power.async_suspend flag and modify it,
> if necessary.
> 

I don't think we should let users meddle with individual devices;
one global flag should be enough to work around "broken-at-the-moment"
issues.

-- 
Dmitry

Re: [PATCH 8] PM: Allow user space to change the power.async_suspend flag of devices

[Rafael J. Wysocki]

On Thursday 27 August 2009, Dmitry Torokhov wrote:
> Hi Rafael,
> 
> On Thu, Aug 27, 2009 at 09:20:55PM +0200, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rjw@sisk.pl>
> > 
> > Add sysfs attribute power/async for every device allowing the user
> > space to access the device's power.async_suspend flag and modify it,
> > if necessary.
> > 
> 
> I don't think we should let users meddle with individual devices;
> one global flag should be enough to work around "broken-at-the-moment"
> issues.

Well, this patch is handy for testing if anyone is interested, so I posted it.

It isn't exactly safe, though, you're right.

Thanks,
Rafael

Re: [PATCH 8] PM: Allow user space to change the power.async_suspend flag of devices

[Pavel Machek]

On Fri 2009-08-28 00:24:06, Rafael J. Wysocki wrote:
> On Thursday 27 August 2009, Dmitry Torokhov wrote:
> > Hi Rafael,
> > 
> > On Thu, Aug 27, 2009 at 09:20:55PM +0200, Rafael J. Wysocki wrote:
> > > From: Rafael J. Wysocki <rjw@sisk.pl>
> > > 
> > > Add sysfs attribute power/async for every device allowing the user
> > > space to access the device's power.async_suspend flag and modify it,
> > > if necessary.
> > > 
> > 
> > I don't think we should let users meddle with individual devices;
> > one global flag should be enough to work around "broken-at-the-moment"
> > issues.
> 
> Well, this patch is handy for testing if anyone is interested, so I posted it.
> 
> It isn't exactly safe, though, you're right.

I guess it would be acceptable to let the users disable it
per-driver... but if we can get without that, it would be better.

(it is obviously ok for debugging).
									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[PATCH 8 update] PM: Allow user space to change the power.async_suspend flag of devices

[Rafael J. Wysocki]

On Friday 28 August 2009, Pavel Machek wrote:
> On Fri 2009-08-28 00:24:06, Rafael J. Wysocki wrote:
> > On Thursday 27 August 2009, Dmitry Torokhov wrote:
> > > Hi Rafael,
> > > 
> > > On Thu, Aug 27, 2009 at 09:20:55PM +0200, Rafael J. Wysocki wrote:
> > > > From: Rafael J. Wysocki <rjw@sisk.pl>
> > > > 
> > > > Add sysfs attribute power/async for every device allowing the user
> > > > space to access the device's power.async_suspend flag and modify it,
> > > > if necessary.
> > > > 
> > > 
> > > I don't think we should let users meddle with individual devices;
> > > one global flag should be enough to work around "broken-at-the-moment"
> > > issues.
> > 
> > Well, this patch is handy for testing if anyone is interested, so I posted it.
> > 
> > It isn't exactly safe, though, you're right.
> 
> I guess it would be acceptable to let the users disable it
> per-driver... but if we can get without that, it would be better.
> 
> (it is obviously ok for debugging).

Hmm.  What about the appended patch, then?

Thanks,
Rafael

---
From: Rafael J. Wysocki <rjw@sisk.pl>
Subject: PM: Add facility for advanced testing of async suspend/resume

Add configuration switch CONFIG_PM_ADVANCED_DEBUG for compiling in
extra PM debugging/testing code allowing one to access some
PM-related attributes of devices from the user space via sysfs.

If CONFIG_PM_ADVANCED_DEBUG is set, add sysfs attribute power/async
for every device allowing the user space to access the device's
power.async_suspend flag and modify it, if desired.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/base/power/sysfs.c |   47 +++++++++++++++++++++++++++++++++++++++++++++
 include/linux/device.h     |    5 ++++
 kernel/power/Kconfig       |   14 +++++++++++++
 3 files changed, 66 insertions(+)

Index: linux-2.6/drivers/base/power/sysfs.c
===================================================================
--- linux-2.6.orig/drivers/base/power/sysfs.c
+++ linux-2.6/drivers/base/power/sysfs.c
@@ -38,6 +38,22 @@
  *	wakeup events internally (unless they are disabled), keeping
  *	their hardware in low power modes whenever they're unused.  This
  *	saves runtime power, without requiring system-wide sleep states.
+ *
+ *	async - Report/change current async suspend setting for the device
+ *
+ *	If set, the PM core will attempt to suspend and resume the device during
+ *	system power transitions (e.g. suspend to RAM, hibernation) in parallel
+ *	with other devices it doesn't appear to depend on (to the PM core's
+ *	knowledge).
+ *
+ *	 + "enabled\n" to permit the asynchronous suspend/resume of the device
+ *	 + "disabled\n" to forbid it
+ *
+ *	NOTE: It generally is unsafe to permit the asynchronous suspend/resume
+ *	of a device unless it is certain that all of the PM dependencies of the
+ *	device are known to the PM core.  However, for some devices this
+ *	attribute is set to "enabled" by the kernel and in that cases it should
+ *	be safe to leave the default value.
  */
 
 static const char enabled[] = "enabled";
@@ -77,9 +93,40 @@ wake_store(struct device * dev, struct d
 
 static DEVICE_ATTR(wakeup, 0644, wake_show, wake_store);
 
+#ifdef CONFIG_PM_SLEEP_ADVANCED_DEBUG
+static ssize_t async_show(struct device *dev, struct device_attribute *attr,
+			  char *buf)
+{
+	return sprintf(buf, "%s\n",
+			device_async_suspend_enabled(dev) ? enabled : disabled);
+}
+
+static ssize_t async_store(struct device *dev, struct device_attribute *attr,
+			   const char *buf, size_t n)
+{
+	char *cp;
+	int len = n;
+
+	cp = memchr(buf, '\n', n);
+	if (cp)
+		len = cp - buf;
+	if (len == sizeof enabled - 1 && strncmp(buf, enabled, len) == 0)
+		device_enable_async_suspend(dev, true);
+	else if (len == sizeof disabled - 1 && strncmp(buf, disabled, len) == 0)
+		device_enable_async_suspend(dev, false);
+	else
+		return -EINVAL;
+	return n;
+}
+
+static DEVICE_ATTR(async, 0644, async_show, async_store);
+#endif /* CONFIG_PM_SLEEP_ADVANCED_DEBUG */
 
 static struct attribute * power_attrs[] = {
 	&dev_attr_wakeup.attr,
+#ifdef CONFIG_PM_SLEEP_ADVANCED_DEBUG
+	&dev_attr_async.attr,
+#endif
 	NULL,
 };
 static struct attribute_group pm_attr_group = {
Index: linux-2.6/include/linux/device.h
===================================================================
--- linux-2.6.orig/include/linux/device.h
+++ linux-2.6/include/linux/device.h
@@ -478,6 +478,11 @@ static inline void device_enable_async_s
 		dev->power.async_suspend = enable;
 }
 
+static inline bool device_async_suspend_enabled(struct device *dev)
+{
+	return !!dev->power.async_suspend;
+}
+
 void driver_init(void);
 
 /*
Index: linux-2.6/kernel/power/Kconfig
===================================================================
--- linux-2.6.orig/kernel/power/Kconfig
+++ linux-2.6/kernel/power/Kconfig
@@ -27,6 +27,15 @@ config PM_DEBUG
 	code. This is helpful when debugging and reporting PM bugs, like
 	suspend support.
 
+config PM_ADVANCED_DEBUG
+	bool "Extra PM attributes in sysfs for low-level debugging/testing"
+	depends on PM_DEBUG
+	default n
+	---help---
+	Add extra sysfs attributes allowing one to access some Power Management
+	fields of device objects from user space.  If you are not a kernel
+	developer interested in debugging/testing Power Management, say "no".
+
 config PM_VERBOSE
 	bool "Verbose Power Management debugging"
 	depends on PM_DEBUG
@@ -85,6 +94,11 @@ config PM_SLEEP
 	depends on SUSPEND || HIBERNATION || XEN_SAVE_RESTORE
 	default y
 
+config PM_SLEEP_ADVANCED_DEBUG
+	bool
+	depends on PM_ADVANCED_DEBUG
+	default n
+
 config SUSPEND
 	bool "Suspend to RAM and standby"
 	depends on PM && ARCH_SUSPEND_POSSIBLE

Re: [PATCH 0/6] PM: Asynchronous suspend and resume of devices

[Alan Stern]

On Wed, 26 Aug 2009, Rafael J. Wysocki wrote:

> Hi,
> 
> The following patches introduce a mechanism allowing us to execute device
> drivers' suspend and resume callbacks asynchronously during system sleep
> transitions, such as suspend to RAM.
> 
> The idea is explained in the changelogs of the first two patches.
> 
> Comments welcome.

I've been terribly busy and haven't had a chance to look at this.  The
earlier version seemed to have a bunch of mutual-exclusion issues; are
they resolved now?  There were also some problems involving unsafe
iteration over the dpm_list -- remember that devices can be
unregistered at any time, not just while they are suspending or
resuming.

When a device finishes, instead of having the async thread look for 
another device to work on, I think it would be better to have the 
thread check the dependents of the current device.  Those which are 
now ready can be added to a "device-ready" list, which is used to 
direct the actions of new async threads.

Alan Stern

Re: [PATCH 0/6] PM: Asynchronous suspend and resume of devices

[Rafael J. Wysocki]

On Thursday 27 August 2009, Alan Stern wrote:
> On Wed, 26 Aug 2009, Rafael J. Wysocki wrote:
> 
> > Hi,
> > 
> > The following patches introduce a mechanism allowing us to execute device
> > drivers' suspend and resume callbacks asynchronously during system sleep
> > transitions, such as suspend to RAM.
> > 
> > The idea is explained in the changelogs of the first two patches.
> > 
> > Comments welcome.
> 
> I've been terribly busy and haven't had a chance to look at this.  The
> earlier version seemed to have a bunch of mutual-exclusion issues; are
> they resolved now?

Please be more specific.

> There were also some problems involving unsafe iteration over the dpm_list
> -- remember that devices can be unregistered at any time, not just while
> they are suspending or resuming.

Not at the time we're holding dpm_list_mtx, though.

I don't think there are any unsafe iterations over dpm_list in the patches.

> When a device finishes, instead of having the async thread look for 
> another device to work on, I think it would be better to have the 
> thread check the dependents of the current device.  Those which are 
> now ready can be added to a "device-ready" list, which is used to 
> direct the actions of new async threads.

Actually, I wanted to avoid adding such a list, because that would be
duplicating of the async framework's actions, to some extent.  Yes, we can
duplicate the async framework just for the suspend/resume needs.  No, I don't
think it's worth it.

BTW, the patches have been tested on a dual-core box and I haven't seen any
problems with them so far.  Also, the testing shows that the waiting for
devices with unsatisfied dependencies is almost eliminated (on my test box
it happens 4-8 times during the entire suspend-resume cycle).

Thanks,
Rafael

[PATCH 9] PM: Measure device suspend and resume times (was: Re: [PATCH 0/6] PM: Asynchronous suspend and resume of devices)

[Rafael J. Wysocki]

On Wednesday 26 August 2009, Rafael J. Wysocki wrote:
> Hi,
> 
> The following patches introduce a mechanism allowing us to execute device
> drivers' suspend and resume callbacks asynchronously during system sleep
> transitions, such as suspend to RAM.
> 
> The idea is explained in the changelogs of the first two patches.
> 
> Comments welcome.

I have one more patch that adds time measurements to the code in
drivers/base/power/main.c, so that one can easily see how much these operations
take.

Thanks,
Rafael

---
From: Rafael J. Wysocki <rjw@sisk.pl>
Subject: PM: Measure device suspend and resume times

Measure and print the time of suspending and resuming all devices.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/base/power/main.c |   56 +++++++++++++++++++++++++++++++++++++++++++++-
 include/linux/pm.h        |    3 ++
 kernel/power/swsusp.c     |    6 ----
 3 files changed, 59 insertions(+), 6 deletions(-)

Index: linux-2.6/kernel/power/swsusp.c
===================================================================
--- linux-2.6.orig/kernel/power/swsusp.c
+++ linux-2.6/kernel/power/swsusp.c
@@ -169,14 +169,10 @@ int swsusp_swap_in_use(void)
 void swsusp_show_speed(struct timeval *start, struct timeval *stop,
 			unsigned nr_pages, char *msg)
 {
-	s64 elapsed_centisecs64;
-	int centisecs;
+	int centisecs = pm_time_elapsed(start, stop);
 	int k;
 	int kps;
 
-	elapsed_centisecs64 = timeval_to_ns(stop) - timeval_to_ns(start);
-	do_div(elapsed_centisecs64, NSEC_PER_SEC / 100);
-	centisecs = elapsed_centisecs64;
 	if (centisecs == 0)
 		centisecs = 1;	/* avoid div-by-zero */
 	k = nr_pages * (PAGE_SIZE / 1024);
Index: linux-2.6/drivers/base/power/main.c
===================================================================
--- linux-2.6.orig/drivers/base/power/main.c
+++ linux-2.6/drivers/base/power/main.c
@@ -28,6 +28,7 @@
 #include <linux/interrupt.h>
 #include <linux/async.h>
 #include <linux/completion.h>
+#include <linux/time.h>
 
 #include "../base.h"
 #include "power.h"
@@ -434,6 +435,20 @@ static bool pm_op_started(struct device 
 	return ret;
 }
 
+/**
+ * pm_time_elapsed - Compute time elapsed between two timestamps.
+ * @start: First timestamp.
+ * @stop: Second timestamp.
+ */
+int pm_time_elapsed(struct timeval *start, struct timeval *stop)
+{
+	s64 elapsed_centisecs64;
+
+	elapsed_centisecs64 = timeval_to_ns(stop) - timeval_to_ns(start);
+	do_div(elapsed_centisecs64, NSEC_PER_SEC / 100);
+	return elapsed_centisecs64;
+}
+
 static char *pm_verb(int event)
 {
 	switch (event) {
@@ -458,6 +473,16 @@ static char *pm_verb(int event)
 	}
 }
 
+static void dpm_show_time(struct timeval *start, struct timeval *stop,
+			  pm_message_t state, const char *info)
+{
+	int centisecs = pm_time_elapsed(start, stop);
+
+	printk(KERN_INFO "PM: %s%s%s of devices complete in %d.%02d seconds\n",
+		info ? info : "", info ? " " : "", pm_verb(state.event),
+		centisecs / 100, centisecs % 100);
+}
+
 static void pm_dev_dbg(struct device *dev, pm_message_t state, char *info)
 {
 	dev_dbg(dev, "%s%s%s\n", info, pm_verb(state.event),
@@ -580,6 +605,9 @@ static int device_resume_noirq(struct de
 void dpm_resume_noirq(pm_message_t state)
 {
 	struct device *dev;
+	struct timeval start, stop;
+
+	do_gettimeofday(&start);
 
 	mutex_lock(&dpm_list_mtx);
 	transition_started = false;
@@ -595,6 +623,10 @@ void dpm_resume_noirq(pm_message_t state
 		}
 	dpm_synchronize_noirq();
 	mutex_unlock(&dpm_list_mtx);
+
+	do_gettimeofday(&stop);
+	dpm_show_time(&start, &stop, state, "EARLY");
+
 	resume_device_irqs();
 }
 EXPORT_SYMBOL_GPL(dpm_resume_noirq);
@@ -740,6 +772,9 @@ static int device_resume(struct device *
 static void dpm_resume(pm_message_t state)
 {
 	struct list_head list;
+	struct timeval start, stop;
+
+	do_gettimeofday(&start);
 
 	INIT_LIST_HEAD(&list);
 	mutex_lock(&dpm_list_mtx);
@@ -770,6 +805,9 @@ static void dpm_resume(pm_message_t stat
 	list_splice(&list, &dpm_list);
 	mutex_unlock(&dpm_list_mtx);
 	dpm_synchronize();
+
+	do_gettimeofday(&stop);
+	dpm_show_time(&start, &stop, state, NULL);
 }
 
 /**
@@ -985,8 +1023,11 @@ static int device_suspend_noirq(struct d
 int dpm_suspend_noirq(pm_message_t state)
 {
 	struct device *dev;
+	struct timeval start, stop;
 	int error = 0;
 
+	do_gettimeofday(&start);
+
 	suspend_device_irqs();
 	mutex_lock(&dpm_list_mtx);
 	pm_transition = state;
@@ -1004,8 +1045,12 @@ int dpm_suspend_noirq(pm_message_t state
 	}
 	dpm_synchronize_noirq();
 	mutex_unlock(&dpm_list_mtx);
-	if (error)
+	if (error) {
 		dpm_resume_noirq(resume_event(state));
+	} else {
+		do_gettimeofday(&stop);
+		dpm_show_time(&start, &stop, state, "LATE");
+	}
 	return error;
 }
 EXPORT_SYMBOL_GPL(dpm_suspend_noirq);
@@ -1157,8 +1202,11 @@ static int device_suspend(struct device 
 static int dpm_suspend(pm_message_t state)
 {
 	struct list_head list;
+	struct timeval start, stop;
 	int error = 0;
 
+	do_gettimeofday(&start);
+
 	INIT_LIST_HEAD(&list);
 	mutex_lock(&dpm_list_mtx);
 	pm_transition = state;
@@ -1188,6 +1236,12 @@ static int dpm_suspend(pm_message_t stat
 	list_splice(&list, dpm_list.prev);
 	mutex_unlock(&dpm_list_mtx);
 	dpm_synchronize();
+
+	if (!error) {
+		do_gettimeofday(&stop);
+		dpm_show_time(&start, &stop, state, NULL);
+	}
+
 	return error;
 }
 
Index: linux-2.6/include/linux/pm.h
===================================================================
--- linux-2.6.orig/include/linux/pm.h
+++ linux-2.6/include/linux/pm.h
@@ -505,6 +505,9 @@ extern int sysdev_suspend(pm_message_t s
 extern int dpm_suspend_noirq(pm_message_t state);
 extern int dpm_suspend_start(pm_message_t state);
 
+struct timeval;
+extern int pm_time_elapsed(struct timeval *start, struct timeval *stop);
+
 extern void __suspend_report_result(const char *function, void *fn, int ret);
 
 #define suspend_report_result(fn, ret)					\