Re: [PATCH] USB: cdc-acm: remove unneeded spin_lock_irqsave/restoreon write path

From: Greg Kroah-Hartman
Date: Fri Jun 07 2013 - 14:41:07 EST


On Fri, Jun 07, 2013 at 10:38:30PM +0400, Sergei Shtylyov wrote:
> Hello.
>
> On 06/07/2013 10:32 PM, Greg Kroah-Hartman wrote:
>
> >From: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> >
> >When writing data we were:
> > lock
> > do some work
> > unlock
> > call function
> > lock
> > do some work
> > unlock
> > return
> > return
> >
> >It turns out, that "function" was only ever called in the one place, so
> >instead of locking/unlocking for no good reason, just inline the
> >function and only grab the lock once.
> >
> >This has sped up the pathological case of sending 1 byte packets to a
> >loop-back cdc-acm device from 49600 bytes per second to 50100 bytes a
> >second on my workstation. A tiny increase yes, but noticable, and now
> >the spinlock isn't the hottest thing on the perf graph anymore. Yes, we
> >are still waiting for the hardware for the most part, but getting rid of
> >a spin_lock_irqsave() call for every packet is still a good thing.
> >
> >And we end up deleting lines of code, always a win overall.
> >
> >This was found by using a Teensy 3.0 device and the test program and
> >firmware located at:
> > http://www.pjrc.com/teensy/benchmark_usb_serial_receive.html
> >
> >Reported-by: Paul Stoffregen <paul@xxxxxxxx>
> >Signed-off-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> >
> >---
> > drivers/usb/class/cdc-acm.c | 51 +++++++++++++++-----------------------------
> > 1 file changed, 18 insertions(+), 33 deletions(-)
> >
> >--- a/drivers/usb/class/cdc-acm.c
> >+++ b/drivers/usb/class/cdc-acm.c
> [...]
> >@@ -653,13 +621,30 @@ static int acm_tty_write(struct tty_stru
> > }
> > wb = &acm->wb[wbn];
> >+ if (!acm->dev) {
> >+ wb->use = 0;
> >+ spin_unlock_irqrestore(&acm->write_lock, flags);
> >+ return -ENODEV;
> >+ }
> >+
> > count = (count > acm->writesize) ? acm->writesize : count;
> > dev_vdbg(&acm->data->dev, "%s - write %d\n", __func__, count);
> > memcpy(wb->buf, buf, count);
> > wb->len = count;
> > spin_unlock_irqrestore(&acm->write_lock, flags);
> >- stat = acm_write_start(acm, wbn);
> >+ usb_autopm_get_interface_async(acm->control);
> >+ if (acm->susp_count) {
> >+ if (!acm->delayed_wb)
> >+ acm->delayed_wb = wb;
> >+ else
> >+ usb_autopm_put_interface_async(acm->control);
> >+ spin_unlock_irqrestore(&acm->write_lock, flags);
>
> But you've already dropped it above the *if* and not taken again?

Oops, it needs to move down a bit further, let me respin this...

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/