On Thu, 7 Apr 2011 16:57:11 -0700
Greg KH <greg@xxxxxxxxx> wrote:
> 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) */
as a quick glance it seems like I can work with it - I'll give it
a test tomorrow am on our platform.
--
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
