[driver-core,v4,4/6] driver core: Probe devices asynchronously instead of the driver
diff mbox series

Message ID 20181015150926.29520.45280.stgit@localhost.localdomain
State Superseded
Headers show
Series
  • Add NUMA aware async_schedule calls
Related show

Commit Message

Alexander Duyck Oct. 15, 2018, 3:09 p.m. UTC
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.

I am using the driver_data member of the device struct to store the driver
pointer while we wait on the deferred probe call. This should be safe to do
as the value will either be set to NULL on a failed probe or driver load
followed by unload, or the driver value itself will be set on a successful
driver load.

Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>
---
 drivers/base/bus.c     |   23 +++--------------------
 drivers/base/dd.c      |   45 +++++++++++++++++++++++++++++++++++++++++++++
 include/linux/device.h |    4 +++-
 3 files changed, 51 insertions(+), 21 deletions(-)

Comments

Bart Van Assche Oct. 18, 2018, 6:11 p.m. UTC | #1
On Mon, 2018-10-15 at 08:09 -0700, Alexander Duyck wrote:
> +static void __driver_attach_async_helper(void *_dev, async_cookie_t cookie)
> +{
> +	struct device *dev = _dev;
> +
> +	__device_driver_lock(dev, dev->parent);
> +
> +	/*
> +	 * If someone attempted to bind a driver either successfully or
> +	 * unsuccessfully before we got here we should just skip the driver
> +	 * probe call.
> +	 */
> +	if (!dev->driver) {
> +		struct device_driver *drv = dev_get_drvdata(dev);
> +
> +		if (drv)
> +			driver_probe_device(drv, dev);
> +	}
> +
> +	__device_driver_unlock(dev, dev->parent);
> +
> +	put_device(dev);
> +
> +	dev_dbg(dev, "async probe completed\n");
> +}
> +
>  static int __driver_attach(struct device *dev, void *data)
>  {
>  	struct device_driver *drv = data;
> @@ -945,6 +971,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;
> +	}
> +
>  	device_driver_attach(drv, dev);

What prevents that the driver pointer becomes invalid after async_schedule() has
been called and before __driver_attach_async_helper() is called? I think we need
protection against concurrent driver_unregister() and __driver_attach_async_helper()
calls. I'm not sure whether that is possible without introducing a new mutex.

Thanks,

Bart.
Alexander Duyck Oct. 18, 2018, 7:38 p.m. UTC | #2
On 10/18/2018 11:11 AM, Bart Van Assche wrote:
> On Mon, 2018-10-15 at 08:09 -0700, Alexander Duyck wrote:
>> +static void __driver_attach_async_helper(void *_dev, async_cookie_t cookie)
>> +{
>> +	struct device *dev = _dev;
>> +
>> +	__device_driver_lock(dev, dev->parent);
>> +
>> +	/*
>> +	 * If someone attempted to bind a driver either successfully or
>> +	 * unsuccessfully before we got here we should just skip the driver
>> +	 * probe call.
>> +	 */

The answer to your question below is up here.

>> +	if (!dev->driver) {
>> +		struct device_driver *drv = dev_get_drvdata(dev);
>> +
>> +		if (drv)
>> +			driver_probe_device(drv, dev);
>> +	}
>> +
>> +	__device_driver_unlock(dev, dev->parent);
>> +
>> +	put_device(dev);
>> +
>> +	dev_dbg(dev, "async probe completed\n");
>> +}
>> +
>>   static int __driver_attach(struct device *dev, void *data)
>>   {
>>   	struct device_driver *drv = data;
>> @@ -945,6 +971,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;
>> +	}
>> +
>>   	device_driver_attach(drv, dev);
> 
> What prevents that the driver pointer becomes invalid after async_schedule() has
> been called and before __driver_attach_async_helper() is called? I think we need
> protection against concurrent driver_unregister() and __driver_attach_async_helper()
> calls. I'm not sure whether that is possible without introducing a new mutex.
> 
> Thanks,
> 
> Bart.

See the spot called out above.

Basically if somebody loads a driver the dev->driver becomes set. If a 
driver is removed it will clear dev->driver and set driver_data to 
0/NULL. That is what I am using as a mutex to track it in conjunction 
with the device mutex. Basically if somebody attempts to attach a driver 
before we get there we just exit and don't attempt to load this driver.
Bart Van Assche Oct. 18, 2018, 8:13 p.m. UTC | #3
On Thu, 2018-10-18 at 12:38 -0700, Alexander Duyck wrote:
> Basically if somebody loads a driver the dev->driver becomes set. If a 
> driver is removed it will clear dev->driver and set driver_data to 
> 0/NULL. That is what I am using as a mutex to track it in conjunction 
> with the device mutex. Basically if somebody attempts to attach a driver 
> before we get there we just exit and don't attempt to load this driver.

I don't think that the above matches your code. __device_attach() does not
set the dev->driver pointer before scheduling an asynchronous probe. Only
dev->driver_data gets set before the asynchonous probe is scheduled. Since
driver_detach() only iterates over devices that are in the per-driver klist
it will skip all devices for which an asynchronous probe has been scheduled
but __device_attach_async_helper() has not yet been called. My conclusion
remains that this patch does not prevent a driver pointer to become invalid
concurrently with __device_attach_async_helper() dereferencing the same
driver pointer.

Bart.
Alexander Duyck Oct. 19, 2018, 2:20 a.m. UTC | #4
On Thu, Oct 18, 2018 at 1:15 PM Bart Van Assche <bvanassche@acm.org> wrote:
>
> On Thu, 2018-10-18 at 12:38 -0700, Alexander Duyck wrote:
> > Basically if somebody loads a driver the dev->driver becomes set. If a
> > driver is removed it will clear dev->driver and set driver_data to
> > 0/NULL. That is what I am using as a mutex to track it in conjunction
> > with the device mutex. Basically if somebody attempts to attach a driver
> > before we get there we just exit and don't attempt to load this driver.
>
> I don't think that the above matches your code. __device_attach() does not
> set the dev->driver pointer before scheduling an asynchronous probe. Only
> dev->driver_data gets set before the asynchonous probe is scheduled. Since
> driver_detach() only iterates over devices that are in the per-driver klist
> it will skip all devices for which an asynchronous probe has been scheduled
> but __device_attach_async_helper() has not yet been called. My conclusion
> remains that this patch does not prevent a driver pointer to become invalid
> concurrently with __device_attach_async_helper() dereferencing the same
> driver pointer.
>
> Bart.

I see what you are talking about now. Actually I think this was an
existing issue before my patch even came into play. Basically the code
as it currently stands is device specific in terms of the attach and
release code.

I wonder if we shouldn't have the async_synchronize_full call in
__device_release_driver moved down and into driver_detach before we
even start the for loop. Assuming the driver is no longer associated
with the bus that should flush out all devices so that we can then
pull them out of the devices list at least. I may look at adding an
additional bitflag to the device struct to indicate that it has a
driver attach pending. Then for things like races between any attach
and detach calls the logic becomes pretty straight forward. Attach
will set the bit and provide driver data, detach will clear the bit
and the driver data. If a driver loads in between it should clear the
bit as well.

I'll work on it over the next couple days and hopefully have something
ready for testing/review early next week.

Thanks.

- Alex
Bart Van Assche Oct. 19, 2018, 2:31 a.m. UTC | #5
On 10/18/18 7:20 PM, Alexander Duyck wrote:
> I see what you are talking about now. Actually I think this was an
> existing issue before my patch even came into play. Basically the code
> as it currently stands is device specific in terms of the attach and
> release code.
> 
> I wonder if we shouldn't have the async_synchronize_full call in
> __device_release_driver moved down and into driver_detach before we
> even start the for loop. Assuming the driver is no longer associated
> with the bus that should flush out all devices so that we can then
> pull them out of the devices list at least. I may look at adding an
> additional bitflag to the device struct to indicate that it has a
> driver attach pending. Then for things like races between any attach
> and detach calls the logic becomes pretty straight forward. Attach
> will set the bit and provide driver data, detach will clear the bit
> and the driver data. If a driver loads in between it should clear the
> bit as well.
> 
> I'll work on it over the next couple days and hopefully have something
> ready for testing/review early next week.

Hi Alex,

How about checking in __driver_attach_async_helper() whether the driver 
pointer is still valid by checking whether bus_for_each_drv(dev->bus, 
...) can still find the driver pointer? That approach requires 
protection with a mutex to avoid races with the driver detach code but 
shouldn't require any new flags in struct device.

Thanks,

Bart.
Alexander Duyck Oct. 19, 2018, 10:35 p.m. UTC | #6
On Thu, 2018-10-18 at 19:31 -0700, Bart Van Assche wrote:
> On 10/18/18 7:20 PM, Alexander Duyck wrote:
> > I see what you are talking about now. Actually I think this was an
> > existing issue before my patch even came into play. Basically the
> > code
> > as it currently stands is device specific in terms of the attach
> > and
> > release code.
> > 
> > I wonder if we shouldn't have the async_synchronize_full call in
> > __device_release_driver moved down and into driver_detach before we
> > even start the for loop. Assuming the driver is no longer
> > associated
> > with the bus that should flush out all devices so that we can then
> > pull them out of the devices list at least. I may look at adding an
> > additional bitflag to the device struct to indicate that it has a
> > driver attach pending. Then for things like races between any
> > attach
> > and detach calls the logic becomes pretty straight forward. Attach
> > will set the bit and provide driver data, detach will clear the bit
> > and the driver data. If a driver loads in between it should clear
> > the
> > bit as well.
> > 
> > I'll work on it over the next couple days and hopefully have
> > something
> > ready for testing/review early next week.
> 
> Hi Alex,
> 
> How about checking in __driver_attach_async_helper() whether the
> driver 
> pointer is still valid by checking whether bus_for_each_drv(dev-
> >bus, 
> ...) can still find the driver pointer? That approach requires 
> protection with a mutex to avoid races with the driver detach code
> but 
> shouldn't require any new flags in struct device.
> 
> Thanks,
> 
> Bart.

That doesn't solve the problem I was pointing out though.

So the issue you are addressing by rechecking the bus should already be
handled by just calling async_synchronize_full in driver_detach. After
all we can't have a driver that is being added to the bus while it is
also being removed. So if we are detaching the driver calling
async_synchronize_full will flush out any deferred attach calls and
there will be no further calls since the driver has already been
removed from the bus.

The issue I was thinking of is how do we deal with races between
device_attach and device_release_driver. In that case we know the
device we want to remove a driver from, but we may not have information
about the driver. The easiest solution is to basically just disable the
pending enable. I could use the approach I am doing now and just NULL
out the driver_data if dev->driver is NULL. The only thing I am
thinking about is if just dev->driver being NULL is enough to signal
that we are using driver_data to carry a pointer to a pending driver,
or if we should add an extra bit to carry that meaning. It would be
pretty easy to just add a bit and then use that to prevent any false
reads of the deferred driver as driver data, or driver data as a
deferred driver as it would essentially act as a type bit.

Thanks.

- Alex

Patch
diff mbox series

diff --git a/drivers/base/bus.c b/drivers/base/bus.c
index 8a630f9bd880..0cd2eadd0816 100644
--- a/drivers/base/bus.c
+++ b/drivers/base/bus.c
@@ -606,17 +606,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.
@@ -649,15 +638,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 e845cd2a87af..c33f893ec9d8 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -802,6 +802,7 @@  static int __device_attach(struct device *dev, bool allow_async)
 			ret = 1;
 		else {
 			dev->driver = NULL;
+			dev_set_drvdata(dev, NULL);
 			ret = 0;
 		}
 	} else {
@@ -918,6 +919,31 @@  int device_driver_attach(struct device_driver *drv, struct device *dev)
 	return ret;
 }
 
+static void __driver_attach_async_helper(void *_dev, async_cookie_t cookie)
+{
+	struct device *dev = _dev;
+
+	__device_driver_lock(dev, dev->parent);
+
+	/*
+	 * If someone attempted to bind a driver either successfully or
+	 * unsuccessfully before we got here we should just skip the driver
+	 * probe call.
+	 */
+	if (!dev->driver) {
+		struct device_driver *drv = dev_get_drvdata(dev);
+
+		if (drv)
+			driver_probe_device(drv, dev);
+	}
+
+	__device_driver_unlock(dev, dev->parent);
+
+	put_device(dev);
+
+	dev_dbg(dev, "async probe completed\n");
+}
+
 static int __driver_attach(struct device *dev, void *data)
 {
 	struct device_driver *drv = data;
@@ -945,6 +971,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;
+	}
+
 	device_driver_attach(drv, dev);
 
 	return 0;
diff --git a/include/linux/device.h b/include/linux/device.h
index 90224e75ade4..b0abb04c29dc 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -906,7 +906,9 @@  struct dev_links_info {
  * 		variants, which GPIO pins act in what additional roles, and so
  * 		on.  This shrinks the "Board Support Packages" (BSPs) and
  * 		minimizes board-specific #ifdefs in drivers.
- * @driver_data: Private pointer for driver specific info.
+ * @driver_data: Private pointer for driver specific info if driver is
+ *		non-NULL. Pointer to deferred driver to be attached if driver
+ *		is NULL.
  * @links:	Links to suppliers and consumers of this device.
  * @power:	For device power management.
  *		See Documentation/driver-api/pm/devices.rst for details.