[PATCH v2] driver core: shut down devices asynchronously

[PATCH v2] driver core: shut down devices asynchronously

[Stuart Hayes]

Shut down devices asynchronously, ensuring that each device is shut down
before its parents.

This can dramatically reduce system shutdown/reboot time on systems that
have devices that take many seconds to shut down, such as some NVMe drives.
On one system tested, the shutdown time went from 11 minutes without this
patch to 55 seconds with the patch.

Signed-off-by: Stuart Hayes <stuart.w.hayes@gmail.com>
---
v1->v2: rewritten using kernel async code (suggested by Lukas Wunner)
---

 drivers/base/base.h   |   2 +
 drivers/base/core.c   | 135 ++++++++++++++++++++++++------------------
 include/linux/async.h |   6 ++
 3 files changed, 87 insertions(+), 56 deletions(-)

diff --git a/drivers/base/base.h b/drivers/base/base.h
index eb4c0ace9242..e193d48d9acc 100644
--- a/drivers/base/base.h
+++ b/drivers/base/base.h
@@ -11,6 +11,7 @@
  *
  */
 #include <linux/notifier.h>
+#include <linux/async.h>
 
 /**
  * struct subsys_private - structure to hold the private to the driver core portions of the bus_type/class structure.
@@ -114,6 +115,7 @@ struct device_private {
 	struct list_head deferred_probe;
 	struct device_driver *async_driver;
 	char *deferred_probe_reason;
+	struct async_domain children_shutdown;
 	struct device *device;
 	u8 dead:1;
 };
diff --git a/drivers/base/core.c b/drivers/base/core.c
index b7d7f410c256..d122788f2944 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -9,6 +9,7 @@
  */
 
 #include <linux/acpi.h>
+#include <linux/async.h>
 #include <linux/cpufreq.h>
 #include <linux/device.h>
 #include <linux/err.h>
@@ -4716,76 +4717,98 @@ int device_change_owner(struct device *dev, kuid_t kuid, kgid_t kgid)
 }
 EXPORT_SYMBOL_GPL(device_change_owner);
 
+static void shutdown_device(void *data, async_cookie_t cookie)
+{
+	struct device *dev = data;
+
+	async_synchronize_full_domain(&dev->p->children_shutdown);
+
+	/*
+	 * Make sure the device is off the kset list, in the
+	 * event that dev->*->shutdown() doesn't remove it.
+	 */
+	spin_lock(&devices_kset->list_lock);
+	list_del_init(&dev->kobj.entry);
+	spin_unlock(&devices_kset->list_lock);
+
+	/* hold lock to avoid race with probe/release */
+	if (dev->parent)
+		device_lock(dev->parent);
+	device_lock(dev);
+
+	/* Don't allow any more runtime suspends */
+	pm_runtime_get_noresume(dev);
+	pm_runtime_barrier(dev);
+
+	if (dev->class && dev->class->shutdown_pre) {
+		if (initcall_debug)
+			dev_info(dev, "shutdown_pre\n");
+		dev->class->shutdown_pre(dev);
+	}
+	if (dev->bus && dev->bus->shutdown) {
+		if (initcall_debug)
+			dev_info(dev, "shutdown\n");
+		dev->bus->shutdown(dev);
+	} else if (dev->driver && dev->driver->shutdown) {
+		if (initcall_debug)
+			dev_info(dev, "shutdown\n");
+		dev->driver->shutdown(dev);
+	}
+
+	device_unlock(dev);
+	if (dev->parent)
+		device_unlock(dev->parent);
+
+	put_device(dev);
+}
+
+/**
+ * device_shutdown_async - schedule ->shutdown() on each device to shutdown
+ * asynchronously, ensuring each device's children are shut down before
+ * shutting down the device
+ */
+static int shutdown_device_and_children(struct device *dev, void *data)
+{
+	struct async_domain *domain = data;
+	struct device_private *p = dev->p;
+
+	INIT_ASYNC_DOMAIN_EXCLUSIVE(&p->children_shutdown);
+
+	/* make sure device doesn't go away before it is shut down */
+	get_device(dev);
+
+	/* schedule shutdown of children */
+	device_for_each_child(dev, &p->children_shutdown,
+			      shutdown_device_and_children);
+
+	/* schedule shutdown of this device */
+	async_schedule_domain(shutdown_device, dev, domain);
+
+	return 0;
+}
+
 /**
  * device_shutdown - call ->shutdown() on each device to shutdown.
  */
 void device_shutdown(void)
 {
-	struct device *dev, *parent;
+	ASYNC_DOMAIN_EXCLUSIVE(shutdown_domain);
+	struct device *dev, *temp;
 
 	wait_for_device_probe();
 	device_block_probing();
 
 	cpufreq_suspend();
 
-	spin_lock(&devices_kset->list_lock);
 	/*
-	 * Walk the devices list backward, shutting down each in turn.
-	 * Beware that device unplug events may also start pulling
-	 * devices offline, even as the system is shutting down.
+	 * schedule shutdown of top level devices
 	 */
-	while (!list_empty(&devices_kset->list)) {
-		dev = list_entry(devices_kset->list.prev, struct device,
-				kobj.entry);
-
-		/*
-		 * hold reference count of device's parent to
-		 * prevent it from being freed because parent's
-		 * lock is to be held
-		 */
-		parent = get_device(dev->parent);
-		get_device(dev);
-		/*
-		 * Make sure the device is off the kset list, in the
-		 * event that dev->*->shutdown() doesn't remove it.
-		 */
-		list_del_init(&dev->kobj.entry);
-		spin_unlock(&devices_kset->list_lock);
-
-		/* hold lock to avoid race with probe/release */
-		if (parent)
-			device_lock(parent);
-		device_lock(dev);
-
-		/* Don't allow any more runtime suspends */
-		pm_runtime_get_noresume(dev);
-		pm_runtime_barrier(dev);
-
-		if (dev->class && dev->class->shutdown_pre) {
-			if (initcall_debug)
-				dev_info(dev, "shutdown_pre\n");
-			dev->class->shutdown_pre(dev);
-		}
-		if (dev->bus && dev->bus->shutdown) {
-			if (initcall_debug)
-				dev_info(dev, "shutdown\n");
-			dev->bus->shutdown(dev);
-		} else if (dev->driver && dev->driver->shutdown) {
-			if (initcall_debug)
-				dev_info(dev, "shutdown\n");
-			dev->driver->shutdown(dev);
-		}
-
-		device_unlock(dev);
-		if (parent)
-			device_unlock(parent);
-
-		put_device(dev);
-		put_device(parent);
-
-		spin_lock(&devices_kset->list_lock);
+	list_for_each_entry_safe_reverse(dev, temp, &devices_kset->list, kobj.entry) {
+		if (dev->parent)
+			continue;
+		shutdown_device_and_children(dev, &shutdown_domain);
 	}
-	spin_unlock(&devices_kset->list_lock);
+	async_synchronize_full_domain(&shutdown_domain);
 }
 
 /*
diff --git a/include/linux/async.h b/include/linux/async.h
index cce4ad31e8fc..ab62402452f8 100644
--- a/include/linux/async.h
+++ b/include/linux/async.h
@@ -35,6 +35,12 @@ struct async_domain {
 	struct async_domain _name = { .pending = LIST_HEAD_INIT(_name.pending), \
 				      .registered = 0 }
 
+static inline void INIT_ASYNC_DOMAIN_EXCLUSIVE(struct async_domain *domain)
+{
+	INIT_LIST_HEAD(&domain->pending);
+	domain->registered = 0;
+}
+
 async_cookie_t async_schedule_node(async_func_t func, void *data,
 				   int node);
 async_cookie_t async_schedule_node_domain(async_func_t func, void *data,
-- 
2.39.3

Re: [PATCH v2] driver core: shut down devices asynchronously

[kernel test robot]

Hi Stuart,

kernel test robot noticed the following build warnings:

[auto build test WARNING on driver-core/driver-core-testing]
[also build test WARNING on driver-core/driver-core-next driver-core/driver-core-linus linus/master v6.6-rc1 next-20230913]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Stuart-Hayes/driver-core-shut-down-devices-asynchronously/20230914-050611
base:   driver-core/driver-core-testing
patch link:    https://lore.kernel.org/r/20230913210516.3545-1-stuart.w.hayes%40gmail.com
patch subject: [PATCH v2] driver core: shut down devices asynchronously
config: parisc-allyesconfig (https://download.01.org/0day-ci/archive/20230914/202309141407.CpZtIa2w-lkp@intel.com/config)
compiler: hppa-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20230914/202309141407.CpZtIa2w-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202309141407.CpZtIa2w-lkp@intel.com/

All warnings (new ones prefixed by >>):

   drivers/base/core.c:68: warning: Function parameter or member 'flags' not described in '__fwnode_link_add'
>> drivers/base/core.c:4771: warning: Function parameter or member 'dev' not described in 'shutdown_device_and_children'
>> drivers/base/core.c:4771: warning: Function parameter or member 'data' not described in 'shutdown_device_and_children'
>> drivers/base/core.c:4771: warning: expecting prototype for device_shutdown_async(). Prototype was for shutdown_device_and_children() instead


vim +4771 drivers/base/core.c

  4764	
  4765	/**
  4766	 * device_shutdown_async - schedule ->shutdown() on each device to shutdown
  4767	 * asynchronously, ensuring each device's children are shut down before
  4768	 * shutting down the device
  4769	 */
  4770	static int shutdown_device_and_children(struct device *dev, void *data)
> 4771	{
  4772		struct async_domain *domain = data;
  4773		struct device_private *p = dev->p;
  4774	
  4775		INIT_ASYNC_DOMAIN_EXCLUSIVE(&p->children_shutdown);
  4776	
  4777		/* make sure device doesn't go away before it is shut down */
  4778		get_device(dev);
  4779	
  4780		/* schedule shutdown of children */
  4781		device_for_each_child(dev, &p->children_shutdown,
  4782				      shutdown_device_and_children);
  4783	
  4784		/* schedule shutdown of this device */
  4785		async_schedule_domain(shutdown_device, dev, domain);
  4786	
  4787		return 0;
  4788	}
  4789	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v2] driver core: shut down devices asynchronously

[stuart hayes]

On 9/13/2023 4:05 PM, Stuart Hayes wrote:
> Shut down devices asynchronously, ensuring that each device is shut down
> before its parents.
> 
> This can dramatically reduce system shutdown/reboot time on systems that
> have devices that take many seconds to shut down, such as some NVMe drives.
> On one system tested, the shutdown time went from 11 minutes without this
> patch to 55 seconds with the patch.
> 
> Signed-off-by: Stuart Hayes <stuart.w.hayes@gmail.com>
> ---
> v1->v2: rewritten using kernel async code (suggested by Lukas Wunner)
> ---
> 
>   drivers/base/base.h   |   2 +
>   drivers/base/core.c   | 135 ++++++++++++++++++++++++------------------
>   include/linux/async.h |   6 ++
>   3 files changed, 87 insertions(+), 56 deletions(-)
> 

Please disregard this patch... I realized this could be done in a simpler way.  I'll send a V3 this week.