| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20250130030625.4461-1-sultan@kerneltoast.com>
Date: Wed, 29 Jan 2025 19:06:25 -0800
From: Sultan Alsawaf <sultan@...neltoast.com>
To: mhklinux@...look.com
Cc: Martin.Belanger@...l.com,
axboe@...com,
djeffery@...hat.com,
dwagner@...e.de,
gregkh@...uxfoundation.org,
hch@....de,
jallison@....com,
jan.kiszka@...mens.com,
kbusch@...nel.org,
linux-kernel@...r.kernel.org,
linux-nvme@...ts.infradead.org,
lukas@...ner.de,
nathan@...nel.org,
oohall@...il.com,
rafael@...nel.org,
sagi@...mberg.me,
spasswolf@....de,
stuart.w.hayes@...il.com
Subject: [PATCH] driver core: optimize async device shutdown
From: Sultan Alsawaf <sultan@...neltoast.com>
The async device shutdown scaffolding assumes any device may be async and
thus schedules an async worker for all devices. This can result in massive
workqueue overhead at shutdown time, even when no async devices are
present.
Optimize async device shutdown by only scheduling async workers for devices
advertising async shutdown support.
This also fixes the (data) race on shutdown_after so that shutdown_after
isn't modified for a device which has already been scheduled for async
shutdown.
Signed-off-by: Sultan Alsawaf <sultan@...neltoast.com>
---
Hi all,
I've created this follow-on patch to address the overhead problem. With this
patch applied on top of the v9 series, workers are only ever spawned for devices
which request async shutdown. This should also improve performance in the async
shutdown case.
Cheers,
Sultan
---
drivers/base/base.h | 4 +-
drivers/base/core.c | 136 ++++++++++++++++++++++++++++----------------
2 files changed, 89 insertions(+), 51 deletions(-)
diff --git a/drivers/base/base.h b/drivers/base/base.h
index ea18aa70f151..28eb9ffe3ff6 100644
--- a/drivers/base/base.h
+++ b/drivers/base/base.h
@@ -105,6 +105,7 @@ struct driver_private {
* @dead - This device is currently either in the process of or has been
* removed from the system. Any asynchronous events scheduled for this
* device should exit without taking any action.
+ * @async_shutdown_queued - indicates async shutdown is enqueued for this device
*
* Nothing outside of the driver core should ever touch these fields.
*/
@@ -119,7 +120,8 @@ struct device_private {
char *deferred_probe_reason;
async_cookie_t shutdown_after;
struct device *device;
- u8 dead:1;
+ u8 dead:1,
+ async_shutdown_queued:1;
};
#define to_device_private_parent(obj) \
container_of(obj, struct device_private, knode_parent)
diff --git a/drivers/base/core.c b/drivers/base/core.c
index df0df531b561..993285bc0d1a 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -3515,7 +3515,6 @@ 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);
- dev->p->shutdown_after = 0;
return 0;
}
@@ -4856,37 +4855,66 @@ static bool device_wants_async_shutdown(struct device *dev)
* @data: the pointer to the struct device to be shutdown
* @cookie: not used
*
- * Shuts down one device, after waiting for previous device to shut down (for
- * synchronous shutdown) or waiting for device's last child or consumer to
- * be shutdown (for async shutdown).
+ * Shuts down one device, after waiting for device's last child or consumer to
+ * be shutdown.
*
* shutdown_after is set to the shutdown cookie of the last child or consumer
* of this device (if any).
*/
static void shutdown_one_device_async(void *data, async_cookie_t cookie)
{
- struct device *dev = data;
- async_cookie_t wait = cookie;
+ struct device_private *p = data;
- if (device_wants_async_shutdown(dev)) {
- wait = dev->p->shutdown_after + 1;
+ if (p->shutdown_after)
+ async_synchronize_cookie_domain(p->shutdown_after, &sd_domain);
+
+ shutdown_one_device(p->device);
+}
+
+/* Assumes an extra reference to @dev and @dev->parent are acquired first */
+static void queue_device_async_shutdown(struct device *dev)
+{
+ struct device *parent = dev->parent;
+ struct device_link *link;
+ async_cookie_t cookie;
+ int idx;
+
+ /*
+ * Add one to this device's cookie so that when shutdown_after is passed
+ * to async_synchronize_cookie_domain(), it will wait until *after*
+ * shutdown_one_device_async() is finished running for this device.
+ */
+ cookie = async_schedule_domain(shutdown_one_device_async, dev->p,
+ &sd_domain) + 1;
+
+ /*
+ * Set async_shutdown_queued to avoid overwriting a parent's
+ * shutdown_after while the parent is shutting down. This can happen if
+ * a parent or supplier is not ordered in the devices_kset list before a
+ * child or consumer, which is not expected.
+ */
+ dev->p->async_shutdown_queued = 1;
+
+ /* Ensure any parent & suppliers wait for this device to shut down */
+ if (parent) {
+ if (!parent->p->async_shutdown_queued)
+ parent->p->shutdown_after = cookie;
+ put_device(parent);
+ }
+
+ idx = device_links_read_lock();
+ list_for_each_entry_rcu(link, &dev->links.suppliers, c_node,
+ device_links_read_lock_held()) {
/*
- * To prevent system hang, revert to sync shutdown in the event
- * that shutdown_after would make this shutdown wait for a
- * shutdown that hasn't been scheduled yet.
- *
- * This can happen if a parent or supplier is not ordered in the
- * devices_kset list before a child or consumer, which is not
- * expected.
+ * sync_state_only devlink consumers aren't dependent on
+ * suppliers
*/
- if (wait > cookie) {
- wait = cookie;
- dev_warn(dev, "Unsafe shutdown ordering, forcing sync order\n");
- }
+ if (!device_link_flag_is_sync_state_only(link->flags) &&
+ !link->supplier->p->async_shutdown_queued)
+ link->supplier->p->shutdown_after = cookie;
}
-
- async_synchronize_cookie_domain(wait, &sd_domain);
- shutdown_one_device(dev);
+ device_links_read_unlock(idx);
+ put_device(dev);
}
/**
@@ -4894,16 +4922,37 @@ static void shutdown_one_device_async(void *data, async_cookie_t cookie)
*/
void device_shutdown(void)
{
- struct device *dev, *parent;
- async_cookie_t cookie;
- struct device_link *link;
- int idx;
+ struct device *dev, *parent, *tmp;
+ LIST_HEAD(async_list);
+ bool wait_for_async;
wait_for_device_probe();
device_block_probing();
cpufreq_suspend();
+ /*
+ * Find devices which can shut down asynchronously, and move them from
+ * the devices list onto the async list in reverse order.
+ */
+ spin_lock(&devices_kset->list_lock);
+ list_for_each_entry_safe(dev, tmp, &devices_kset->list, kobj.entry) {
+ if (device_wants_async_shutdown(dev)) {
+ get_device(dev->parent);
+ get_device(dev);
+ list_move(&dev->kobj.entry, &async_list);
+ }
+ }
+ spin_unlock(&devices_kset->list_lock);
+
+ /*
+ * Dispatch asynchronous shutdowns first so they don't have to wait
+ * behind any synchronous shutdowns.
+ */
+ wait_for_async = !list_empty(&async_list);
+ list_for_each_entry_safe(dev, tmp, &async_list, kobj.entry)
+ queue_device_async_shutdown(dev);
+
spin_lock(&devices_kset->list_lock);
/*
* Walk the devices list backward, shutting down each in turn.
@@ -4928,37 +4977,24 @@ void device_shutdown(void)
list_del_init(&dev->kobj.entry);
spin_unlock(&devices_kset->list_lock);
- get_device(dev);
- get_device(parent);
-
- cookie = async_schedule_domain(shutdown_one_device_async,
- dev, &sd_domain);
/*
- * Ensure any parent & suppliers will wait for this device to
- * shut down
+ * Dispatch an async shutdown if this device has a child or
+ * consumer that is async. Otherwise, shut down synchronously.
*/
- if (parent) {
- parent->p->shutdown_after = cookie;
- put_device(parent);
- }
-
- idx = device_links_read_lock();
- list_for_each_entry_rcu(link, &dev->links.suppliers, c_node,
- device_links_read_lock_held()) {
- /*
- * sync_state_only devlink consumers aren't dependent on
- * suppliers
- */
- if (!device_link_flag_is_sync_state_only(link->flags))
- link->supplier->p->shutdown_after = cookie;
+ if (dev->p->shutdown_after) {
+ get_device(parent);
+ get_device(dev);
+ queue_device_async_shutdown(dev);
+ } else {
+ shutdown_one_device(dev);
}
- device_links_read_unlock(idx);
- put_device(dev);
spin_lock(&devices_kset->list_lock);
}
spin_unlock(&devices_kset->list_lock);
- async_synchronize_full_domain(&sd_domain);
+
+ if (wait_for_async)
+ async_synchronize_full_domain(&sd_domain);
}
/*
--
2.48.1
Powered by blists - more mailing lists