From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-pl1-f174.google.com (mail-pl1-f174.google.com [209.85.214.174]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 57BCB8BEC for ; Thu, 30 Jan 2025 03:06:44 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.214.174 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1738206406; cv=none; b=pfGHwZVU87thypeH4LZvfTgxkuHja0uQEItnRn+BSJ5kQL2n7dBTcGGvDczX3IrN28uP3RZEMLnV0OlY3F59We3IxV1RSMXnWn4ZBSrk4UVq+3K143sEWqu5ShxdHClTP8R4fnuBN3rk46NzNdH+8DtyONfpV0SvQA9im4PqxzA= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1738206406; c=relaxed/simple; bh=FnRDM0CkU75QdEYxTVdEglAPqUHuJ29tz4d5hzCPxdM=; h=From:To:Cc:Subject:Date:Message-ID:In-Reply-To:References: MIME-Version; b=PvpEzJUbhKLlTAIcv7Ls6xr8SjRAEB4bmhIxjtiM+5zI2IvVh1npGxXv9Nh6yMYU3CihvhVjWstYrSL9rRlaeiFpy8WtrRdLhFm4JVmYCxOb8+yC5b81rl7Vm/UAeoi5UECbQEB35BYxAAZiVPEDzPsphQI6w7VjFKZqw/RPh28= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=kerneltoast.com; spf=pass smtp.mailfrom=gmail.com; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b=C+Gt9sfK; arc=none smtp.client-ip=209.85.214.174 Authentication-Results: smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=kerneltoast.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=gmail.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="C+Gt9sfK" Received: by mail-pl1-f174.google.com with SMTP id d9443c01a7336-2161eb95317so4769645ad.1 for ; Wed, 29 Jan 2025 19:06:44 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1738206403; x=1738811203; darn=vger.kernel.org; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:sender:from:to:cc:subject:date :message-id:reply-to; bh=op1d1tXUm2tCCekAlnALKVuCzfVX7pfXlggV9vI9r+8=; b=C+Gt9sfK8izFBl5apuDIvZfv+w/VN/4mNZhW4fLcUk3x0Zee5l0hNhUK85amoz82Sz WZ1KkflYMOZCFaj2sdXvKFPpt4V/PlRbTij6O/SqNVdsuEPd2RY51KPP7z0+fajKO/dJ JsoXcV993v9ZEANirGnI1Y3fvdtyxxX1Kjzq167IJtmN/YM3vrn3dzVMDxk9J5RGyWR9 sE5BQP5aqf/B3V2vrW1ECJdDHML2oBK0XqkgqYJu4EdOr/WW1QyEiGXq0kK3hcYANUq8 RedOH5ls4sxTylMQJHOthVFAe4nQNIFqrQQ2r8Ztk8rlAQS52GL7AoR0zEzPET0mNud7 UMyQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1738206403; x=1738811203; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:sender:x-gm-message-state:from :to:cc:subject:date:message-id:reply-to; bh=op1d1tXUm2tCCekAlnALKVuCzfVX7pfXlggV9vI9r+8=; b=nhmtRCWegslCtwJ21FIdXoLZFHePMrho34IFoLkzdCN3bXtzKkvoyfRH75fuxLkRcE rMM1zsuc2MfO/vdEB1/EFlLiTNjmOV319qMkY1A5p4sKQ90XR8GSkDnICoIbMRtH0XyP 5UH+w0xGztg6opjExoc04/XFf2mQp/i/nAzr74W3x4M8MSveoYsQMfuOyX/ZRNa+LRH5 ikabuoLn58HzSbO93o8mIGtoh5dUvFz1tZsq/3eKJf39W04OlGtIXu5zrGcf8Y0aMT3m tQWwiD8/KBEk/03LDOXkJE7QKWjQHsWkpobl11IE8AnRSyJ4KSCtD0myhsowkXLFwiU2 WZ5w== X-Forwarded-Encrypted: i=1; AJvYcCX2ZuKbAhCCH4tj3E2cLLMWIgo9JEdoHE4eJ4u2kemvijdUi2At4HuSvekaWjk5mRfZEisjC9849n0p55w=@vger.kernel.org X-Gm-Message-State: AOJu0YydTfOSgyzJkzCHOieH7arDVqPgfdkxEaZ3a5uwIdAvwY3EM6EK 1bq1CUhIQGaKSMtnwCLmRF0E1aIVVa1HuD7wKHF8/Ty7P+1iKikg X-Gm-Gg: ASbGncsL7InmIB/B/DOlg3t0sNJDs6pRBoiXlvPyi+XDQ+sB9oIdhnzqRdl48Z35n02 bVnbKW2GwcBixZ6sI+EoidnSZf9XykNjyK46pOVw9o8JjRE5C0J53U98idQ0clgeUpWnTGqF1sr LOjm6DzYNpZzPI/0476YmfqMhzKMJ3wqF15/38DxlEg7yVFr6lcMY3Yedm5RHFybHpmOjJHKIyi Kz91ALd74U4bvaKtszPkPuZbciPKHq0zdxSyinhDoH/7n+lYNGrUxjGX7DAHqT9JPkIg4RqZFcd zglH X-Google-Smtp-Source: AGHT+IHI9hubfXKcJhgGPeXIbm6pWvHvCDpKuB+0Sr/ceItq/TzOuv8SgDTjhcsyAOu2v86IyVbgkA== X-Received: by 2002:a05:6a21:8dc8:b0:1e1:bd5b:b82a with SMTP id adf61e73a8af0-1ed7a61ced9mr10026282637.40.1738206403244; Wed, 29 Jan 2025 19:06:43 -0800 (PST) Received: from sultan-box.localdomain ([142.147.89.224]) by smtp.gmail.com with ESMTPSA id 41be03b00d2f7-acebe856890sm270387a12.42.2025.01.29.19.06.40 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 29 Jan 2025 19:06:42 -0800 (PST) Sender: Sultan Alsawaf From: Sultan Alsawaf X-Google-Original-From: Sultan Alsawaf To: mhklinux@outlook.com Cc: Martin.Belanger@dell.com, axboe@fb.com, djeffery@redhat.com, dwagner@suse.de, gregkh@linuxfoundation.org, hch@lst.de, jallison@ciq.com, jan.kiszka@seimens.com, kbusch@kernel.org, linux-kernel@vger.kernel.org, linux-nvme@lists.infradead.org, lukas@wunner.de, nathan@kernel.org, oohall@gmail.com, rafael@kernel.org, sagi@grimberg.me, spasswolf@web.de, stuart.w.hayes@gmail.com Subject: [PATCH] driver core: optimize async device shutdown Date: Wed, 29 Jan 2025 19:06:25 -0800 Message-ID: <20250130030625.4461-1-sultan@kerneltoast.com> X-Mailer: git-send-email 2.48.1 In-Reply-To: References: Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Transfer-Encoding: 8bit From: Sultan Alsawaf 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 --- 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