Hi Andiry,
This patch looks fine, so I'll queue it to my for-usb-next branch.
However, I wonder if we could be even more clever about xhci_td
allocation. It seems like instead of using kmalloc all the time, the
better method might be to allocate memory pools of xhci_tds (or
lookaside caches) with different numbers of buffer pointers and just use
that. One pool for xhci_tds with only one buffer (for control, bulk,
and interrupt endpoints), one pool with, say, three buffer pointers,
another with 10 buffer pointers, and beyond that we'd use kmalloc to
allocate the xhci_td. We're allocating and deallocating them so often
that it might make sense to keep a pool of them around.
But I don't want to do any premature optimizations without using perf to
show that's where the bottle neck actually is. Speaking of that, have
you run any performance tests for this? It would be nice to know if it
actually reduces CPU load or time spent in the kernel during HS webcam
video transfer. Or at least doesn't negatively impact performance.
I'll still queue it, but I would like performance patches to come with
some sort of number attached. :)
Sarah Sharp
On Tue, Jul 26, 2011 at 08:34:11AM +0800, Andiry Xu wrote:
> In xhci_urb_enqueue(), allocate a block of memory for all the TDs instead
> of allocating memory for each of them separately. This reduces the number
> of kzalloc calling when an isochronous usb is submitted.
>
> Signed-off-by: Andiry Xu <andiry.xu@xxxxxxx>
> ---
> drivers/usb/host/xhci-mem.c | 14 +++-----------
> drivers/usb/host/xhci.c | 15 +++++++++------
> 2 files changed, 12 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
> index 1370db8..275d393 100644
> --- a/drivers/usb/host/xhci-mem.c
> +++ b/drivers/usb/host/xhci-mem.c
> @@ -1465,18 +1465,10 @@ struct xhci_command *xhci_alloc_command(struct xhci_hcd *xhci,
>
> void xhci_urb_free_priv(struct xhci_hcd *xhci, struct urb_priv *urb_priv)
> {
> - int last;
> -
> - if (!urb_priv)
> - return;
> -
> - last = urb_priv->length - 1;
> - if (last >= 0) {
> - int i;
> - for (i = 0; i <= last; i++)
> - kfree(urb_priv->td[i]);
> + if (urb_priv) {
> + kfree(urb_priv->td[0]);
> + kfree(urb_priv);
> }
> - kfree(urb_priv);
> }
>
> void xhci_free_command(struct xhci_hcd *xhci,
> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> index d0a6540..95c6d91 100644
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
> @@ -1029,6 +1029,7 @@ static int xhci_check_maxpacket(struct xhci_hcd *xhci, unsigned int slot_id,
> int xhci_urb_enqueue(struct usb_hcd *hcd, struct urb *urb, gfp_t mem_flags)
> {
> struct xhci_hcd *xhci = hcd_to_xhci(hcd);
> + struct xhci_td *buffer;
> unsigned long flags;
> int ret = 0;
> unsigned int slot_id, ep_index;
> @@ -1059,13 +1060,15 @@ int xhci_urb_enqueue(struct usb_hcd *hcd, struct urb *urb, gfp_t mem_flags)
> if (!urb_priv)
> return -ENOMEM;
>
> + buffer = kzalloc(size * sizeof(struct xhci_td), mem_flags);
> + if (!buffer) {
> + kfree(urb_priv);
> + return -ENOMEM;
> + }
> +
> for (i = 0; i < size; i++) {
> - urb_priv->td[i] = kzalloc(sizeof(struct xhci_td), mem_flags);
> - if (!urb_priv->td[i]) {
> - urb_priv->length = i;
> - xhci_urb_free_priv(xhci, urb_priv);
> - return -ENOMEM;
> - }
> + urb_priv->td[i] = buffer;
> + buffer++;
> }
>
> urb_priv->length = size;
> --
> 1.7.1
>
>
--
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
