[PATCH] Moving spinlock to struct usb_hcd

[PATCH] Moving spinlock to struct usb_hcd

[Romit Dasgupta]

Hi,
   This is an attempt to move the hcd_urb_list_lock to struct usb_hcd.
The lock is taken on functions that try to add/delete/use urb against a
given hcd. I have not seen any association of an urb with multiple hcds.
Hence I thought this can be moved within usb_hcd. This should help
reduce contention to usb during high load where i/o is happening  to
multiple hcds. I am also trying to see if hcd_root_hub_lock can also be
moved to usb_hcd. Any comments on this?  I have done some testing with
this patch and it seems to be holding fine. If this looks ok I will
submit the lock statistics before and after the change.

Thanks,
-Romit


---
 drivers/usb/core/hcd.c |   24 +++++++++++-------------
 drivers/usb/core/hcd.h |    1 +
 2 files changed, 12 insertions(+), 13 deletions(-)

diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index d5ed3fa..6eb0f45 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -98,9 +98,6 @@ EXPORT_SYMBOL_GPL (usb_bus_list_lock);
 /* used for controlling access to virtual root hubs */
 static DEFINE_SPINLOCK(hcd_root_hub_lock);
 
-/* used when updating an endpoint's URB list */
-static DEFINE_SPINLOCK(hcd_urb_list_lock);
-
 /* wait queue for synchronous unlinks */
 DECLARE_WAIT_QUEUE_HEAD(usb_kill_urb_queue);
 
@@ -1000,7 +997,7 @@ int usb_hcd_link_urb_to_ep(struct usb_hcd *hcd,
struct urb *urb)
 {
 	int		rc = 0;
 
-	spin_lock(&hcd_urb_list_lock);
+	spin_lock(&hcd->hcd_urb_list_lock);
 
 	/* Check that the URB isn't being killed */
 	if (unlikely(urb->reject)) {
@@ -1033,7 +1030,7 @@ int usb_hcd_link_urb_to_ep(struct usb_hcd *hcd,
struct urb *urb)
 		goto done;
 	}
  done:
-	spin_unlock(&hcd_urb_list_lock);
+	spin_unlock(&hcd->hcd_urb_list_lock);
 	return rc;
 }
 EXPORT_SYMBOL_GPL(usb_hcd_link_urb_to_ep);
@@ -1106,9 +1103,9 @@ EXPORT_SYMBOL_GPL(usb_hcd_check_unlink_urb);
 void usb_hcd_unlink_urb_from_ep(struct usb_hcd *hcd, struct urb *urb)
 {
 	/* clear all state linking urb to this dev (and hcd) */
-	spin_lock(&hcd_urb_list_lock);
+	spin_lock(&hcd->hcd_urb_list_lock);
 	list_del_init(&urb->urb_list);
-	spin_unlock(&hcd_urb_list_lock);
+	spin_unlock(&hcd->hcd_urb_list_lock);
 }
 EXPORT_SYMBOL_GPL(usb_hcd_unlink_urb_from_ep);
 
@@ -1311,7 +1308,7 @@ void usb_hcd_flush_endpoint(struct usb_device
*udev,
 	hcd = bus_to_hcd(udev->bus);
 
 	/* No more submits can occur */
-	spin_lock_irq(&hcd_urb_list_lock);
+	spin_lock_irq(&hcd->hcd_urb_list_lock);
 rescan:
 	list_for_each_entry (urb, &ep->urb_list, urb_list) {
 		int	is_in;
@@ -1320,7 +1317,7 @@ rescan:
 			continue;
 		usb_get_urb (urb);
 		is_in = usb_urb_dir_in(urb);
-		spin_unlock(&hcd_urb_list_lock);
+		spin_unlock(&hcd->hcd_urb_list_lock);
 
 		/* kick hcd */
 		unlink1(hcd, urb, -ESHUTDOWN);
@@ -1345,14 +1342,14 @@ rescan:
 		usb_put_urb (urb);
 
 		/* list contents may have changed */
-		spin_lock(&hcd_urb_list_lock);
+		spin_lock(&hcd->hcd_urb_list_lock);
 		goto rescan;
 	}
-	spin_unlock_irq(&hcd_urb_list_lock);
+	spin_unlock_irq(&hcd->hcd_urb_list_lock);
 
 	/* Wait until the endpoint queue is completely empty */
 	while (!list_empty (&ep->urb_list)) {
-		spin_lock_irq(&hcd_urb_list_lock);
+		spin_lock_irq(&hcd->hcd_urb_list_lock);
 
 		/* The list may have changed while we acquired the spinlock */
 		urb = NULL;
@@ -1361,7 +1358,7 @@ rescan:
 					urb_list);
 			usb_get_urb (urb);
 		}
-		spin_unlock_irq(&hcd_urb_list_lock);
+		spin_unlock_irq(&hcd->hcd_urb_list_lock);
 
 		if (urb) {
 			usb_kill_urb (urb);
@@ -1618,6 +1615,7 @@ struct usb_hcd *usb_create_hcd (const struct
hc_driver *driver,
 		dev_dbg (dev, "hcd alloc failed\n");
 		return NULL;
 	}
+	spin_lock_init(&hcd->hcd_urb_list_lock);
 	dev_set_drvdata(dev, hcd);
 	kref_init(&hcd->kref);
 
diff --git a/drivers/usb/core/hcd.h b/drivers/usb/core/hcd.h
index 98e2419..e23ff45 100644
--- a/drivers/usb/core/hcd.h
+++ b/drivers/usb/core/hcd.h
@@ -128,6 +128,7 @@ struct usb_hcd {
 	 * input size of periodic table to an interrupt scheduler. 
 	 * (ohci 32, uhci 1024, ehci 256/512/1024).
 	 */
+	spinlock_t hcd_urb_list_lock;
 
 	/* The HC driver's private data is stored at the end of
 	 * this structure.
-- 
1.4.4.2

Re: [PATCH] Moving spinlock to struct usb_hcd

[David Brownell]

On Friday 25 January 2008, Romit Dasgupta wrote:
> This should help
> reduce contention to usb during high load where i/o is happening  to
> multiple hcds.

Looking at how this lock is used, contention doesn't look likely
to be an issue.  It's never held for long ...

> @@ -1106,9 +1103,9 @@ EXPORT_SYMBOL_GPL(usb_hcd_check_unlink_urb);
>  void usb_hcd_unlink_urb_from_ep(struct usb_hcd *hcd, struct urb *urb)
>  {
>         /* clear all state linking urb to this dev (and hcd) */
> -       spin_lock(&hcd_urb_list_lock);
> +       spin_lock(&hcd->hcd_urb_list_lock);
>         list_del_init(&urb->urb_list);
> -       spin_unlock(&hcd_urb_list_lock);
> +       spin_unlock(&hcd->hcd_urb_list_lock);
>  }
>  EXPORT_SYMBOL_GPL(usb_hcd_unlink_urb_from_ep);
>  

Do you have any proof that contention is an actual problem?
Because otherwise I see no benefit to such a change.

- Dave
 

Re: [PATCH] Moving spinlock to struct usb_hcd

[Romit Dasgupta]

>
>
> Looking at how this lock is used, contention doesn't look likely
> to be an issue.  It's never held for long ...
yes in the general case but in usb_hcd_flush_endpoint routine it seems
to be held for longer than other routines. I agree that
usb_hcd_flush_endpoint is an infrequently called routine. Normal
systems dont have too many HCs. My computer shows 1 EHCI and 3 OHCIs
so I guess when I connect high speed devices there are less chances of
contention. With more HC this lock might be contended for.
Nevertheless, the right place for the lock seems to be inside usb_hcd.
What do you think?

>
>
> Do you have any proof that contention is an actual problem?
> Because otherwise I see no benefit to such a change.
>
I will try to see what I can find with /proc/lock_stat.

Thanks,
-Romit

Re: [PATCH] Moving spinlock to struct usb_hcd

[Romit Dasgupta]

Hi,
                 Should I go ahead and submit the patch with the usual
"signed-off" thingie? Or is it totally useless patch that is going to
be ignored?

Thanks,
-Romit


On Jan 26, 2008 9:06 PM, Romit Dasgupta <romlinux@gmail.com> wrote:
> >
> >
> > Looking at how this lock is used, contention doesn't look likely
> > to be an issue.  It's never held for long ...
> yes in the general case but in usb_hcd_flush_endpoint routine it seems
> to be held for longer than other routines. I agree that
> usb_hcd_flush_endpoint is an infrequently called routine. Normal
> systems dont have too many HCs. My computer shows 1 EHCI and 3 OHCIs
> so I guess when I connect high speed devices there are less chances of
> contention. With more HC this lock might be contended for.
> Nevertheless, the right place for the lock seems to be inside usb_hcd.
> What do you think?
>
> >
> >
> > Do you have any proof that contention is an actual problem?
> > Because otherwise I see no benefit to such a change.
> >
>
> I will try to see what I can find with /proc/lock_stat.
>
> Thanks,
> -Romit
>

Re: [PATCH] Moving spinlock to struct usb_hcd

[Greg KH]

On Mon, Jan 28, 2008 at 09:50:36AM +0530, Romit Dasgupta wrote:
> Hi,
>                  Should I go ahead and submit the patch with the usual
> "signed-off" thingie? Or is it totally useless patch that is going to
> be ignored?

Let's see some proof that it actually helps, and then we can evaluate if
it is needed or not :)

thanks,

greg k-h