Message-ID: <53B42B14.7010303@baslerweb.com>
Date:	Wed, 2 Jul 2014 17:53:56 +0200
From:	Stefan Klug <stefan.klug@...lerweb.com>
To:	<linux-usb@...r.kernel.org>
CC:	<linux-kernel@...r.kernel.org>, <stern@...land.harvard.edu>
Subject: [PATCH][RFC] USB: zerocopy support for usbfs

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@...lerweb.com>
---

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@....ee.ethz.ch)
+ *      Copyright (C) 2014  Stefan Klug (stefan.klug@...lerweb.com)
   *
   *      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@...lerweb.com

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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists