Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Wed, 8 May 2019 01:02:20 +0900
From:   Suwan Kim <suwan.kim027@gmail.com>
To:     mathias.nyman@linux.intel.com
Cc:     linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org,
        gregkh@linuxfoundation.org
Subject: Re: [PATCH v2] usb: host: xhci: Support running urb giveback in
 tasklet context
Message-ID: <20190507160219.GB2427@localhost.localdomain>
References: <20190401141611.10087-1-suwan.kim027@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20190401141611.10087-1-suwan.kim027@gmail.com>
User-Agent: Mutt/1.11.3 (2019-02-01)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Mon, Apr 01, 2019 at 11:16:11PM +0900, Suwan Kim wrote:
> Patch "USB: HCD: support giveback of URB in tasklet context"[1]
> introduced giveback of urb in tasklet context. [1] This patch was
> applied to ehci but not xhci. [2] This patch significantly reduces
> the hard irq time of xhci. Especially for uvc driver, the hard irq
> including the uvc completion function runs quite long but applying
> this patch reduces the hard irq time of xhci.
> 
> I have tested four SS devices to check if performance degradation
> occurs when urb completion functions run in the tasklet context.
> 
> As a result of the test, all devices works well and shows very
> similar performance with the upstream kernel. Moreover, usb ethernet
> adapter show better performance than the upstream kernel about 5% for
> RX and 2% for TX. Four SS devices is as follows.
> 
> SS devices for test
> 
> 1. WD My Passport 2TB (external hard drive)
> 2. Sandisk Ultra Flair USB 3.0 32GB
> 3. Logitech Brio webcam
> 4. Iptime 1gigabit ethernet adapter (Mediatek RTL8153)
> 
> Test description
> 
> 1. Mass storage (hard drive) performance test
> - run below command 10 times and compute the average performance
> 
>     dd if=/dev/sdN iflag=direct of=/dev/null bs=1G count=1
> 
> 2. Mass storage (flash memory) performance test
> - run below command 10 times and compute the average performance
> 
>     dd if=/dev/sdN iflag=direct of=/dev/null bs=1G count=1
> 
> 3. Webcam streaming performance test
> - run simple capture program and get the average frame rate per second
> - capture 1500 frames
> - program link
> 
>     https://github.com/asfaca/Webcam-performance-analyzing-tool
> 
> - video resolution : 4096 X 2160 (4K) at 30 or 24 fps
> - device (Logitech Brio) spec url for the highest resolution and fps
> 
>     https://support.logitech.com/en_gb/product/brio-stream/specs
> 
> 4. USB Ethernet adapter performance test
> - directly connect two linux machines with ethernet cable
> - run pktgen of linux kernel and send 1500 bytes packets
> - run vnstat to measure the network bandwidth for 180 seconds
> 
> Test machine
> 
> - CPU : Intel i5-7600 @ 3.5GHz
> 
> Test results
> 
> 1. Mass storage (hard drive) performance test
> 
>             WD My Passport 2TB (external hard drive)
> --------------------------------------------------------------------
>     xhci without tasklet        |          xhci with tasklet
> --------------------------------------------------------------------
>          103.667MB/s            |            103.692MB/s
> --------------------------------------------------------------------
> 
> 2. Mass storage (flash memory) performance test
> 
>                Sandisk Ultra Flair USB 3.0 32GB
> --------------------------------------------------------------------
>     xhci without tasklet        |          xhci with tasklet
> --------------------------------------------------------------------
>          129.727MB/s            |            130.2MB/s
> --------------------------------------------------------------------
> 
> 3. Webcam streaming performance test
> 
>                      Logitech Brio webcam
> --------------------------------------------------------------------
>     xhci without tasklet        |          xhci with tasklet
> --------------------------------------------------------------------
>           26.4451 fps           |            26.3949 fps
> --------------------------------------------------------------------
> 
> 4. USB Ethernet adapter performance test
> 
>       Iptime 1gigabit ethernet adapter (Mediatek RTL8153)
> --------------------------------------------------------------------
>     xhci without tasklet        |          xhci with tasklet
> --------------------------------------------------------------------
> RX      933.86 Mbit/s           |            983.86 Mbit/s
> --------------------------------------------------------------------
> TX      830.18 Mbit/s           |            882.75 Mbit/s
> --------------------------------------------------------------------
> 
> [1], https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=94dfd7edfd5c9b605caf7b562de7a813d216e011
> [2], https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=428aac8a81058e2303677a8fbf26670229e51d3a
> 
> Signed-off-by: Suwan Kim <suwan.kim027@gmail.com>
> 
> ---
> v2 change:
>         - Add SS device test results and rewrite the description
> ---
>  drivers/usb/host/xhci-ring.c | 2 --
>  drivers/usb/host/xhci.c      | 2 +-
>  2 files changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> index 40fa25c4d041..0ede5265e6e2 100644
> --- a/drivers/usb/host/xhci-ring.c
> +++ b/drivers/usb/host/xhci-ring.c
> @@ -644,10 +644,8 @@ static void xhci_giveback_urb_in_irq(struct xhci_hcd *xhci,
>  	}
>  	xhci_urb_free_priv(urb_priv);
>  	usb_hcd_unlink_urb_from_ep(hcd, urb);
> -	spin_unlock(&xhci->lock);
>  	trace_xhci_urb_giveback(urb);
>  	usb_hcd_giveback_urb(hcd, urb, status);
> -	spin_lock(&xhci->lock);
>  }
>  
>  static void xhci_unmap_td_bounce_buffer(struct xhci_hcd *xhci,
> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> index 7fa58c99f126..bb212b4669cf 100644
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
> @@ -5141,7 +5141,7 @@ static const struct hc_driver xhci_hc_driver = {
>  	 * generic hardware linkage
>  	 */
>  	.irq =			xhci_irq,
> -	.flags =		HCD_MEMORY | HCD_USB3 | HCD_SHARED,
> +	.flags =		HCD_MEMORY | HCD_USB3 | HCD_SHARED | HCD_BH,
>  
>  	/*
>  	 * basic lifecycle operations
> -- 
> 2.20.1

Hi,

I sent this patch a month ago but got no answer.
Is there any feedback for this?
Please let me know if there are any faults or it needs more tests.

Regards

Suwan Kim