| Message ID | 20240207184100.18066-2-djeffery@redhat.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | async device shutdown support | expand |
On Wed, Feb 7, 2024 at 10:40 AM David Jeffery <djeffery@redhat.com> wrote: > > Adds the async_shutdown_start and async_shutdown_end calls to perform async > shutdown. Implements a very minimalist method of async shutdown support > within device_shutdown(). The device at the head of the shutdown list is > checked against a list of devices under async shutdown. If the head is a > parent of a device on the async list, all active async shutdown operations > are completed before the parent's shutdown call is performed. > > The number of async operations also has a max limit to prevent the list being > checked for a child from getting overly large. > > Signed-off-by: David Jeffery <djeffery@redhat.com> > Tested-by: Laurence Oberman <loberman@redhat.com> > > --- > drivers/base/core.c | 116 +++++++++++++++++++++++++++++++++- > include/linux/device/bus.h | 8 ++- > include/linux/device/driver.h | 7 ++ > 3 files changed, 127 insertions(+), 4 deletions(-) > > diff --git a/drivers/base/core.c b/drivers/base/core.c > index 14d46af40f9a..5bc2282c00cd 100644 > --- a/drivers/base/core.c > +++ b/drivers/base/core.c > @@ -4719,12 +4719,92 @@ int device_change_owner(struct device *dev, kuid_t kuid, kgid_t kgid) > } > EXPORT_SYMBOL_GPL(device_change_owner); > > + > +#define MAX_ASYNC_SHUTDOWNS 32 > +static int async_shutdown_count; > +static LIST_HEAD(async_shutdown_list); > + > +/** > + * If a device has a child busy with an async shutdown or there are too many > + * async shutdowns active, the device may not be shut down at this time. > + */ > +static bool may_shutdown_device(struct device *dev) > +{ > + struct device *tmp; > + > + if (async_shutdown_count >= MAX_ASYNC_SHUTDOWNS) > + return false; > + > + list_for_each_entry(tmp, &async_shutdown_list, kobj.entry) { > + if (tmp->parent == dev) > + return false; > + } > + return true; > +} > + > +/** > + * Call and track each async shutdown call > + */ > +static void async_shutdown_start(struct device *dev, void (*callback) (struct device *)) > +{ > + if (initcall_debug) > + dev_info(dev, "async_shutdown_start\n"); > + > + (*callback)(dev); > + list_add_tail(&dev->kobj.entry, &async_shutdown_list); > + async_shutdown_count++; > +} > + > +/** > + * Wait for all async shutdown operations currently active to complete > + */ > +static void wait_for_active_async_shutdown(void) > +{ > + struct device *dev, *parent; > + > + while (!list_empty(&async_shutdown_list)) { > + dev = list_entry(async_shutdown_list.next, struct device, > + kobj.entry); > + > + parent = dev->parent; I didn't check the code thoroughly, but so there might be other big issues. But you definitely need to take device links into account. Shutdown all your consumers first similar to how you shutdown the children devices first. Look at the async suspend/resume code for some guidance. > + > + /* > + * Make sure the device is off the list > + */ > + list_del_init(&dev->kobj.entry); > + if (parent) > + device_lock(parent); > + device_lock(dev); > + if (dev->bus && dev->bus->async_shutdown_end) { > + if (initcall_debug) > + dev_info(dev, > + "async_shutdown_end called\n"); > + dev->bus->async_shutdown_end(dev); > + } else if (dev->driver && dev->driver->async_shutdown_end) { > + if (initcall_debug) > + dev_info(dev, > + "async_shutdown_end called\n"); > + dev->driver->async_shutdown_end(dev); > + } > + device_unlock(dev); > + if (parent) > + device_unlock(parent); > + > + put_device(dev); > + put_device(parent); > + } > + if (initcall_debug) > + printk(KERN_INFO "device shutdown: waited for %d async shutdown callbacks\n", async_shutdown_count); > + async_shutdown_count = 0; > +} > + > /** > * device_shutdown - call ->shutdown() on each device to shutdown. > */ > void device_shutdown(void) > { > struct device *dev, *parent; > + bool async_busy; > > wait_for_device_probe(); > device_block_probing(); > @@ -4741,6 +4821,8 @@ void device_shutdown(void) > dev = list_entry(devices_kset->list.prev, struct device, > kobj.entry); > > + async_busy = false; > + > /* > * hold reference count of device's parent to > * prevent it from being freed because parent's > @@ -4748,6 +4830,17 @@ void device_shutdown(void) > */ > parent = get_device(dev->parent); > get_device(dev); > + > + if (!may_shutdown_device(dev)) { > + put_device(dev); > + put_device(parent); > + > + spin_unlock(&devices_kset->list_lock); > + wait_for_active_async_shutdown(); > + spin_lock(&devices_kset->list_lock); > + continue; > + } > + > /* > * Make sure the device is off the kset list, in the > * event that dev->*->shutdown() doesn't remove it. > @@ -4769,26 +4862,43 @@ void device_shutdown(void) > dev_info(dev, "shutdown_pre\n"); > dev->class->shutdown_pre(dev); > } > - if (dev->bus && dev->bus->shutdown) { > + if (dev->bus && dev->bus->async_shutdown_start) { > + async_shutdown_start(dev, dev->bus->async_shutdown_start); > + async_busy = true; > + } else if (dev->bus && dev->bus->shutdown) { > if (initcall_debug) > dev_info(dev, "shutdown\n"); > dev->bus->shutdown(dev); > + } else if (dev->driver && dev->driver->async_shutdown_start) { > + async_shutdown_start(dev, dev->driver->async_shutdown_start); > + async_busy = true; > } else if (dev->driver && dev->driver->shutdown) { > if (initcall_debug) > dev_info(dev, "shutdown\n"); > dev->driver->shutdown(dev); > + } else { > + if (initcall_debug) > + dev_info(dev, "no shutdown callback\n"); > } > > device_unlock(dev); > if (parent) > device_unlock(parent); > > - put_device(dev); > - put_device(parent); > + /* if device has an async shutdown, drop the ref when done */ > + if (!async_busy) { > + put_device(dev); > + put_device(parent); > + } > > spin_lock(&devices_kset->list_lock); > } > spin_unlock(&devices_kset->list_lock); > + /* > + * Wait for any async shutdown still running. > + */ > + if (!list_empty(&async_shutdown_list)) > + wait_for_active_async_shutdown(); > } > > /* > diff --git a/include/linux/device/bus.h b/include/linux/device/bus.h > index 5ef4ec1c36c3..7a4a2ff0bc23 100644 > --- a/include/linux/device/bus.h > +++ b/include/linux/device/bus.h > @@ -48,7 +48,11 @@ struct fwnode_handle; > * will never get called until they do. > * @remove: Called when a device removed from this bus. > * @shutdown: Called at shut-down time to quiesce the device. > - * > + * @async_shutdown_start: Optional call to support and begin the shutdown > + * process on the device in an asynchronous manner. > + * @async_shutdown_end: Optional call to complete an asynchronous > + * shutdown of the device. Must be provided if a > + * sync_shutdown_start call is provided. > * @online: Called to put the device back online (after offlining it). > * @offline: Called to put the device offline for hot-removal. May fail. > * > @@ -87,6 +91,8 @@ struct bus_type { > void (*sync_state)(struct device *dev); > void (*remove)(struct device *dev); > void (*shutdown)(struct device *dev); > + void (*async_shutdown_start)(struct device *dev); > + void (*async_shutdown_end)(struct device *dev); > > int (*online)(struct device *dev); > int (*offline)(struct device *dev); > diff --git a/include/linux/device/driver.h b/include/linux/device/driver.h > index 7738f458995f..af0ad2d3687a 100644 > --- a/include/linux/device/driver.h > +++ b/include/linux/device/driver.h > @@ -71,6 +71,11 @@ enum probe_type { > * @remove: Called when the device is removed from the system to > * unbind a device from this driver. > * @shutdown: Called at shut-down time to quiesce the device. > + * @async_shutdown_start: Optional call to support and begin the shutdown > + * process on the device in an asynchronous manner. > + * @async_shutdown_end: Optional call to complete an asynchronous > + * shutdown of the device. Must be provided if a > + * sync_shutdown_start call is provided. > * @suspend: Called to put the device to sleep mode. Usually to a > * low power state. > * @resume: Called to bring a device from sleep mode. > @@ -110,6 +115,8 @@ struct device_driver { > void (*sync_state)(struct device *dev); > int (*remove) (struct device *dev); > void (*shutdown) (struct device *dev); > + void (*async_shutdown_start) (struct device *dev); > + void (*async_shutdown_end) (struct device *dev); Why not use the existing shutdown and call it from an async thread and wait for it to finish? Similar to how async probes are handled. Also, adding separate ops for this feels clunky and a very narrow fix. Just use a flag to indicate the driver can support async shutdown using the existing shutdown() op. -Saravana
On Tue, Feb 13, 2024 at 10:43 PM Saravana Kannan <saravanak@google.com> wrote: > > On Wed, Feb 7, 2024 at 10:40 AM David Jeffery <djeffery@redhat.com> wrote: > > > > + * Wait for all async shutdown operations currently active to complete > > + */ > > +static void wait_for_active_async_shutdown(void) > > +{ > > + struct device *dev, *parent; > > + > > + while (!list_empty(&async_shutdown_list)) { > > + dev = list_entry(async_shutdown_list.next, struct device, > > + kobj.entry); > > + > > + parent = dev->parent; > > I didn't check the code thoroughly, but so there might be other big > issues. But you definitely need to take device links into account. > Shutdown all your consumers first similar to how you shutdown the > children devices first. Look at the async suspend/resume code for some > guidance. > Sure, I'll work on adding that into the order rules. > > @@ -110,6 +115,8 @@ struct device_driver { > > void (*sync_state)(struct device *dev); > > int (*remove) (struct device *dev); > > void (*shutdown) (struct device *dev); > > + void (*async_shutdown_start) (struct device *dev); > > + void (*async_shutdown_end) (struct device *dev); > > Why not use the existing shutdown and call it from an async thread and > wait for it to finish? Similar to how async probes are handled. Also, > adding separate ops for this feels clunky and a very narrow fix. Just > use a flag to indicate the driver can support async shutdown using the > existing shutdown() op. > It is rather clunky. It was carried from older patches where I mistakenly thought people wanted this separate interface. And adding threads seemed like overkill. Others have been working on similar patches on linux-nvme that I was unaware of. They add an optional shutdown_wait call instead of this interface. I had planned on adapting to work with their interface design. David Jeffery
diff --git a/drivers/base/core.c b/drivers/base/core.c index 14d46af40f9a..5bc2282c00cd 100644 --- a/drivers/base/core.c +++ b/drivers/base/core.c @@ -4719,12 +4719,92 @@ int device_change_owner(struct device *dev, kuid_t kuid, kgid_t kgid) } EXPORT_SYMBOL_GPL(device_change_owner); + +#define MAX_ASYNC_SHUTDOWNS 32 +static int async_shutdown_count; +static LIST_HEAD(async_shutdown_list); + +/** + * If a device has a child busy with an async shutdown or there are too many + * async shutdowns active, the device may not be shut down at this time. + */ +static bool may_shutdown_device(struct device *dev) +{ + struct device *tmp; + + if (async_shutdown_count >= MAX_ASYNC_SHUTDOWNS) + return false; + + list_for_each_entry(tmp, &async_shutdown_list, kobj.entry) { + if (tmp->parent == dev) + return false; + } + return true; +} + +/** + * Call and track each async shutdown call + */ +static void async_shutdown_start(struct device *dev, void (*callback) (struct device *)) +{ + if (initcall_debug) + dev_info(dev, "async_shutdown_start\n"); + + (*callback)(dev); + list_add_tail(&dev->kobj.entry, &async_shutdown_list); + async_shutdown_count++; +} + +/** + * Wait for all async shutdown operations currently active to complete + */ +static void wait_for_active_async_shutdown(void) +{ + struct device *dev, *parent; + + while (!list_empty(&async_shutdown_list)) { + dev = list_entry(async_shutdown_list.next, struct device, + kobj.entry); + + parent = dev->parent; + + /* + * Make sure the device is off the list + */ + list_del_init(&dev->kobj.entry); + if (parent) + device_lock(parent); + device_lock(dev); + if (dev->bus && dev->bus->async_shutdown_end) { + if (initcall_debug) + dev_info(dev, + "async_shutdown_end called\n"); + dev->bus->async_shutdown_end(dev); + } else if (dev->driver && dev->driver->async_shutdown_end) { + if (initcall_debug) + dev_info(dev, + "async_shutdown_end called\n"); + dev->driver->async_shutdown_end(dev); + } + device_unlock(dev); + if (parent) + device_unlock(parent); + + put_device(dev); + put_device(parent); + } + if (initcall_debug) + printk(KERN_INFO "device shutdown: waited for %d async shutdown callbacks\n", async_shutdown_count); + async_shutdown_count = 0; +} + /** * device_shutdown - call ->shutdown() on each device to shutdown. */ void device_shutdown(void) { struct device *dev, *parent; + bool async_busy; wait_for_device_probe(); device_block_probing(); @@ -4741,6 +4821,8 @@ void device_shutdown(void) dev = list_entry(devices_kset->list.prev, struct device, kobj.entry); + async_busy = false; + /* * hold reference count of device's parent to * prevent it from being freed because parent's @@ -4748,6 +4830,17 @@ void device_shutdown(void) */ parent = get_device(dev->parent); get_device(dev); + + if (!may_shutdown_device(dev)) { + put_device(dev); + put_device(parent); + + spin_unlock(&devices_kset->list_lock); + wait_for_active_async_shutdown(); + spin_lock(&devices_kset->list_lock); + continue; + } + /* * Make sure the device is off the kset list, in the * event that dev->*->shutdown() doesn't remove it. @@ -4769,26 +4862,43 @@ void device_shutdown(void) dev_info(dev, "shutdown_pre\n"); dev->class->shutdown_pre(dev); } - if (dev->bus && dev->bus->shutdown) { + if (dev->bus && dev->bus->async_shutdown_start) { + async_shutdown_start(dev, dev->bus->async_shutdown_start); + async_busy = true; + } else if (dev->bus && dev->bus->shutdown) { if (initcall_debug) dev_info(dev, "shutdown\n"); dev->bus->shutdown(dev); + } else if (dev->driver && dev->driver->async_shutdown_start) { + async_shutdown_start(dev, dev->driver->async_shutdown_start); + async_busy = true; } else if (dev->driver && dev->driver->shutdown) { if (initcall_debug) dev_info(dev, "shutdown\n"); dev->driver->shutdown(dev); + } else { + if (initcall_debug) + dev_info(dev, "no shutdown callback\n"); } device_unlock(dev); if (parent) device_unlock(parent); - put_device(dev); - put_device(parent); + /* if device has an async shutdown, drop the ref when done */ + if (!async_busy) { + put_device(dev); + put_device(parent); + } spin_lock(&devices_kset->list_lock); } spin_unlock(&devices_kset->list_lock); + /* + * Wait for any async shutdown still running. + */ + if (!list_empty(&async_shutdown_list)) + wait_for_active_async_shutdown(); } /* diff --git a/include/linux/device/bus.h b/include/linux/device/bus.h index 5ef4ec1c36c3..7a4a2ff0bc23 100644 --- a/include/linux/device/bus.h +++ b/include/linux/device/bus.h @@ -48,7 +48,11 @@ struct fwnode_handle; * will never get called until they do. * @remove: Called when a device removed from this bus. * @shutdown: Called at shut-down time to quiesce the device. - * + * @async_shutdown_start: Optional call to support and begin the shutdown + * process on the device in an asynchronous manner. + * @async_shutdown_end: Optional call to complete an asynchronous + * shutdown of the device. Must be provided if a + * sync_shutdown_start call is provided. * @online: Called to put the device back online (after offlining it). * @offline: Called to put the device offline for hot-removal. May fail. * @@ -87,6 +91,8 @@ struct bus_type { void (*sync_state)(struct device *dev); void (*remove)(struct device *dev); void (*shutdown)(struct device *dev); + void (*async_shutdown_start)(struct device *dev); + void (*async_shutdown_end)(struct device *dev); int (*online)(struct device *dev); int (*offline)(struct device *dev); diff --git a/include/linux/device/driver.h b/include/linux/device/driver.h index 7738f458995f..af0ad2d3687a 100644 --- a/include/linux/device/driver.h +++ b/include/linux/device/driver.h @@ -71,6 +71,11 @@ enum probe_type { * @remove: Called when the device is removed from the system to * unbind a device from this driver. * @shutdown: Called at shut-down time to quiesce the device. + * @async_shutdown_start: Optional call to support and begin the shutdown + * process on the device in an asynchronous manner. + * @async_shutdown_end: Optional call to complete an asynchronous + * shutdown of the device. Must be provided if a + * sync_shutdown_start call is provided. * @suspend: Called to put the device to sleep mode. Usually to a * low power state. * @resume: Called to bring a device from sleep mode. @@ -110,6 +115,8 @@ struct device_driver { void (*sync_state)(struct device *dev); int (*remove) (struct device *dev); void (*shutdown) (struct device *dev); + void (*async_shutdown_start) (struct device *dev); + void (*async_shutdown_end) (struct device *dev); int (*suspend) (struct device *dev, pm_message_t state); int (*resume) (struct device *dev); const struct attribute_group **groups;