[PATCH][RFC] USB: zerocopy support for usbfs

From: Stefan Klug
Date: Wed Jul 02 2014 - 12:04:21 EST

Hi everybody,
in short: The attached patch adds zerocopy support to the usbfs driver.
I tested it on x86 and on a globalscale mirabox ARM board. Until now it works
quite nice and I'd love to get some comments and feedback on the patch.

Longer version: The usbfs driver does not support direct data transfers from the controller
to user-memory. For several use-cases (in my case USB3 Vision cameras pushing up to 360MB/s)
the copy operation from kernel to user-space results in a lot of unnecessary CPU overhead
especially on small embedded devices. I measured a decrease in CPU usage from 44% to 22% on
a the mirabox.

This patch implements zerocopy beside the existing code paths, so it is easy to disable
zerocopy at runtime and to directly compare the CPU usage.
To disable zerocopy just do an
echo 1 > /sys/module/usbcore/parameters/usbfs_disable_zerocopy
and to reenable it
echo 0 > /sys/module/usbcore/parameters/usbfs_disable_zerocopy

On modern desktop systems the change in CPU usage is quite low so this mostly affects
smaller embedded devices.

Implementation details:
The patch only touches drivers/usb/core/devio.c.
In procy_do_submiturb(), it is checked if zerocopy is allowed. This is currently a rough
check which compares the number of required pages to ps->dev->bus->sg_tablesize.
I don't know if there is more to check there.
Then the user memory provided inside the usbdevfs_urb structure is pinned to
physical memory using get_user_pages_fast().
All the user pages are added to the scatter-gather list and the logic continues as before.
In copy_urb_data_to_user() the pages are marked with set_page_dirty_lock() if the transfer
was a zerocopy one.
In free_async() the pinned pages are released, if the transfer was a zerocopy one.

Questions/Notes:
- I'm quite unhappy with the added member async::is_user_mem. Is there a better place
where I could store this information?
- I had a look at some other drivers using the get_user_pages -> sg_set_page -> page_cache_release
technique and didn't see any special code to handle corner cases.
Are there corner cases? - Like usb controllers not supporting the whole memory etc. ?
- In the kernel log I see messages like:
xhci_hcd 0000:00:14.0: WARN Event TRB for slot 4 ep 8 with no TDs queued?
after enabling zerocopy. Any idea where this might come from?

This patch should cleanly apply to the current master (3.16-rc2)

Kind regards
Stefan

Signed-off-by: Stefan Klug <stefan.klug@xxxxxxxxxxxxx>
---

diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
index 257876e..c97d1ec 100644
--- a/drivers/usb/core/devio.c
+++ b/drivers/usb/core/devio.c
@@ -4,6 +4,7 @@
* devio.c -- User space communication with USB devices.
*
* Copyright (C) 1999-2000 Thomas Sailer (sailer@xxxxxxxxxxxxxx)
+ * Copyright (C) 2014 Stefan Klug (stefan.klug@xxxxxxxxxxxxx)
*
* This program is free software; you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published by
@@ -30,6 +31,7 @@
* 04.01.2000 0.2 Turned into its own filesystem
* 30.09.2005 0.3 Fix user-triggerable oops in async URB delivery
* (CAN-2005-3055)
+ * 07.05.2014 0.4 Added zerocopy support
*/

/*****************************************************************************/
@@ -52,6 +54,7 @@
#include <linux/uaccess.h>
#include <asm/byteorder.h>
#include <linux/moduleparam.h>
+#include <linux/pagemap.h>

#include "usb.h"

@@ -94,6 +97,9 @@ struct async {
u32 secid;
u8 bulk_addr;
u8 bulk_status;
+
+ //set to 1, if the buffer is pinned user-memory
+ u8 is_user_mem;
};

static bool usbfs_snoop;
@@ -118,6 +124,12 @@ module_param(usbfs_memory_mb, uint, 0644);
MODULE_PARM_DESC(usbfs_memory_mb,
"maximum MB allowed for usbfs buffers (0 = no limit)");

+
+static bool usbfs_disable_zerocopy = false;
+module_param(usbfs_disable_zerocopy, bool, S_IRUGO | S_IWUSR);
+MODULE_PARM_DESC(usbfs_disable_zerocopy, "true to disable zerocopy and fallback to the old implementation");
+
+
/* Hard limit, necessary to avoid arithmetic overflow */
#define USBFS_XFER_MAX (UINT_MAX / 2 - 1000000)

@@ -289,14 +301,23 @@ static struct async *alloc_async(unsigned int numisoframes)
static void free_async(struct async *as)
{
int i;
+ struct page* p;

put_pid(as->pid);
if (as->cred)
put_cred(as->cred);
+
for (i = 0; i < as->urb->num_sgs; i++) {
- if (sg_page(&as->urb->sg[i]))
- kfree(sg_virt(&as->urb->sg[i]));
+ p = sg_page(&as->urb->sg[i]);
+ if (p) {
+ if(as->is_user_mem) {
+ page_cache_release(p);
+ } else {
+ kfree(sg_virt(&as->urb->sg[i]));
+ }
+ }
}
+
kfree(as->urb->sg);
kfree(as->urb->transfer_buffer);
kfree(as->urb->setup_packet);
@@ -419,9 +440,11 @@ static void snoop_urb_data(struct urb *urb, unsigned len)
}
}

-static int copy_urb_data_to_user(u8 __user *userbuffer, struct urb *urb)
+static int copy_urb_data_to_user(u8 __user *userbuffer, struct async *as)
{
unsigned i, len, size;
+ struct urb *urb = as->urb;
+ struct page* page;

if (urb->number_of_packets > 0) /* Isochronous */
len = urb->transfer_buffer_length;
@@ -434,12 +457,20 @@ static int copy_urb_data_to_user(u8 __user *userbuffer, struct urb *urb)
return 0;
}

- for (i = 0; i < urb->num_sgs && len; i++) {
- size = (len > USB_SG_SIZE) ? USB_SG_SIZE : len;
- if (copy_to_user(userbuffer, sg_virt(&urb->sg[i]), size))
- return -EFAULT;
- userbuffer += size;
- len -= size;
+ if(as->is_user_mem) {
+ for (i = 0; i < urb->num_sgs; i++) {
+ page = sg_page(&urb->sg[i]);
+ if (page)
+ set_page_dirty_lock(page);
+ }
+ } else {
+ for (i = 0; i < urb->num_sgs && len; i++) {
+ size = (len > USB_SG_SIZE) ? USB_SG_SIZE : len;
+ if (copy_to_user(userbuffer, sg_virt(&urb->sg[i]), size))
+ return -EFAULT;
+ userbuffer += size;
+ len -= size;
+ }
}

return 0;
@@ -1294,6 +1325,12 @@ static int proc_do_submiturb(struct usb_dev_state *ps, struct usbdevfs_urb *uurb
unsigned int stream_id = 0;
void *buf;

+ int num_pages = 0;
+ void *buf_aligned = 0; //the page aligned version of urb->buffer
+ int buf_offset = 0; // the offset nbetween urb->buffer and buf_alignment
+ int o;
+ struct page **pages = NULL;
+
if (uurb->flags & ~(USBDEVFS_URB_ISO_ASAP |
USBDEVFS_URB_SHORT_NOT_OK |
USBDEVFS_URB_BULK_CONTINUATION |
@@ -1369,9 +1406,21 @@ static int proc_do_submiturb(struct usb_dev_state *ps, struct usbdevfs_urb *uurb
uurb->type = USBDEVFS_URB_TYPE_INTERRUPT;
goto interrupt_urb;
}
- num_sgs = DIV_ROUND_UP(uurb->buffer_length, USB_SG_SIZE);
- if (num_sgs == 1 || num_sgs > ps->dev->bus->sg_tablesize)
- num_sgs = 0;
+
+
+ buf_aligned = (char*)((unsigned long)uurb->buffer & PAGE_MASK);
+ buf_offset = uurb->buffer - buf_aligned;
+ if(uurb->buffer_length > 0)
+ num_pages = DIV_ROUND_UP(uurb->buffer_length + buf_offset, PAGE_SIZE);
+
+ if(usbfs_disable_zerocopy || num_pages > ps->dev->bus->sg_tablesize) {
+ num_pages = 0;
+
+ //fallback to the non-zerocopy path
+ num_sgs = DIV_ROUND_UP(uurb->buffer_length, USB_SG_SIZE);
+ if (num_sgs == 1 || num_sgs > ps->dev->bus->sg_tablesize)
+ num_sgs = 0;
+ }
if (ep->streams)
stream_id = uurb->stream_id;
break;
@@ -1435,8 +1484,14 @@ static int proc_do_submiturb(struct usb_dev_state *ps, struct usbdevfs_urb *uurb
goto error;
}

- u += sizeof(struct async) + sizeof(struct urb) + uurb->buffer_length +
- num_sgs * sizeof(struct scatterlist);
+ u += sizeof(struct async) + sizeof(struct urb) +
+ num_sgs * sizeof(struct scatterlist) +
+ num_pages * sizeof(struct scatterlist);
+
+ //only add buffer_length if not doing zerocopy
+ if(!num_pages)
+ u += uurb->buffer_length;
+
ret = usbfs_increase_memory_usage(u);
if (ret)
goto error;
@@ -1471,6 +1526,57 @@ static int proc_do_submiturb(struct usb_dev_state *ps, struct usbdevfs_urb *uurb
}
totlen -= u;
}
+ } else if(num_pages) {
+ pages = kmalloc(num_pages*sizeof(struct page*), GFP_KERNEL);
+ if(!pages) {
+ ret = -ENOMEM;
+ goto error;
+ }
+
+ //create the scatterlist
+ as->urb->sg = kmalloc(num_pages * sizeof(struct scatterlist), GFP_KERNEL);
+ if (!as->urb->sg) {
+ ret = -ENOMEM;
+ goto error;
+ }
+
+ ret = get_user_pages_fast((unsigned long)buf_aligned,
+ num_pages,
+ is_in,
+ pages);
+
+ if(ret < 0) {
+ printk("get_user_pages failed %i\n", ret);
+ goto error;
+ }
+
+ //did we get all pages?
+ if(ret < num_pages) {
+ printk("get_user_pages didn't deliver all pages %i\n", ret);
+ //free the pages and error out
+ for(i=0; i<ret; i++) {
+ page_cache_release(pages[i]);
+ }
+ ret = -ENOMEM;
+ goto error;
+ }
+
+ as->is_user_mem = 1;
+ as->urb->num_sgs = num_pages;
+ sg_init_table(as->urb->sg, as->urb->num_sgs);
+
+ totlen = uurb->buffer_length + buf_offset;
+ o = buf_offset;
+ for (i = 0; i < as->urb->num_sgs; i++) {
+ u = (totlen > PAGE_SIZE) ? PAGE_SIZE : totlen;
+ u-= o;
+ sg_set_page(&as->urb->sg[i], pages[i], u, o);
+ totlen -= u + o;
+ o = 0;
+ }
+
+ kfree(pages);
+ pages = NULL;
} else if (uurb->buffer_length > 0) {
as->urb->transfer_buffer = kmalloc(uurb->buffer_length,
GFP_KERNEL);
@@ -1602,6 +1708,7 @@ static int proc_do_submiturb(struct usb_dev_state *ps, struct usbdevfs_urb *uurb
error:
kfree(isopkt);
kfree(dr);
+ kfree(pages);
if (as)
free_async(as);
return ret;
@@ -1650,7 +1757,7 @@ static int processcompl(struct async *as, void __user * __user *arg)
unsigned int i;

if (as->userbuffer && urb->actual_length) {
- if (copy_urb_data_to_user(as->userbuffer, urb))
+ if (copy_urb_data_to_user(as->userbuffer, as))
goto err_out;
}
if (put_user(as->status, &userurb->status))
@@ -1818,7 +1925,7 @@ static int processcompl_compat(struct async *as, void __user * __user *arg)
unsigned int i;

if (as->userbuffer && urb->actual_length) {
- if (copy_urb_data_to_user(as->userbuffer, urb))
+ if (copy_urb_data_to_user(as->userbuffer, as))
return -EFAULT;
}
if (put_user(as->status, &userurb->status))



---


Stefan Klug
Software Developer

Basler AG
An der Strusbek 60-62
22926 Ahrensburg
Germany

Tel. +49 4102 463 582
Fax +49 4102 463 46 582

Stefan.Klug@xxxxxxxxxxxxx

www.baslerweb.com

Vorstand: Dr.-Ing. Dietmar Ley (Vorsitzender)ÂÂÂJohn P. JenningsÂÂÂArndt Bake ÂÂHardy Mehl
Aufsichtsratsvorsitzender: Norbert Basler Basler AG Â Amtsgericht LÃbeck HRB 4090 Â Ust-IdNr.: DE 135 098 121 Â Steuer-Nr.: 30 292 04497 Â WEEE-Reg.-Nr. DE 83888045
--
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/