linux-kernel - Re: [PATCH] USB: cdc-acm: remove unneeded spin_lock

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <51B228A6.3010908@cogentembedded.com>
Date:	Fri, 07 Jun 2013 22:38:30 +0400
From:	Sergei Shtylyov <sergei.shtylyov@...entembedded.com>
To:	Greg Kroah-Hartman <gregkh@...uxfoundation.org>
CC:	Oliver Neukum <oliver@...kum.org>, linux-usb@...r.kernel.org,
	linux-kernel@...r.kernel.org, Paul Stoffregen <paul@...c.com>
Subject: Re: [PATCH] USB: cdc-acm: remove unneeded spin_lock_irqsave/restore
 on write path

Hello.

On 06/07/2013 10:32 PM, Greg Kroah-Hartman wrote:

> From: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
>
> 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@...c.com>
> Signed-off-by: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
>
> ---
>   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?

> +		return count;	/* A white lie */
> +	}
> +	usb_mark_last_busy(acm->dev);
> +
> +	stat = acm_start_wb(acm, wb);
>   	if (stat < 0)
>   		return stat;
>   	return count;
>

WBR, Sergei

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