Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1325972 > unrolled thread

Re: [PATCH v2] Add support for usbfs zerocopy.

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v2] Add support for usbfs zerocopy. Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2016-02-03 22:30 +0100
    [PATCH v4] Add support for usbfs zerocopy. "Steinar H. Gunderson" <sesse@google.com> - 2016-02-03 23:10 +0100
    Re: [PATCH v2] Add support for usbfs zerocopy. "Steinar H. Gunderson" <sesse@google.com> - 2016-02-03 23:10 +0100
      Re: [PATCH v2] Add support for usbfs zerocopy. Felipe Balbi <balbi@kernel.org> - 2016-02-03 23:20 +0100
        Re: [PATCH v2] Add support for usbfs zerocopy. "Steinar H. Gunderson" <sesse@google.com> - 2016-02-04 00:50 +0100
          Re: [PATCH v2] Add support for usbfs zerocopy. "Steinar H. Gunderson" <sesse@google.com> - 2016-02-04 11:30 +0100
          Re: [PATCH v2] Add support for usbfs zerocopy. Bjørn Mork <bjorn@mork.no> - 2016-02-04 11:30 +0100

#1325972 — Re: [PATCH v2] Add support for usbfs zerocopy.

On Mon, Jan 25, 2016 at 09:03:57AM +0100, Steinar H. Gunderson wrote:
> On Sun, Jan 24, 2016 at 01:12:08PM -0800, Greg Kroah-Hartman wrote:
> > Something is really wrong with your email client, it is saying this is
> > sent on Nov 26, the same exact time as your previous patch, yet you sent
> > this in January.  Which implies that this is an old patch and not an
> > updated one :(
> 
> It comes directly from git format-patch. I guess git commit --amend doesn't
> properly reset the date?

I guess not, odd.

> > Can you send me the latest verison of this, with the date set properly
> > on your machine for it?
> 
> I did git rebase --ignore-date HEAD^ just to reset the date. Sending it as an
> attachment just to be sure.

Attachments don't work, you know better than that :(

I need a _real_ email that I don't have to hand-edit to get it to apply.

thanks,

greg k-h

[toc] | [next] | [standalone]

#1326001 — [PATCH v4] Add support for usbfs zerocopy.

Add a new interface for userspace to preallocate memory that can be
used with usbfs. This gives two primary benefits:

 - Zerocopy; data no longer needs to be copied between the userspace
   and the kernel, but can instead be read directly by the driver from
   userspace's buffers. This works for all kinds of transfers (even if
   nonsensical for control and interrupt transfers); isochronous also
   no longer need to memset() the buffer to zero to avoid leaking kernel data.

 - Once the buffers are allocated, USB transfers can no longer fail due to
   memory fragmentation; previously, long-running programs could run into
   problems finding a large enough contiguous memory chunk, especially on
   embedded systems or at high rates.

Memory is allocated by using mmap() against the usbfs file descriptor,
and similarly deallocated by munmap(). Once memory has been allocated,
using it as pointers to a bulk or isochronous operation means you will
automatically get zerocopy behavior. Note that this also means you cannot
modify outgoing data until the transfer is complete. The same holds for
data on the same cache lines as incoming data; DMA modifying them at the
same time could lead to your changes being overwritten.

There's a new capability USBDEVFS_CAP_MMAP that userspace can query to see
if the running kernel supports this functionality, if just trying mmap() is
not acceptable.

Largely based on a patch by Markus Rechberger with some updates. The original
patch can be found at:

  http://sundtek.de/support/devio_mmap_v0.4.diff

Signed-off-by: Steinar H. Gunderson <sesse@google.com>
Signed-off-by: Markus Rechberger <mrechberger@gmail.com>
Acked-by: Alan Stern <stern@rowland.harvard.edu>
---
 drivers/usb/core/devio.c          | 227 +++++++++++++++++++++++++++++++++-----
 include/uapi/linux/usbdevice_fs.h |   1 +
 2 files changed, 203 insertions(+), 25 deletions(-)

diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
index 59e7a33..a9fc6ff 100644
--- a/drivers/usb/core/devio.c
+++ b/drivers/usb/core/devio.c
@@ -50,6 +50,7 @@
 #include <linux/user_namespace.h>
 #include <linux/scatterlist.h>
 #include <linux/uaccess.h>
+#include <linux/dma-mapping.h>
 #include <asm/byteorder.h>
 #include <linux/moduleparam.h>
 
@@ -69,6 +70,7 @@ struct usb_dev_state {
 	spinlock_t lock;            /* protects the async urb lists */
 	struct list_head async_pending;
 	struct list_head async_completed;
+	struct list_head memory_list;
 	wait_queue_head_t wait;     /* wake up if a request completed */
 	unsigned int discsignr;
 	struct pid *disc_pid;
@@ -79,6 +81,17 @@ struct usb_dev_state {
 	u32 disabled_bulk_eps;
 };
 
+struct usb_memory {
+	struct list_head memlist;
+	int vma_use_count;
+	int urb_use_count;
+	u32 size;
+	void *mem;
+	dma_addr_t dma_handle;
+	unsigned long vm_start;
+	struct usb_dev_state *ps;
+};
+
 struct async {
 	struct list_head asynclist;
 	struct usb_dev_state *ps;
@@ -89,6 +102,7 @@ struct async {
 	void __user *userbuffer;
 	void __user *userurb;
 	struct urb *urb;
+	struct usb_memory *usbm;
 	unsigned int mem_usage;
 	int status;
 	u32 secid;
@@ -162,6 +176,111 @@ static int connected(struct usb_dev_state *ps)
 			ps->dev->state != USB_STATE_NOTATTACHED);
 }
 
+static void dec_usb_memory_use_count(struct usb_memory *usbm, int *count)
+{
+	struct usb_dev_state *ps = usbm->ps;
+	unsigned long flags;
+
+	spin_lock_irqsave(&ps->lock, flags);
+	--*count;
+	if (usbm->urb_use_count == 0 && usbm->vma_use_count == 0) {
+		list_del(&usbm->memlist);
+		spin_unlock_irqrestore(&ps->lock, flags);
+
+		usb_free_coherent(ps->dev, usbm->size, usbm->mem,
+				usbm->dma_handle);
+		usbfs_decrease_memory_usage(
+			usbm->size + sizeof(struct usb_memory));
+		kfree(usbm);
+	} else {
+		spin_unlock_irqrestore(&ps->lock, flags);
+	}
+}
+
+static void usbdev_vm_open(struct vm_area_struct *vma)
+{
+	struct usb_memory *usbm = vma->vm_private_data;
+	unsigned long flags;
+
+	spin_lock_irqsave(&usbm->ps->lock, flags);
+	++usbm->vma_use_count;
+	spin_unlock_irqrestore(&usbm->ps->lock, flags);
+}
+
+static void usbdev_vm_close(struct vm_area_struct *vma)
+{
+	struct usb_memory *usbm = vma->vm_private_data;
+
+	dec_usb_memory_use_count(usbm, &usbm->vma_use_count);
+}
+
+struct vm_operations_struct usbdev_vm_ops = {
+	.open = usbdev_vm_open,
+	.close = usbdev_vm_close
+};
+
+static int usbdev_mmap(struct file *file, struct vm_area_struct *vma)
+{
+	struct usb_memory *usbm = NULL;
+	struct usb_dev_state *ps = file->private_data;
+	size_t size = vma->vm_end - vma->vm_start;
+	void *mem;
+	unsigned long flags;
+	dma_addr_t dma_handle;
+	int ret;
+
+	ret = usbfs_increase_memory_usage(size + sizeof(struct usb_memory));
+	if (ret)
+		goto error;
+
+	usbm = kzalloc(sizeof(struct usb_memory), GFP_KERNEL);
+	if (!usbm) {
+		ret = -ENOMEM;
+		goto error_decrease_mem;
+	}
+
+	mem = usb_alloc_coherent(ps->dev, size, GFP_USER, &dma_handle);
+	if (!mem) {
+		ret = -ENOMEM;
+		goto error_free_usbm;
+	}
+
+	memset(mem, 0, size);
+
+	usbm->mem = mem;
+	usbm->dma_handle = dma_handle;
+	usbm->size = size;
+	usbm->ps = ps;
+	usbm->vm_start = vma->vm_start;
+	usbm->vma_use_count = 1;
+	INIT_LIST_HEAD(&usbm->memlist);
+
+	if (remap_pfn_range(vma, vma->vm_start,
+			virt_to_phys(usbm->mem) >> PAGE_SHIFT,
+			size, vma->vm_page_prot) < 0) {
+		dec_usb_memory_use_count(usbm, &usbm->vma_use_count);
+		return -EAGAIN;
+	}
+
+	vma->vm_flags |= VM_IO;
+	vma->vm_flags |= (VM_DONTEXPAND | VM_DONTDUMP);
+	vma->vm_ops = &usbdev_vm_ops;
+	vma->vm_private_data = usbm;
+
+	spin_lock_irqsave(&ps->lock, flags);
+	list_add_tail(&usbm->memlist, &ps->memory_list);
+	spin_unlock_irqrestore(&ps->lock, flags);
+
+	return 0;
+
+error_free_usbm:
+	kfree(usbm);
+error_decrease_mem:
+	usbfs_decrease_memory_usage(size + sizeof(struct usb_memory));
+error:
+	return ret;
+}
+
 static ssize_t usbdev_read(struct file *file, char __user *buf, size_t nbytes,
 			   loff_t *ppos)
 {
@@ -278,8 +397,13 @@ static void free_async(struct async *as)
 		if (sg_page(&as->urb->sg[i]))
 			kfree(sg_virt(&as->urb->sg[i]));
 	}
+
 	kfree(as->urb->sg);
-	kfree(as->urb->transfer_buffer);
+	if (as->usbm == NULL)
+		kfree(as->urb->transfer_buffer);
+	else
+		dec_usb_memory_use_count(as->usbm, &as->usbm->urb_use_count);
+
 	kfree(as->urb->setup_packet);
 	usb_free_urb(as->urb);
 	usbfs_decrease_memory_usage(as->mem_usage);
@@ -893,6 +1017,7 @@ static int usbdev_open(struct inode *inode, struct file *file)
 	INIT_LIST_HEAD(&ps->list);
 	INIT_LIST_HEAD(&ps->async_pending);
 	INIT_LIST_HEAD(&ps->async_completed);
+	INIT_LIST_HEAD(&ps->memory_list);
 	init_waitqueue_head(&ps->wait);
 	ps->discsignr = 0;
 	ps->disc_pid = get_pid(task_pid(current));
@@ -945,6 +1070,7 @@ static int usbdev_release(struct inode *inode, struct file *file)
 		free_async(as);
 		as = async_getcompleted(ps);
 	}
+
 	kfree(ps);
 	return 0;
 }
@@ -1266,6 +1392,31 @@ static int proc_setconfig(struct usb_dev_state *ps, void __user *arg)
 	return status;
 }
 
+static struct usb_memory *
+find_memory_area(struct usb_dev_state *ps, const struct usbdevfs_urb *uurb)
+{
+	struct usb_memory *usbm = NULL, *iter;
+	unsigned long flags;
+	unsigned long uurb_start = (unsigned long)uurb->buffer;
+
+	spin_lock_irqsave(&ps->lock, flags);
+	list_for_each_entry(iter, &ps->memory_list, memlist) {
+		if (uurb_start >= iter->vm_start &&
+				uurb_start < iter->vm_start + iter->size) {
+			if (uurb->buffer_length > iter->vm_start + iter->size -
+					uurb_start) {
+				usbm = ERR_PTR(-EINVAL);
+			} else {
+				usbm = iter;
+				usbm->urb_use_count++;
+			}
+			break;
+		}
+	}
+	spin_unlock_irqrestore(&ps->lock, flags);
+	return usbm;
+}
+
 static int proc_do_submiturb(struct usb_dev_state *ps, struct usbdevfs_urb *uurb,
 			struct usbdevfs_iso_packet_desc __user *iso_frame_desc,
 			void __user *arg)
@@ -1422,6 +1573,19 @@ static int proc_do_submiturb(struct usb_dev_state *ps, struct usbdevfs_urb *uurb
 		goto error;
 	}
 
+	as->usbm = find_memory_area(ps, uurb);
+	if (IS_ERR(as->usbm)) {
+		ret = PTR_ERR(as->usbm);
+		as->usbm = NULL;
+		goto error;
+	}
+
+	/* do not use SG buffers when memory mapped segments
+	 * are in use
+	 */
+	if (as->usbm)
+		num_sgs = 0;
+
 	u += sizeof(struct async) + sizeof(struct urb) + uurb->buffer_length +
 	     num_sgs * sizeof(struct scatterlist);
 	ret = usbfs_increase_memory_usage(u);
@@ -1459,29 +1623,35 @@ static int proc_do_submiturb(struct usb_dev_state *ps, struct usbdevfs_urb *uurb
 			totlen -= u;
 		}
 	} else if (uurb->buffer_length > 0) {
-		as->urb->transfer_buffer = kmalloc(uurb->buffer_length,
-				GFP_KERNEL);
-		if (!as->urb->transfer_buffer) {
-			ret = -ENOMEM;
-			goto error;
-		}
+		if (as->usbm) {
+			unsigned long uurb_start = (unsigned long)uurb->buffer;
 
-		if (!is_in) {
-			if (copy_from_user(as->urb->transfer_buffer,
-					   uurb->buffer,
-					   uurb->buffer_length)) {
-				ret = -EFAULT;
+			as->urb->transfer_buffer = as->usbm->mem +
+					(uurb_start - as->usbm->vm_start);
+		} else {
+			as->urb->transfer_buffer = kmalloc(uurb->buffer_length,
+					GFP_KERNEL);
+			if (!as->urb->transfer_buffer) {
+				ret = -ENOMEM;
 				goto error;
 			}
-		} else if (uurb->type == USBDEVFS_URB_TYPE_ISO) {
-			/*
-			 * Isochronous input data may end up being
-			 * discontiguous if some of the packets are short.
-			 * Clear the buffer so that the gaps don't leak
-			 * kernel data to userspace.
-			 */
-			memset(as->urb->transfer_buffer, 0,
-					uurb->buffer_length);
+			if (!is_in) {
+				if (copy_from_user(as->urb->transfer_buffer,
+						   uurb->buffer,
+						   uurb->buffer_length)) {
+					ret = -EFAULT;
+					goto error;
+				}
+			} else if (uurb->type == USBDEVFS_URB_TYPE_ISO) {
+				/*
+				 * Isochronous input data may end up being
+				 * discontiguous if some of the packets are
+				 * short. Clear the buffer so that the gaps
+				 * don't leak kernel data to userspace.
+				 */
+				memset(as->urb->transfer_buffer, 0,
+						uurb->buffer_length);
+			}
 		}
 	}
 	as->urb->dev = ps->dev;
@@ -1528,10 +1698,14 @@ static int proc_do_submiturb(struct usb_dev_state *ps, struct usbdevfs_urb *uurb
 	isopkt = NULL;
 	as->ps = ps;
 	as->userurb = arg;
-	if (is_in && uurb->buffer_length > 0)
+	if (as->usbm) {
+		unsigned long uurb_start = (unsigned long)uurb->buffer;
+
+		as->urb->transfer_flags |= URB_NO_TRANSFER_DMA_MAP;
+		as->urb->transfer_dma = as->usbm->dma_handle +
+				(uurb_start - as->usbm->vm_start);
+	} else if (is_in && uurb->buffer_length > 0)
 		as->userbuffer = uurb->buffer;
-	else
-		as->userbuffer = NULL;
 	as->signr = uurb->signr;
 	as->ifnum = ifnum;
 	as->pid = get_pid(task_pid(current));
@@ -1587,6 +1761,8 @@ static int proc_do_submiturb(struct usb_dev_state *ps, struct usbdevfs_urb *uurb
 	return 0;
 
  error:
+	if (as && as->usbm)
+		dec_usb_memory_use_count(as->usbm, &as->usbm->urb_use_count);
 	kfree(isopkt);
 	kfree(dr);
 	if (as)
@@ -2040,7 +2216,7 @@ static int proc_get_capabilities(struct usb_dev_state *ps, void __user *arg)
 	__u32 caps;
 
 	caps = USBDEVFS_CAP_ZERO_PACKET | USBDEVFS_CAP_NO_PACKET_SIZE_LIM |
-			USBDEVFS_CAP_REAP_AFTER_DISCONNECT;
+			USBDEVFS_CAP_REAP_AFTER_DISCONNECT | USBDEVFS_CAP_MMAP;
 	if (!ps->dev->bus->no_stop_on_short)
 		caps |= USBDEVFS_CAP_BULK_CONTINUATION;
 	if (ps->dev->bus->sg_tablesize)
@@ -2366,6 +2542,7 @@ const struct file_operations usbdev_file_operations = {
 #ifdef CONFIG_COMPAT
 	.compat_ioctl =   usbdev_compat_ioctl,
 #endif
+	.mmap =           usbdev_mmap,
 	.open =		  usbdev_open,
 	.release =	  usbdev_release,
 };
diff --git a/include/uapi/linux/usbdevice_fs.h b/include/uapi/linux/usbdevice_fs.h
index 019ba1e..ecbd176 100644
--- a/include/uapi/linux/usbdevice_fs.h
+++ b/include/uapi/linux/usbdevice_fs.h
@@ -134,6 +134,7 @@ struct usbdevfs_hub_portinfo {
 #define USBDEVFS_CAP_NO_PACKET_SIZE_LIM		0x04
 #define USBDEVFS_CAP_BULK_SCATTER_GATHER	0x08
 #define USBDEVFS_CAP_REAP_AFTER_DISCONNECT	0x10
+#define USBDEVFS_CAP_MMAP			0x20
 
 /* USBDEVFS_DISCONNECT_CLAIM flags & struct */
 
-- 
2.1.4

[toc] | [prev] | [next] | [standalone]

#1326002

On Wed, Feb 03, 2016 at 01:23:17PM -0800, Greg Kroah-Hartman wrote:
> Attachments don't work, you know better than that :(

Since I've now been bitten by this several times: Is there any sort of best
practice for integrating git with MUAs? What I'm doing right now is
cut-and-paste from mutt to get the to/cc/in-reply-to headers right, and
that's suboptimal. It feels like it should be a FAQ, but I can't really find
anything obvious.

> I need a _real_ email that I don't have to hand-edit to get it to apply.

Trying again; sending v4 as a reply to your email. This is on top of git
master, since the patch no longer applied there. If you want it against any
other tree, please let me know which.

/* Steinar */
-- 
Software Engineer, Google Switzerland

[toc] | [prev] | [next] | [standalone]

#1326006

[Multipart message — attachments visible in raw view] — view raw

"Steinar H. Gunderson" <sesse@google.com> writes:

> On Wed, Feb 03, 2016 at 01:23:17PM -0800, Greg Kroah-Hartman wrote:
>> Attachments don't work, you know better than that :(
>
> Since I've now been bitten by this several times: Is there any sort of best
> practice for integrating git with MUAs? What I'm doing right now is
> cut-and-paste from mutt to get the to/cc/in-reply-to headers right, and
> that's suboptimal. It feels like it should be a FAQ, but I can't really find
> anything obvious.

have you tried git send-email ? If you wanna send as a reply to another
message, use:

  $ git send-email --to foo@bar.com --cc bar@foo.com --in-reply-to=abcd-message-id@foo.bar.com

note that git send-email can read mutt aliases. It's all documented in
the man page:

  $ git help send-email

-- 
balbi

[toc] | [prev] | [next] | [standalone]

#1326252

On Thu, Feb 04, 2016 at 12:15:50AM +0200, Felipe Balbi wrote:
>> Since I've now been bitten by this several times: Is there any sort of best
>> practice for integrating git with MUAs? What I'm doing right now is
>> cut-and-paste from mutt to get the to/cc/in-reply-to headers right, and
>> that's suboptimal. It feels like it should be a FAQ, but I can't really find
>> anything obvious.
> have you tried git send-email ? If you wanna send as a reply to another
> message, use:
> 
>   $ git send-email --to foo@bar.com --cc bar@foo.com --in-reply-to=abcd-message-id@foo.bar.com

Unfortunately that doesn't change anything; I still need to cut-and-paste the
to, cc and in-reply-to manually. What I'd want is something like
“git send-email as a reply to the last email in my outbox”.

Attaching the patch gives me exactly this, by the way, but seemingly ends up
with a format that's more cumbersome to receive (which is a bad tradeoff).

/* Steinar */
-- 
Software Engineer, Google Switzerland

[toc] | [prev] | [next] | [standalone]

#1326595

On Thu, Feb 04, 2016 at 11:17:26AM +0100, Bjørn Mork wrote:
> Then use Mutt to reply, but include the patch inline instead of
> attaching it.  I believe this is discussed in the Mutt section of
> Documentation/email-clients.txt

Thanks; if that works (even though it changes the “From” line and such)
that's a good solution. It doesn't work for series of multiple patches, but I
can certainly live with that.

/* Steinar */
-- 
Software Engineer, Google Switzerland

[toc] | [prev] | [next] | [standalone]

#1326596

"Steinar H. Gunderson" <sesse@google.com> writes:
> On Thu, Feb 04, 2016 at 12:15:50AM +0200, Felipe Balbi wrote:
>>> Since I've now been bitten by this several times: Is there any sort of best
>>> practice for integrating git with MUAs? What I'm doing right now is
>>> cut-and-paste from mutt to get the to/cc/in-reply-to headers right, and
>>> that's suboptimal. It feels like it should be a FAQ, but I can't really find
>>> anything obvious.
>> have you tried git send-email ? If you wanna send as a reply to another
>> message, use:
>> 
>>   $ git send-email --to foo@bar.com --cc bar@foo.com --in-reply-to=abcd-message-id@foo.bar.com
>
> Unfortunately that doesn't change anything; I still need to cut-and-paste the
> to, cc and in-reply-to manually. What I'd want is something like
> “git send-email as a reply to the last email in my outbox”.
>
> Attaching the patch gives me exactly this, by the way, but seemingly ends up
> with a format that's more cumbersome to receive (which is a bad tradeoff).

Then use Mutt to reply, but include the patch inline instead of
attaching it.  I believe this is discussed in the Mutt section of
Documentation/email-clients.txt



Bjørn

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web