On Mon, 14 Aug 2006, Greg KH wrote:
> > I think the best approach would be to put the entire second half of
> > hub_port_connect_change (i.e., the big loop that runs SET_CONFIG_TRIES
> > times) into the sub-thread. Then usb_new_device could be left as is,
> > which will avoid problems with HCDs registering their root hubs.
> >
> > To avoid the other possible problems, khubd should arrange to wait until
> > all the sub-threads have ended before calling usb_unlock_device. That's
> > the main requirement: The parent hub must remain locked during the entire
> > time its children are being probed. This would mean that all the devices
> > attached to a hub could be probed in parallel, but separate hubs would
> > still be serialized.
> >
> > Sound good? I could work on a patch next week.
>
> Yes, that sounds like a good idea. A bit more complex then my simple
> hack, but it probably is the "correct" one :)
I decided that it made more sense to put all of hub_port_connect_change
into the sub-thread, because it contains the debouncing code, which can
most easily benefit from multithreading. Here is a patch, very lightly
tested. It did work, in the sense that two devices plugged into the same
hub were enumerated concurrently.
However I'm not sure that this will end up saving much time. Device
enumeration and probing usually go very quickly when everything's working.
In my simple test, the patch reduced the time needed for initializing the
two devices by about 400 ms. Clearly there was a 100 ms gain from doing
the debouncing in parallel; the rest is harder to identify. Probably a
lot of it is random variation. For example, the non-parallel test run
included a 280 ms delay for no apparent reason that wasn't present in the
parallel run. Maybe it was some other process getting a time slice?
On a UP system, parallelization saves mainly on I/O delays, and there
aren't too many of them in a typical USB enumeration sequence. There
did appear to be significant delays associated with uevent and udev
activity, but it's hard to tell how they fit in.
If things _don't_ work correctly then this patch definitely won't help
very much. khubd will be forced to remain parked on the hub until all the
sub-threads have completed, so a malfunctioning driver can still bring
everything to a halt.
In fact, that's pretty much unavoidable. The device is locked while the
driver is probed, and if the driver hangs that lock will never be
released. Then all sorts of other tasks like khubd and lsusb will
inevitably get hung up on the lock, and everything will collide in a big
pile-up.
A different approach would be to create a sub-thread upon entry to
drivers/base/dd.c:device_attach(). That is, allow device registration to
complete quickly but probe for drivers in a different thread. It won't
solve the problem of the unreleased lock, but it will allow more things to
run in parallel. I think you had considered something like that before
and decided it would cause problems. Do you remember what those problems
were?
Alan Stern
This is meant to apply on top of an early gregkh-all-2.6.18-rc4.patch, and
it undoes the multithreading you had already added. It will probably
need some adjustments to match your current tree.
In any case, this patch is intended for testing and review only. Do not
apply!
Index: usb-2.6/drivers/usb/core/Kconfig
===================================================================
--- usb-2.6.orig/drivers/usb/core/Kconfig
+++ usb-2.6/drivers/usb/core/Kconfig
@@ -72,6 +72,20 @@ config USB_SUSPEND
If you are unsure about this, say N here.
+config USB_MULTITHREAD_PROBE
+ bool "USB Multi-threaded probe (EXPERIMENTAL)"
+ depends on USB && EXPERIMENTAL
+ help
+ Say Y here if you want the USB core to spawn a new thread for
+ every USB device that is probed. This can cause a small speedup
+ in boot times on systems with a lot of different USB devices.
+
+ This option should be safe to enable, but if any odd probing
+ problems are found, please disable it, or dynamically turn it
+ off in the /sys/module/usbcore/parameters/multithread_probe
+ file
+
+ When in doubt, say N.
config USB_OTG
bool
Index: usb-2.6/drivers/usb/core/hub.c
===================================================================
--- usb-2.6.orig/drivers/usb/core/hub.c
+++ usb-2.6/drivers/usb/core/hub.c
@@ -45,6 +45,16 @@ static DECLARE_WAIT_QUEUE_HEAD(khubd_wai
static struct task_struct *khubd_task;
+/* multithreaded probe logic */
+#ifdef CONFIG_USB_MULTITHREAD_PROBE
+static int multithread_probe = 1;
+module_param(multithread_probe, bool, S_IRUGO | S_IWUSR);
+MODULE_PARM_DESC(multithread_probe,
+ "Run each USB device probe in a new thread");
+#else
+#define multithread_probe 0
+#endif
+
/* cycle leds on hubs that aren't blinking for attention */
static int blinkenlights = 0;
module_param (blinkenlights, bool, S_IRUGO);
@@ -1199,9 +1209,8 @@ static void show_string(struct usb_devic
*
* Only the hub driver or root-hub registrar should ever call this.
*/
-static int __usb_new_device(void *void_data)
+int usb_new_device(struct usb_device *udev)
{
- struct usb_device *udev = void_data;
int err;
err = usb_get_configuration(udev);
@@ -1312,18 +1321,6 @@ fail:
return err;
}
-int usb_new_device(struct usb_device *udev)
-{
- struct task_struct *probe_task;
- int ret = 0;
-
- probe_task = kthread_run(__usb_new_device, udev,
- "usb-probe-%s", udev->devnum);
- if (IS_ERR(probe_task))
- ret = PTR_ERR(probe_task);
- return ret;
-}
-
static int hub_port_status(struct usb_hub *hub, int port1,
u16 *status, u16 *change)
{
@@ -2495,6 +2492,57 @@ done:
hub_port_disable(hub, port1, 1);
}
+#ifdef CONFIG_USB_MULTITHREAD_PROBE
+struct connect_change_req {
+ struct usb_hub *hub;
+ int port1;
+ u16 portstatus, portchange;
+ struct completion *compl;
+ };
+
+static int __hub_port_connect_change(void *_req)
+{
+ struct connect_change_req *req = _req;
+ struct usb_hub *hub = req->hub;
+ struct completion *compl = req->compl;
+
+ hub_port_connect_change(hub, req->port1,
+ req->portstatus, req->portchange);
+ kfree(req);
+
+ /* Tell the parent that we are ending */
+ complete_and_exit(compl, 0);
+}
+
+static int hub_start_thread(struct usb_hub *hub, int port1,
+ u16 portstatus, u16 portchange, struct completion *compl)
+{
+ struct task_struct *enum_thread;
+ struct connect_change_req *req = kmalloc(sizeof(*req), GFP_KERNEL);
+
+ if (!req)
+ return -ENOMEM;
+
+ req->hub = hub;
+ req->port1 = port1;
+ req->portstatus = portstatus;
+ req->portchange = portchange;
+ req->compl = compl;
+
+ enum_thread = kthread_run(__hub_port_connect_change, req,
+ "usb-enum-%s.%d", hub->hdev->devpath, port1);
+ if (IS_ERR(enum_thread)) {
+ kfree(req);
+ return PTR_ERR(enum_thread);
+ }
+ return 0;
+}
+#else
+#define __hub_port_connect_change NULL
+#define hub_start_thread(hub, port1, portstatus, portchange, compl) \
+ -EINVAL
+#endif
+
static void hub_events(void)
{
struct list_head *tmp;
@@ -2508,6 +2556,8 @@ static void hub_events(void)
u16 portchange;
int i, ret;
int connect_change;
+ DECLARE_COMPLETION_ONSTACK(thread_compl);
+ int subthread_count;
/*
* We restart the list every time to avoid a deadlock with
@@ -2589,6 +2639,7 @@ static void hub_events(void)
}
/* deal with port status changes */
+ subthread_count = 0;
for (i = 1; i <= hub->descriptor->bNbrPorts; i++) {
if (test_bit(i, hub->busy_bits))
continue;
@@ -2674,9 +2725,17 @@ static void hub_events(void)
USB_PORT_FEAT_C_RESET);
}
- if (connect_change)
- hub_port_connect_change(hub, i,
+ if (connect_change) {
+ if (multithread_probe && hub_start_thread(
+ hub, i,
+ portstatus, portchange,
+ &thread_compl) == 0)
+ ++subthread_count;
+ else
+ hub_port_connect_change(hub, i,
portstatus, portchange);
+ }
+
} /* end for i */
/* deal with hub status changes */
@@ -2704,6 +2763,12 @@ static void hub_events(void)
hub->activating = 0;
+ /* Wait for all subthreads to complete before unlocking */
+ while (subthread_count > 0) {
+ wait_for_completion(&thread_compl);
+ --subthread_count;
+ }
+
/* If this is a root hub, tell the HCD it's okay to
* re-enable port-change interrupts now. */
if (!hdev->parent && !hub->busy_bits[0])
-------------------------------------------------------------------------
Using Tomcat but need to do more? Need to support web services, security?
Get stuff done quickly with pre-integrated technology to make your job easier
Download IBM WebSphere Application Server v.1.0.1 based on Apache Geronimo
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=120709&bid=263057&dat=121642
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
