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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux