linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Saravana Kannan <saravanak@google.com>
To: David Jeffery <djeffery@redhat.com>
Cc: linux-kernel@vger.kernel.org, linux-nvme@lists.infradead.org,
	 linux-scsi@vger.kernel.org,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	 "Rafael J . Wysocki" <rafael@kernel.org>,
	Laurence Oberman <loberman@redhat.com>,
	 Android Kernel Team <kernel-team@android.com>
Subject: Re: [RFC PATCH 1/6] minimal async shutdown infrastructure
Date: Tue, 13 Feb 2024 19:42:50 -0800	[thread overview]
Message-ID: <CAGETcx91KahXY6ULXDpek3Xf6k4s646Y6+jn_LtfZPDDOpg7hA@mail.gmail.com> (raw)
In-Reply-To: <20240207184100.18066-2-djeffery@redhat.com>

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

  reply	other threads:[~2024-02-14  3:43 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-02-07 18:40 [RFC PATCH 0/6] async device shutdown support David Jeffery
2024-02-07 18:40 ` [RFC PATCH 1/6] minimal async shutdown infrastructure David Jeffery
2024-02-14  3:42   ` Saravana Kannan [this message]
2024-02-14 20:55     ` David Jeffery
2024-02-07 18:40 ` [RFC PATCH 2/6] Improve ability to perform async shutdown in parallel David Jeffery
2024-02-07 18:40 ` [RFC PATCH 3/6] pci bus async shutdown support David Jeffery
2024-02-07 18:40 ` [RFC PATCH 4/6] pci nvme " David Jeffery
2024-02-07 18:40 ` [RFC PATCH 5/6] scsi mid layer support for async command submit David Jeffery
2024-02-07 18:41 ` [RFC PATCH 6/6] sd: async cache flush on shutdown David Jeffery

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAGETcx91KahXY6ULXDpek3Xf6k4s646Y6+jn_LtfZPDDOpg7hA@mail.gmail.com \
    --to=saravanak@google.com \
    --cc=djeffery@redhat.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=kernel-team@android.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-nvme@lists.infradead.org \
    --cc=linux-scsi@vger.kernel.org \
    --cc=loberman@redhat.com \
    --cc=rafael@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).