On Thu, Apr 07, 2011 at 03:34:59PM -0700, Greg KH wrote:
> On Thu, Apr 07, 2011 at 02:54:39PM -0700, Kristen Carlson Accardi wrote:
> > On Thu, 7 Apr 2011 13:35:02 -0700
> > Greg KH <greg@xxxxxxxxx> wrote:
> >
> > > On Thu, Apr 07, 2011 at 09:56:54AM -0700, Kristen Carlson Accardi wrote:
> > > > > Have you made timing/performance studies, comparing (for example) USB
> > > > > disk throughput with and without threaded interrupts?
> > > > >
> > > >
> > > > I could never do this with all the different types of hw. I suggest
> > > > that if you decide to move to a threaded irq you have a long period of
> > > > testing time where people can determine whether or not it impacts
> > > > performance on their own hw. Just from googling I can see that tglx
> > > > did some performance analysis, but you'd have to ask him what the
> > > > findings were.
> > >
> > > I asked him about this an hour ago, and he found that he got a
> > > performance increase in USB throughput switching to threaded irqs,
> > > especially for usb-storage devices.
> > >
> > > So I'm all for it now, send me the patch and let's see what breaks :)
> > >
> > > thanks,
> > >
> > > greg k-h
> >
> > I sent him an email to see if he'll submit the patches he's been
> > testing with -- I figured it'd be better to use those since he's
> > got time on them already.
>
> Yes, it turns out that your patch will not work properly on some
> hardware in the first place, so his changes are needed. I see him
> working on them right now in front of me in the conference meeting we
> are currently sitting in...
Here's Thomas's first cut at doing this, totally untested and written
during a legal track at the LF Collab summit.
Kristen, can you take this and test and see if it works for you?
thanks,
greg k-h
Subject: usb.patch
From: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
Date: Thu, 07 Apr 2011 23:35:16 +0200
Signed-off-by: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
---
drivers/usb/core/hcd.c | 42 ++++++++++++++++++++----------------------
drivers/usb/host/ehci-hcd.c | 33 +++++++++++++++++++++++++++------
drivers/usb/host/ehci-pci.c | 1 +
include/linux/usb/hcd.h | 1 +
4 files changed, 49 insertions(+), 28 deletions(-)
Index: linux-2.6/drivers/usb/core/hcd.c
===================================================================
--- linux-2.6.orig/drivers/usb/core/hcd.c
+++ linux-2.6/drivers/usb/core/hcd.c
@@ -2111,34 +2111,36 @@ EXPORT_SYMBOL_GPL(usb_bus_start_enum);
irqreturn_t usb_hcd_irq (int irq, void *__hcd)
{
struct usb_hcd *hcd = __hcd;
- unsigned long flags;
irqreturn_t rc;
- /* IRQF_DISABLED doesn't work correctly with shared IRQs
- * when the first handler doesn't use it. So let's just
- * assume it's never used.
- */
- local_irq_save(flags);
-
if (unlikely(HCD_DEAD(hcd) || !HCD_HW_ACCESSIBLE(hcd))) {
rc = IRQ_NONE;
- } else if (hcd->driver->irq(hcd) == IRQ_NONE) {
- rc = IRQ_NONE;
} else {
+ rc = hcd->driver->irq(hcd);
+ if (rc == IRQ_NONE)
+ return rc;
+
set_bit(HCD_FLAG_SAW_IRQ, &hcd->flags);
if (hcd->shared_hcd)
set_bit(HCD_FLAG_SAW_IRQ, &hcd->shared_hcd->flags);
if (unlikely(hcd->state == HC_STATE_HALT))
usb_hc_died(hcd);
- rc = IRQ_HANDLED;
}
-
- local_irq_restore(flags);
return rc;
}
EXPORT_SYMBOL_GPL(usb_hcd_irq);
+irqreturn_t usb_hcd_thread_irq (int irq, void *__hcd)
+{
+ struct usb_hcd *hcd = __hcd;
+
+ if (unlikely(HCD_DEAD(hcd) || !HCD_HW_ACCESSIBLE(hcd)))
+ return IRQ_NONE;
+
+ return hcd->driver->threaded_irq(hcd);
+}
+
/*-------------------------------------------------------------------------*/
/**
@@ -2189,7 +2191,7 @@ EXPORT_SYMBOL_GPL (usb_hc_died);
/**
* usb_create_shared_hcd - create and initialize an HCD structure
* @driver: HC driver that will use this hcd
- * @dev: device for this HC, stored in hcd->self.controller
+s * @dev: device for this HC, stored in hcd->self.controller
* @bus_name: value to store in hcd->self.bus_name
* @primary_hcd: a pointer to the usb_hcd structure that is sharing the
* PCI device. Only allocate certain resources for the primary HCD
@@ -2323,17 +2325,13 @@ static int usb_hcd_request_irqs(struct u
if (hcd->driver->irq) {
- /* IRQF_DISABLED doesn't work as advertised when used together
- * with IRQF_SHARED. As usb_hcd_irq() will always disable
- * interrupts we can remove it here.
- */
- if (irqflags & IRQF_SHARED)
- irqflags &= ~IRQF_DISABLED;
-
snprintf(hcd->irq_descr, sizeof(hcd->irq_descr), "%s:usb%d",
hcd->driver->description, hcd->self.busnum);
- retval = request_irq(irqnum, &usb_hcd_irq, irqflags,
- hcd->irq_descr, hcd);
+
+ retval = request_threaded_irq(irqnum,
+ hcd->driver->irq ? &usb_hcd_irq : NULL,
+ hcd->driver->threaded_irq ? &usb_hcd_thread_irq : NULL,
+ irqflags, hcd->irq_descr, hcd);
if (retval != 0) {
dev_err(hcd->self.controller,
"request interrupt %d failed\n",
Index: linux-2.6/drivers/usb/host/ehci-hcd.c
===================================================================
--- linux-2.6.orig/drivers/usb/host/ehci-hcd.c
+++ linux-2.6/drivers/usb/host/ehci-hcd.c
@@ -764,10 +764,35 @@ static int ehci_run (struct usb_hcd *hcd
static irqreturn_t ehci_irq (struct usb_hcd *hcd)
{
struct ehci_hcd *ehci = hcd_to_ehci (hcd);
+ u32 status;
+
+ spin_lock(&ehci->lock);
+
+ status = ehci_readl(ehci, &ehci->regs->status);
+ if (status & INTR_MASK)
+ ehci_writel(ehci, 0, &ehci->regs->intr_enable);
+
+ spin_unlock(&ehci->lock);
+
+ return status & INTR_MASK ? IRQ_WAKE_THREAD : IRQ_NONE;
+}
+
+static irqreturn_t ehci_thread_irq (struct usb_hcd *hcd)
+{
+ struct ehci_hcd *ehci = hcd_to_ehci (hcd);
u32 status, masked_status, pcd_status = 0, cmd;
int bh;
- spin_lock (&ehci->lock);
+ /*
+ * The scope of this lock wants to be reduced to protect the
+ * status register access. Serializiation at the thread level
+ * should simply use a mutex, which is not simple either as
+ * you have to deal with the watchdog timer stuff. Needs some
+ * thought. Making it spin_lock_bh() now should be fine. We
+ * just need to disable interrupts right before we write the
+ * interrupt mask register.
+ */
+ spin_lock_irq(&ehci->lock);
status = ehci_readl(ehci, &ehci->regs->status);
@@ -778,10 +803,6 @@ static irqreturn_t ehci_irq (struct usb_
}
masked_status = status & INTR_MASK;
- if (!masked_status) { /* irq sharing? */
- spin_unlock(&ehci->lock);
- return IRQ_NONE;
- }
/* clear (just) interrupts */
ehci_writel(ehci, masked_status, &ehci->regs->status);
@@ -881,7 +902,7 @@ dead:
if (bh)
ehci_work (ehci);
- spin_unlock (&ehci->lock);
+ spin_unlock_irq (&ehci->lock);
if (pcd_status)
usb_hcd_poll_rh_status(hcd);
return IRQ_HANDLED;
Index: linux-2.6/drivers/usb/host/ehci-pci.c
===================================================================
--- linux-2.6.orig/drivers/usb/host/ehci-pci.c
+++ linux-2.6/drivers/usb/host/ehci-pci.c
@@ -430,6 +430,7 @@ static const struct hc_driver ehci_pci_h
* generic hardware linkage
*/
.irq = ehci_irq,
+ .threaded_irq = ehci_thread_irq,
.flags = HCD_MEMORY | HCD_USB2,
/*
Index: linux-2.6/include/linux/usb/hcd.h
===================================================================
--- linux-2.6.orig/include/linux/usb/hcd.h
+++ linux-2.6/include/linux/usb/hcd.h
@@ -207,6 +207,7 @@ struct hc_driver {
/* irq handler */
irqreturn_t (*irq) (struct usb_hcd *hcd);
+ irqreturn_t (*threaded_irq) (struct usb_hcd *hcd);
int flags;
#define HCD_MEMORY 0x0001 /* HC regs use memory (else I/O) */
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
