From: "Rafael J. Wysocki" <rafael@kernel.org>
To: alexander.h.duyck@linux.intel.com
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
Len Brown <len.brown@intel.com>,
"Rafael J. Wysocki" <rafael@kernel.org>,
Linux PM <linux-pm@vger.kernel.org>,
Lai Jiangshan <jiangshanlai@gmail.com>,
Pavel Machek <pavel@ucw.cz>,
zwisler@kernel.org, Tejun Heo <tj@kernel.org>,
Andrew Morton <akpm@linux-foundation.org>
Subject: Re: [driver-core PATCH v3 3/5] driver core: Probe devices asynchronously instead of the driver
Date: Fri, 12 Oct 2018 09:52:12 +0200 [thread overview]
Message-ID: <CAJZ5v0gEssyHx-Ao+PfopHziBQ=WnU2cspSPpvEN=B_E-EVDPA@mail.gmail.com> (raw)
In-Reply-To: <20181011220933.1408.6882.stgit@localhost.localdomain>
On Fri, Oct 12, 2018 at 12:10 AM Alexander Duyck
<alexander.h.duyck@linux.intel.com> wrote:
>
> This change makes it so that we probe devices asynchronously instead of the
> driver. This results in us seeing the same behavior if the device is
> registered before the driver or after. This way we can avoid serializing
> the initialization should the driver not be loaded until after the devices
> have already been added.
>
> The motivation behind this is that if we have a set of devices that
> take a significant amount of time to load we can greatly reduce the time to
> load by processing them in parallel instead of one at a time. In addition,
> each device can exist on a different node so placing a single thread on one
> CPU to initialize all of the devices for a given driver can result in poor
> performance on a system with multiple nodes.
>
> One issue I can see with this patch is that I am using the
> dev_set/get_drvdata functions to store the driver in the device while I am
> waiting on the asynchronous init to complete. For now I am protecting it by
> using the lack of a dev->driver and the device lock.
>
> Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> ---
> drivers/base/bus.c | 23 +++--------------------
> drivers/base/dd.c | 42 ++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 45 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/base/bus.c b/drivers/base/bus.c
> index 8bfd27ec73d6..2a17bed657ec 100644
> --- a/drivers/base/bus.c
> +++ b/drivers/base/bus.c
> @@ -616,17 +616,6 @@ static ssize_t uevent_store(struct device_driver *drv, const char *buf,
> }
> static DRIVER_ATTR_WO(uevent);
>
> -static void driver_attach_async(void *_drv, async_cookie_t cookie)
> -{
> - struct device_driver *drv = _drv;
> - int ret;
> -
> - ret = driver_attach(drv);
> -
> - pr_debug("bus: '%s': driver %s async attach completed: %d\n",
> - drv->bus->name, drv->name, ret);
> -}
> -
> /**
> * bus_add_driver - Add a driver to the bus.
> * @drv: driver.
> @@ -659,15 +648,9 @@ int bus_add_driver(struct device_driver *drv)
>
> klist_add_tail(&priv->knode_bus, &bus->p->klist_drivers);
> if (drv->bus->p->drivers_autoprobe) {
> - if (driver_allows_async_probing(drv)) {
> - pr_debug("bus: '%s': probing driver %s asynchronously\n",
> - drv->bus->name, drv->name);
> - async_schedule(driver_attach_async, drv);
> - } else {
> - error = driver_attach(drv);
> - if (error)
> - goto out_unregister;
> - }
> + error = driver_attach(drv);
> + if (error)
> + goto out_unregister;
> }
> module_add_driver(drv->owner, drv);
>
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index 169412ee4ae8..5ba366c1cb83 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -864,6 +864,29 @@ void device_initial_probe(struct device *dev)
> __device_attach(dev, true);
> }
>
> +static void __driver_attach_async_helper(void *_dev, async_cookie_t cookie)
> +{
> + struct device *dev = _dev;
> +
> + if (dev->parent && dev->bus->need_parent_lock)
> + device_lock(dev->parent);
> + device_lock(dev);
> +
> + if (!dev->driver) {
> + struct device_driver *drv = dev_get_drvdata(dev);
> +
> + driver_probe_device(drv, dev);
> + }
> +
> + dev_dbg(dev, "async probe completed\n");
> +
> + device_unlock(dev);
> + if (dev->parent && dev->bus->need_parent_lock)
> + device_unlock(dev->parent);
The above duplicates some code from __driver_attach().
You could avoid that by adding one more function to call directly from
__driver_attach() in the "sync" case and from here, along the lines of
what the PM code does.
> +
> + put_device(dev);
> +}
> +
> static int __driver_attach(struct device *dev, void *data)
> {
> struct device_driver *drv = data;
> @@ -891,6 +914,25 @@ static int __driver_attach(struct device *dev, void *data)
> return ret;
> } /* ret > 0 means positive match */
>
> + if (driver_allows_async_probing(drv)) {
> + /*
> + * Instead of probing the device synchronously we will
> + * probe it asynchronously to allow for more parallelism.
> + *
> + * We only take the device lock here in order to guarantee
> + * that the dev->driver and driver_data fields are protected
> + */
> + dev_dbg(dev, "scheduling asynchronous probe\n");
> + device_lock(dev);
> + if (!dev->driver) {
> + get_device(dev);
> + dev_set_drvdata(dev, drv);
> + async_schedule(__driver_attach_async_helper, dev);
> + }
> + device_unlock(dev);
> + return 0;
> + }
> +
> if (dev->parent && dev->bus->need_parent_lock)
> device_lock(dev->parent);
> device_lock(dev);
>
next prev parent reply other threads:[~2018-10-12 7:52 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-10-11 22:09 [driver-core PATCH v3 0/5] Add NUMA aware async_schedule calls Alexander Duyck
2018-10-11 22:09 ` [driver-core PATCH v3 1/5] workqueue: Provide queue_work_node to queue work near a given NUMA node Alexander Duyck
2018-10-11 22:09 ` [driver-core PATCH v3 2/5] async: Add support for queueing on specific " Alexander Duyck
2018-10-11 22:09 ` [driver-core PATCH v3 3/5] driver core: Probe devices asynchronously instead of the driver Alexander Duyck
2018-10-12 7:52 ` Rafael J. Wysocki [this message]
2018-10-11 22:09 ` [driver-core PATCH v3 4/5] driver core: Attach devices on CPU local to device node Alexander Duyck
2018-10-11 22:09 ` [driver-core PATCH v3 5/5] PM core: Use new async_schedule_dev command Alexander Duyck
2018-10-12 7:53 ` Rafael J. Wysocki
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='CAJZ5v0gEssyHx-Ao+PfopHziBQ=WnU2cspSPpvEN=B_E-EVDPA@mail.gmail.com' \
--to=rafael@kernel.org \
--cc=akpm@linux-foundation.org \
--cc=alexander.h.duyck@linux.intel.com \
--cc=gregkh@linuxfoundation.org \
--cc=jiangshanlai@gmail.com \
--cc=len.brown@intel.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pm@vger.kernel.org \
--cc=pavel@ucw.cz \
--cc=tj@kernel.org \
--cc=zwisler@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).