high cpu load on omap3 using musb

high cpu load on omap3 using musb

[Adam Wozniak]

[-- Attachment #1: Type: text/plain, Size: 1232 bytes --]

With a USB 2.0 webcam attached to the OTG port on an OMAP3 (applies to 
overo gumstix, beagleboard, probably others) we see a high CPU load in a 
kworker thread.

Between 2.6.33 and 2.6.34 musb_core.c changed.

IRQ handlers changed with the result that a worker in musb_core.c got 
scheduled far more frequently than needed.

I've included a patch below against 3.7, but i think it'll apply against 
mainline.
[I apologize for any whitespace mangling.  I've also attached the patch.]

I'd like more eyeballs to tell me if this is right.  I'd also like to 
know who I need to talk to to get this pushed into mainline.

--Adam

# The MUSB IRQ schedules work on every interrupt.
# This is unnecessary, and causes excessive CPU load.
#
# Here we only schedule work if there is something for
# the worker to do.

Index: git/drivers/usb/musb/musb_core.c
===================================================================
--- git.orig/drivers/usb/musb/musb_core.c
+++ git/drivers/usb/musb/musb_core.c
@@ -925,7 +925,9 @@ b_host:
         }
  #endif

-       schedule_work(&musb->irq_work);
+       if (musb->xceiv->state != musb->xceiv_old_state) {
+               schedule_work(&musb->irq_work);
+       }

         return handled;
  }


[-- Attachment #2: musb_smartwork.patch --]
[-- Type: text/x-patch, Size: 560 bytes --]

# The MUSB IRQ schedules work on every interrupt.
# This is unnecessary, and causes excessive CPU load.
#
# Here we only schedule work if there is something for
# the worker to do.

Index: git/drivers/usb/musb/musb_core.c
===================================================================
--- git.orig/drivers/usb/musb/musb_core.c
+++ git/drivers/usb/musb/musb_core.c
@@ -925,7 +925,9 @@ b_host:
 	}
 #endif
 
-	schedule_work(&musb->irq_work);
+	if (musb->xceiv->state != musb->xceiv_old_state) {
+		schedule_work(&musb->irq_work);
+	}
 
 	return handled;
 }

Re: high cpu load on omap3 using musb

[Laurent Pinchart]

Hi Adam,

On Wednesday 29 January 2014 08:44:57 Adam Wozniak wrote:
> With a USB 2.0 webcam attached to the OTG port on an OMAP3 (applies to
> overo gumstix, beagleboard, probably others) we see a high CPU load in a
> kworker thread.
> 
> Between 2.6.33 and 2.6.34 musb_core.c changed.
> 
> IRQ handlers changed with the result that a worker in musb_core.c got
> scheduled far more frequently than needed.
> 
> I've included a patch below against 3.7, but i think it'll apply against
> mainline.
> [I apologize for any whitespace mangling.  I've also attached the patch.]
> 
> I'd like more eyeballs to tell me if this is right.  I'd also like to
> know who I need to talk to to get this pushed into mainline.

Running the scripts/get_maintainer.pl script on your patch produces

Felipe Balbi <balbi@ti.com> (maintainer:MUSB MULTIPOINT H...)
Greg Kroah-Hartman <gregkh@linuxfoundation.org> (supporter:USB SUBSYSTEM)
linux-usb@vger.kernel.org (open list:MUSB MULTIPOINT H...)
linux-kernel@vger.kernel.org (open list)

Felipe Balbi (CC'ed) is the person who you should talk to.

While we're touching the subject of scripts, you should run the 
scripts/checkpatch.pl script and fix errors and warnings before submitting 
patches. Please see Documentation/SubmittingPatches.

Last (but not least) piece of advice, don't give up if you don't receive 
replies to your patches. People are busy and mails fall to cracks from time to 
time.

Felipe, apart from the coding style violation and the possibly missing 
locking, what's your opinion on this ? Does the patch make sense ?

> # The MUSB IRQ schedules work on every interrupt.
> # This is unnecessary, and causes excessive CPU load.
> #
> # Here we only schedule work if there is something for
> # the worker to do.
> 
> Index: git/drivers/usb/musb/musb_core.c
> ===================================================================
> --- git.orig/drivers/usb/musb/musb_core.c
> +++ git/drivers/usb/musb/musb_core.c
> @@ -925,7 +925,9 @@ b_host:
>          }
>   #endif
> 
> -       schedule_work(&musb->irq_work);
> +       if (musb->xceiv->state != musb->xceiv_old_state) {
> +               schedule_work(&musb->irq_work);
> +       }
> 
>          return handled;
>   }

-- 
Regards,

Laurent Pinchart

Re: high cpu load on omap3 using musb

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 2485 bytes --]

On Mon, Jul 21, 2014 at 05:28:58PM +0200, Laurent Pinchart wrote:
> Hi Adam,
> 
> On Wednesday 29 January 2014 08:44:57 Adam Wozniak wrote:
> > With a USB 2.0 webcam attached to the OTG port on an OMAP3 (applies to
> > overo gumstix, beagleboard, probably others) we see a high CPU load in a
> > kworker thread.
> > 
> > Between 2.6.33 and 2.6.34 musb_core.c changed.
> > 
> > IRQ handlers changed with the result that a worker in musb_core.c got
> > scheduled far more frequently than needed.
> > 
> > I've included a patch below against 3.7, but i think it'll apply against
> > mainline.
> > [I apologize for any whitespace mangling.  I've also attached the patch.]
> > 
> > I'd like more eyeballs to tell me if this is right.  I'd also like to
> > know who I need to talk to to get this pushed into mainline.
> 
> Running the scripts/get_maintainer.pl script on your patch produces
> 
> Felipe Balbi <balbi@ti.com> (maintainer:MUSB MULTIPOINT H...)
> Greg Kroah-Hartman <gregkh@linuxfoundation.org> (supporter:USB SUBSYSTEM)
> linux-usb@vger.kernel.org (open list:MUSB MULTIPOINT H...)
> linux-kernel@vger.kernel.org (open list)
> 
> Felipe Balbi (CC'ed) is the person who you should talk to.
> 
> While we're touching the subject of scripts, you should run the 
> scripts/checkpatch.pl script and fix errors and warnings before submitting 
> patches. Please see Documentation/SubmittingPatches.
> 
> Last (but not least) piece of advice, don't give up if you don't receive 
> replies to your patches. People are busy and mails fall to cracks from time to 
> time.
> 
> Felipe, apart from the coding style violation and the possibly missing 
> locking, what's your opinion on this ? Does the patch make sense ?

It's a duplication of the check which is already in musb_irq_work():

1742 static void musb_irq_work(struct work_struct *data)
1743 {
1744         struct musb *musb = container_of(data, struct musb, irq_work);
1745 
1746         if (musb->xceiv->state != musb->xceiv_old_state) {
1747                 musb->xceiv_old_state = musb->xceiv->state;
1748                 sysfs_notify(&musb->controller->kobj, NULL, "mode");
1749         }
1750 }

That does look better, but I'd need the check inside musb_irq_work() to
be removed and commit log would have to improve a bit.

ps: there's no missing locking, musb_stage0_irq() is called within
musb_interrupt() which is called within a locked IRQ handler.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: high cpu load on omap3 using musb

[Laurent Pinchart]

[-- Attachment #1: Type: text/plain, Size: 2812 bytes --]

Hi Felipe and Adam,

On Monday 21 July 2014 10:40:52 Felipe Balbi wrote:
> On Mon, Jul 21, 2014 at 05:28:58PM +0200, Laurent Pinchart wrote:
> > On Wednesday 29 January 2014 08:44:57 Adam Wozniak wrote:
> > > With a USB 2.0 webcam attached to the OTG port on an OMAP3 (applies to
> > > overo gumstix, beagleboard, probably others) we see a high CPU load in a
> > > kworker thread.
> > > 
> > > Between 2.6.33 and 2.6.34 musb_core.c changed.
> > > 
> > > IRQ handlers changed with the result that a worker in musb_core.c got
> > > scheduled far more frequently than needed.
> > > 
> > > I've included a patch below against 3.7, but i think it'll apply against
> > > mainline.
> > > [I apologize for any whitespace mangling.  I've also attached the
> > > patch.]
> > > 
> > > I'd like more eyeballs to tell me if this is right.  I'd also like to
> > > know who I need to talk to to get this pushed into mainline.
> > 
> > Running the scripts/get_maintainer.pl script on your patch produces
> > 
> > Felipe Balbi <balbi@ti.com> (maintainer:MUSB MULTIPOINT H...)
> > Greg Kroah-Hartman <gregkh@linuxfoundation.org> (supporter:USB SUBSYSTEM)
> > linux-usb@vger.kernel.org (open list:MUSB MULTIPOINT H...)
> > linux-kernel@vger.kernel.org (open list)
> > 
> > Felipe Balbi (CC'ed) is the person who you should talk to.
> > 
> > While we're touching the subject of scripts, you should run the
> > scripts/checkpatch.pl script and fix errors and warnings before submitting
> > patches. Please see Documentation/SubmittingPatches.
> > 
> > Last (but not least) piece of advice, don't give up if you don't receive
> > replies to your patches. People are busy and mails fall to cracks from
> > time to time.
> > 
> > Felipe, apart from the coding style violation and the possibly missing
> > locking, what's your opinion on this ? Does the patch make sense ?
> 
> It's a duplication of the check which is already in musb_irq_work():
> 
> 1742 static void musb_irq_work(struct work_struct *data)
> 1743 {
> 1744         struct musb *musb = container_of(data, struct musb, irq_work);
> 1745
> 1746         if (musb->xceiv->state != musb->xceiv_old_state) {
> 1747                 musb->xceiv_old_state = musb->xceiv->state;
> 1748                 sysfs_notify(&musb->controller->kobj, NULL, "mode");
> 1749         }
> 1750 }
> 
> That does look better, but I'd need the check inside musb_irq_work() to
> be removed and commit log would have to improve a bit.

OK. Adam, could you please modify the patch accordingly and resubmit it ?

> ps: there's no missing locking, musb_stage0_irq() is called within
> musb_interrupt() which is called within a locked IRQ handler.

I hadn't checked that, thank you for the confirmation.

-- 
Regards,

Laurent Pinchart

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 473 bytes --]