[PATCH v4] driver core: shut down devices asynchronously

[PATCH v4] 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)
v2->v3: removed recursive functions to schedule children to be shutdown
        before parents, since existing device_shutdown loop will already
        do this
v3->v4: bug fix (used "parent" not "dev->parent", in device_shutdown)
---
 drivers/base/base.h   |   3 ++
 drivers/base/core.c   | 104 ++++++++++++++++++++++++++----------------
 include/linux/async.h |   6 +++
 3 files changed, 74 insertions(+), 39 deletions(-)

diff --git a/drivers/base/base.h b/drivers/base/base.h
index eb4c0ace9242..9b9d80e575ca 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.
@@ -97,6 +98,7 @@ struct driver_private {
  *	the device; typically because it depends on another driver getting
  *	probed first.
  * @async_driver - pointer to device driver awaiting probe via async_probe
+ * @child_domain - domain for async shutdown work of children
  * @device - pointer back to the struct device that this structure is
  * associated with.
  * @dead - This device is currently either in the process of or has been
@@ -114,6 +116,7 @@ struct device_private {
 	struct list_head deferred_probe;
 	struct device_driver *async_driver;
 	char *deferred_probe_reason;
+	struct async_domain child_domain;
 	struct device *device;
 	u8 dead:1;
 };
diff --git a/drivers/base/core.c b/drivers/base/core.c
index 4d8b315c48a1..e939ff5269a4 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>
@@ -3473,6 +3474,7 @@ static int device_private_init(struct device *dev)
 	klist_init(&dev->p->klist_children, klist_children_get,
 		   klist_children_put);
 	INIT_LIST_HEAD(&dev->p->deferred_probe);
+	INIT_ASYNC_DOMAIN_EXCLUSIVE(&dev->p->child_domain);
 	return 0;
 }
 
@@ -4718,12 +4720,69 @@ int device_change_owner(struct device *dev, kuid_t kuid, kgid_t kgid)
 }
 EXPORT_SYMBOL_GPL(device_change_owner);
 
+/**
+ * shutdown_device - shutdown one device
+ * @data: the pointer to the struct device to be shutdown
+ * @cookie: not used
+ *
+ * This shuts down one device, after waiting for children to finish
+ * shutdown.  This should be scheduled for any children first.
+ */
+static void shutdown_device(void *data, async_cookie_t cookie)
+{
+	struct device *dev = data;
+
+	/*
+	 * wait for shutdown work of children to finish
+	 */
+	async_synchronize_full_domain(&dev->p->child_domain);
+
+	/*
+	 * 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 - call ->shutdown() on each device to shutdown.
  */
 void device_shutdown(void)
 {
-	struct device *dev, *parent;
+	ASYNC_DOMAIN_EXCLUSIVE(top_domain);
+	struct device *dev;
 
 	wait_for_device_probe();
 	device_block_probing();
@@ -4732,20 +4791,14 @@ void device_shutdown(void)
 
 	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
+	 * Walk the devices list backward, scheduling shutdown of each in
+	 * turn. Beware that device unplug events may also start pulling
 	 * devices offline, even as the system is shutting down.
 	 */
 	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
@@ -4754,40 +4807,13 @@ void device_shutdown(void)
 		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);
+		async_schedule_domain(shutdown_device, dev,
+			dev->parent ? &dev->parent->p->child_domain : &top_domain);
 
 		spin_lock(&devices_kset->list_lock);
 	}
 	spin_unlock(&devices_kset->list_lock);
+	async_synchronize_full_domain(&top_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 v4] driver core: shut down devices asynchronously

[Greg Kroah-Hartman]

On Thu, Sep 21, 2023 at 11:34:43AM -0500, 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.

That's a nice improvement, but I think we need a lot more testing on a
wide range of systems before we can take a patch like this.

Also, what about busses that don't want this type of shutdown?  We allow
busses to opt-in for async probing, shouldn't that be also done for
shutting them down to resolve issues for busses that can not handle
this?

thanks,

greg k-h

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

[stuart hayes]

On 10/5/2023 4:36 AM, Greg Kroah-Hartman wrote:
> On Thu, Sep 21, 2023 at 11:34:43AM -0500, 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.
> 
> That's a nice improvement, but I think we need a lot more testing on a
> wide range of systems before we can take a patch like this.
> 
> Also, what about busses that don't want this type of shutdown?  We allow
> busses to opt-in for async probing, shouldn't that be also done for
> shutting them down to resolve issues for busses that can not handle
> this?
> 
> thanks,
> 
> greg k-h

Yes, I could add something like what is done for async probing, so drivers
have to opt in to async shutdown of their devices.

But I'm not sure how to get it tested on a wide range of systems, other than
than having the patch in the kernel.  What if it defaults to synchronous
shutdown for now, but the option is there so people are able to test async
shutdown by changing an attribute in sysfs?  Then drivers could be patched
later to opt in to async shutdown of their devices by default...?

Thanks for the feedback!

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

[Greg Kroah-Hartman]

On Thu, Oct 19, 2023 at 03:40:48PM -0500, stuart hayes wrote:
> 
> 
> On 10/5/2023 4:36 AM, Greg Kroah-Hartman wrote:
> > On Thu, Sep 21, 2023 at 11:34:43AM -0500, 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.
> > 
> > That's a nice improvement, but I think we need a lot more testing on a
> > wide range of systems before we can take a patch like this.
> > 
> > Also, what about busses that don't want this type of shutdown?  We allow
> > busses to opt-in for async probing, shouldn't that be also done for
> > shutting them down to resolve issues for busses that can not handle
> > this?
> > 
> > thanks,
> > 
> > greg k-h
> 
> Yes, I could add something like what is done for async probing, so drivers
> have to opt in to async shutdown of their devices.
> 
> But I'm not sure how to get it tested on a wide range of systems, other than
> than having the patch in the kernel.

Yes, breaking things is the only way you will know :)

> What if it defaults to synchronous
> shutdown for now, but the option is there so people are able to test async
> shutdown by changing an attribute in sysfs?  Then drivers could be patched
> later to opt in to async shutdown of their devices by default...?

Yes, that will be the proper way forward, default to what you know works
today, and then busses can opt-in for the async option if they work
properly.  That's what we did with probing, so it can mirror that.

thanks,

greg k-h