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

Groups > linux.kernel > #1301673 > unrolled thread

Does vm_operations_struct require a .owner field?

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

  Does vm_operations_struct require a .owner field? Alan Stern <stern@rowland.harvard.edu> - 2016-01-05 17:30 +0100
    Re: Does vm_operations_struct require a .owner field? "Kirill A. Shutemov" <kirill@shutemov.name> - 2016-01-05 22:00 +0100
      Re: Does vm_operations_struct require a .owner field? Alan Stern <stern@rowland.harvard.edu> - 2016-01-05 22:40 +0100
        Re: Does vm_operations_struct require a .owner field? "Steinar H. Gunderson" <sesse@google.com> - 2016-01-06 01:00 +0100
        [PATCH] Add support for usbfs zerocopy. "Steinar H. Gunderson" <sesse@google.com> - 2016-01-06 01:00 +0100
          Re: [PATCH] Add support for usbfs zerocopy. Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2016-01-06 01:20 +0100
            Re: [PATCH] Add support for usbfs zerocopy. "Steinar H. Gunderson" <sesse@google.com> - 2016-01-06 01:30 +0100
            [PATCH v2] Add support for usbfs zerocopy. "Steinar H. Gunderson" <sesse@google.com> - 2016-01-06 01:30 +0100
              Re: [PATCH v2] Add support for usbfs zerocopy. Lingzhu Xiang <lingzhu.xiang@mail.utoronto.ca> - 2016-01-07 03:40 +0100
                Re: [PATCH v2] Add support for usbfs zerocopy. Alan Stern <stern@rowland.harvard.edu> - 2016-01-07 16:50 +0100
                  Re: [PATCH v2] Add support for usbfs zerocopy. Oliver Neukum <oneukum@suse.com> - 2016-01-11 12:20 +0100
                    Re: [PATCH v2] Add support for usbfs zerocopy. Oliver Neukum <oneukum@suse.com> - 2016-01-12 11:10 +0100
              Re: [PATCH v2] Add support for usbfs zerocopy. Lingzhu Xiang <lingzhu.xiang@mail.utoronto.ca> - 2016-01-09 05:20 +0100
                Re: [PATCH v2] Add support for usbfs zerocopy. "Steinar H. Gunderson" <sesse@google.com> - 2016-01-09 12:00 +0100
          Re: [PATCH] Add support for usbfs zerocopy. Christoph Hellwig <hch@infradead.org> - 2016-01-06 07:50 +0100
            Re: [PATCH] Add support for usbfs zerocopy. "Steinar H. Gunderson" <sesse@google.com> - 2016-01-06 15:50 +0100
              Re: [PATCH] Add support for usbfs zerocopy. Peter Stuge <peter@stuge.se> - 2016-01-06 16:30 +0100
                Re: [PATCH] Add support for usbfs zerocopy. "Steinar H. Gunderson" <sesse@google.com> - 2016-01-06 16:40 +0100
                Re: [PATCH] Add support for usbfs zerocopy. Alan Stern <stern@rowland.harvard.edu> - 2016-01-06 16:40 +0100
              Re: [PATCH] Add support for usbfs zerocopy. Alan Stern <stern@rowland.harvard.edu> - 2016-01-06 16:40 +0100
                Re: [PATCH] Add support for usbfs zerocopy. Christoph Hellwig <hch@infradead.org> - 2016-01-08 10:50 +0100
                  RE: [PATCH] Add support for usbfs zerocopy. David Laight <David.Laight@ACULAB.COM> - 2016-01-08 11:30 +0100
                  Re: [PATCH] Add support for usbfs zerocopy. Oliver Neukum <oneukum@suse.com> - 2016-01-08 17:10 +0100
                  Re: [PATCH] Add support for usbfs zerocopy. "Steinar H. Gunderson" <sesse@google.com> - 2016-01-12 22:30 +0100
                    Re: [PATCH] Add support for usbfs zerocopy. Alan Stern <stern@rowland.harvard.edu> - 2016-01-12 23:10 +0100
                      Re: [PATCH] Add support for usbfs zerocopy. Lingzhu Xiang <lingzhu.xiang@mail.utoronto.ca> - 2016-01-13 08:10 +0100
                        Re: [PATCH] Add support for usbfs zerocopy. Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2016-01-13 18:30 +0100
              Re: [PATCH] Add support for usbfs zerocopy. Christoph Hellwig <hch@infradead.org> - 2016-01-08 10:50 +0100

Page 1 of 2  [1] 2  Next page →

#1301673 — Does vm_operations_struct require a .owner field?

Hello:

Question: The vm_operations_struct structure contains lots of callback
pointers.  Is there any mechanism to prevent the callback routines and
the structure itself being unloaded from memory (if they are built into
modules) while the relevant VMAs are still in use?

Consider a simple example: A user program calls mmap(2) on a device
file.  Later on, the file is closed and the device driver's module is
unloaded.  But until munmap(2) is called or the user program exits, the
memory mapping and the corresponding VMA will remain in existence.  
(The man page for munmap specifically says "closing the file descriptor
does not unmap the region".)  Thus when the user program does do an
munmap(), the callback to vma->vm_ops->close will reference nonexistent
memory and cause an oops.

Normally this sort of thing is prevented by try_module_get(...->owner).  
But vm_operations_struct doesn't include a .owner field.

Am I missing something here?

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1302165

On Tue, Jan 05, 2016 at 11:27:45AM -0500, Alan Stern wrote:
> Hello:
> 
> Question: The vm_operations_struct structure contains lots of callback
> pointers.  Is there any mechanism to prevent the callback routines and
> the structure itself being unloaded from memory (if they are built into
> modules) while the relevant VMAs are still in use?
> 
> Consider a simple example: A user program calls mmap(2) on a device
> file.  Later on, the file is closed and the device driver's module is
> unloaded.  But until munmap(2) is called or the user program exits, the
> memory mapping and the corresponding VMA will remain in existence.  
> (The man page for munmap specifically says "closing the file descriptor
> does not unmap the region".)  Thus when the user program does do an
> munmap(), the callback to vma->vm_ops->close will reference nonexistent
> memory and cause an oops.
> 
> Normally this sort of thing is prevented by try_module_get(...->owner).  
> But vm_operations_struct doesn't include a .owner field.
> 
> Am I missing something here?

mmap(2) takes reference of the file, therefore the file is not closed from
kernel POV until vma is gone and you cannot unload relevant module.
See get_file() in mmap_region().

-- 
 Kirill A. Shutemov
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1302229

On Tue, 5 Jan 2016, Kirill A. Shutemov wrote:

> On Tue, Jan 05, 2016 at 11:27:45AM -0500, Alan Stern wrote:
> > Hello:
> > 
> > Question: The vm_operations_struct structure contains lots of callback
> > pointers.  Is there any mechanism to prevent the callback routines and
> > the structure itself being unloaded from memory (if they are built into
> > modules) while the relevant VMAs are still in use?
> > 
> > Consider a simple example: A user program calls mmap(2) on a device
> > file.  Later on, the file is closed and the device driver's module is
> > unloaded.  But until munmap(2) is called or the user program exits, the
> > memory mapping and the corresponding VMA will remain in existence.  
> > (The man page for munmap specifically says "closing the file descriptor
> > does not unmap the region".)  Thus when the user program does do an
> > munmap(), the callback to vma->vm_ops->close will reference nonexistent
> > memory and cause an oops.
> > 
> > Normally this sort of thing is prevented by try_module_get(...->owner).  
> > But vm_operations_struct doesn't include a .owner field.
> > 
> > Am I missing something here?
> 
> mmap(2) takes reference of the file, therefore the file is not closed from
> kernel POV until vma is gone and you cannot unload relevant module.
> See get_file() in mmap_region().

Thank you.  So it looks like I was worried about nothing.

Steinar, you can remove the try_module_get/module_put lines from your
patch.  Also, the list_del() and comment in usbdev_release() aren't 
needed -- at that point we know the memory_list has to be empty since 
there can't be any outstanding URBs or VMA references.  If you take 
those things out then the patch should be ready for merging.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1302278

On Tue, Jan 05, 2016 at 04:31:09PM -0500, Alan Stern wrote:
> Thank you.  So it looks like I was worried about nothing.
> 
> Steinar, you can remove the try_module_get/module_put lines from your
> patch.  Also, the list_del() and comment in usbdev_release() aren't 
> needed -- at that point we know the memory_list has to be empty since 
> there can't be any outstanding URBs or VMA references.  If you take 
> those things out then the patch should be ready for merging.

Good, thanks. Did so, compiled, testing it still works, sending :-)

/* Steinar */
-- 
Software Engineer, Google Switzerland
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1302281 — [PATCH] 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 38ae877c..0238c78 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;
@@ -157,6 +171,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 loff_t usbdev_lseek(struct file *file, loff_t offset, int orig)
 {
 	loff_t ret;
@@ -297,8 +416,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);
@@ -910,6 +1034,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));
@@ -962,6 +1087,7 @@ static int usbdev_release(struct inode *inode, struct file *file)
 		free_async(as);
 		as = async_getcompleted(ps);
 	}
+
 	kfree(ps);
 	return 0;
 }
@@ -1283,6 +1409,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)
@@ -1439,6 +1590,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);
@@ -1476,29 +1640,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;
@@ -1545,10 +1715,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));
@@ -1604,6 +1778,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)
@@ -2047,7 +2223,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)
@@ -2373,6 +2549,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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

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

On Thu, Nov 26, 2015 at 01:19:13AM +0100, Steinar H. Gunderson wrote:
> Add a new interface for userspace to preallocate memory that can be
> used with usbfs. This gives two primary benefits:

Please 'version' your patches, so that I have a chance to figure out
what patch is what, and what changed between patches.

otherwise the odds of me picking the "wrong" one is _very_ high...

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

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

On Tue, Jan 05, 2016 at 04:11:43PM -0800, Greg Kroah-Hartman wrote:
>> Add a new interface for userspace to preallocate memory that can be
>> used with usbfs. This gives two primary benefits:
> Please 'version' your patches, so that I have a chance to figure out
> what patch is what, and what changed between patches.
> 
> otherwise the odds of me picking the "wrong" one is _very_ high...

OK. I won't make any attempt at reconstructing the history, but I'll resend
the one I just sent you as v2, ie. --reroll-count=2.

Somehow it feels like there should be a way to integrate this better into my
MUA, but hopefully this is soon all done anyway. :-)

/* Steinar */
-- 
Software Engineer, Google Switzerland
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1302294 — [PATCH v2] 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 38ae877c..0238c78 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;
@@ -157,6 +171,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 loff_t usbdev_lseek(struct file *file, loff_t offset, int orig)
 {
 	loff_t ret;
@@ -297,8 +416,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);
@@ -910,6 +1034,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));
@@ -962,6 +1087,7 @@ static int usbdev_release(struct inode *inode, struct file *file)
 		free_async(as);
 		as = async_getcompleted(ps);
 	}
+
 	kfree(ps);
 	return 0;
 }
@@ -1283,6 +1409,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)
@@ -1439,6 +1590,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);
@@ -1476,29 +1640,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;
@@ -1545,10 +1715,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));
@@ -1604,6 +1778,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)
@@ -2047,7 +2223,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)
@@ -2373,6 +2549,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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

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

On Wed, Nov 25, 2015 at 7:19 PM, Steinar H. Gunderson <sesse@google.com> wrote:
> Add a new interface for userspace to preallocate memory that can be
> used with usbfs. This gives two primary benefits:

I got this when trying to allocate a little bit large buffer (~4MB)
using the new userspace libusb_dev_mem_alloc():

> [ 1706.212407] usb 2-1.1: reset SuperSpeed USB device number 3 using xhci_hcd
> [ 1706.234823] xhci_hcd 0000:00:14.0: swiotlb buffer is full (sz: 4325376 bytes)
> [ 1706.234827] swiotlb: coherent allocation failed for device 0000:00:14.0 size=4325376
> [ 1706.234830] CPU: 1 PID: 3233 Comm: Protonect Tainted: G     U  W       4.4.0-rc8-amd64 #1 Debian 4.4~rc8-1~exp1
> [ 1706.234831] Hardware name: LENOVO 20ALCTO1WW/20ALCTO1WW, BIOS GIET76WW (2.26 ) 08/27/2014
> [ 1706.234833]  0000000000000000 000000000f50c266 ffffffff812e6019 ffffffffffffffff
> [ 1706.234836]  ffffffff8130dc45 ffff88020000000b 0000000000420000 ffffffff81a2a0e0
> [ 1706.234838]  ffff880206263d80 0000000000000000 ffff88021c892f40 0000000000420040
> [ 1706.234841] Call Trace:
> [ 1706.234847]  [<ffffffff812e6019>] ? dump_stack+0x40/0x57
> [ 1706.234851]  [<ffffffff8130dc45>] ? swiotlb_alloc_coherent+0x135/0x150
> [ 1706.234867]  [<ffffffffa021deb1>] ? hcd_buffer_alloc+0xb1/0x130 [usbcore]
> [ 1706.234875]  [<ffffffffa0221ab5>] ? usbdev_mmap+0xa5/0x1b0 [usbcore]
> [ 1706.234880]  [<ffffffff813bbc25>] ? tty_insert_flip_string_fixed_flag+0x85/0xe0
> [ 1706.234885]  [<ffffffff8119af87>] ? mmap_region+0x3e7/0x660
> [ 1706.234888]  [<ffffffff8119b536>] ? do_mmap+0x336/0x420
> [ 1706.234892]  [<ffffffff8118213f>] ? vm_mmap_pgoff+0xaf/0xf0
> [ 1706.234895]  [<ffffffff811999dd>] ? SyS_mmap_pgoff+0x1ad/0x270
> [ 1706.234898]  [<ffffffff811d53b6>] ? SyS_write+0x76/0xc0
> [ 1706.234903]  [<ffffffff815829f2>] ? system_call_fast_compare_end+0xc/0x67

I understand there are some requirements on the allocation such that
large blocks are not always available. But what is the proper way to
determine the upper limit of the size such that the user can avoid
generating warnings like this? (Also, the application really wants to
be able to allocate large buffers, maybe tune swiotlb=?.)

Test results:

Basic testing with smaller buffer size shows the patch works well with
Kinect v2 (~260MB/s isochronous). Tests were performed on top of
4.4-rc8 with Debian config.

I'm still trying to verify whether this patch fixes page allocation
failure, but is having some trouble reproducing memory fragmentation.
I will later test it on the original machine which had the problem.

Regards,
Lingzhu
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

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

On Wed, 6 Jan 2016, Lingzhu Xiang wrote:

> On Wed, Nov 25, 2015 at 7:19 PM, Steinar H. Gunderson <sesse@google.com> wrote:
> > Add a new interface for userspace to preallocate memory that can be
> > used with usbfs. This gives two primary benefits:
> 
> I got this when trying to allocate a little bit large buffer (~4MB)
> using the new userspace libusb_dev_mem_alloc():
> 
> > [ 1706.212407] usb 2-1.1: reset SuperSpeed USB device number 3 using xhci_hcd
> > [ 1706.234823] xhci_hcd 0000:00:14.0: swiotlb buffer is full (sz: 4325376 bytes)
> > [ 1706.234827] swiotlb: coherent allocation failed for device 0000:00:14.0 size=4325376
> > [ 1706.234830] CPU: 1 PID: 3233 Comm: Protonect Tainted: G     U  W       4.4.0-rc8-amd64 #1 Debian 4.4~rc8-1~exp1
> > [ 1706.234831] Hardware name: LENOVO 20ALCTO1WW/20ALCTO1WW, BIOS GIET76WW (2.26 ) 08/27/2014
> > [ 1706.234833]  0000000000000000 000000000f50c266 ffffffff812e6019 ffffffffffffffff
> > [ 1706.234836]  ffffffff8130dc45 ffff88020000000b 0000000000420000 ffffffff81a2a0e0
> > [ 1706.234838]  ffff880206263d80 0000000000000000 ffff88021c892f40 0000000000420040
> > [ 1706.234841] Call Trace:
> > [ 1706.234847]  [<ffffffff812e6019>] ? dump_stack+0x40/0x57
> > [ 1706.234851]  [<ffffffff8130dc45>] ? swiotlb_alloc_coherent+0x135/0x150
> > [ 1706.234867]  [<ffffffffa021deb1>] ? hcd_buffer_alloc+0xb1/0x130 [usbcore]
> > [ 1706.234875]  [<ffffffffa0221ab5>] ? usbdev_mmap+0xa5/0x1b0 [usbcore]
> > [ 1706.234880]  [<ffffffff813bbc25>] ? tty_insert_flip_string_fixed_flag+0x85/0xe0
> > [ 1706.234885]  [<ffffffff8119af87>] ? mmap_region+0x3e7/0x660
> > [ 1706.234888]  [<ffffffff8119b536>] ? do_mmap+0x336/0x420
> > [ 1706.234892]  [<ffffffff8118213f>] ? vm_mmap_pgoff+0xaf/0xf0
> > [ 1706.234895]  [<ffffffff811999dd>] ? SyS_mmap_pgoff+0x1ad/0x270
> > [ 1706.234898]  [<ffffffff811d53b6>] ? SyS_write+0x76/0xc0
> > [ 1706.234903]  [<ffffffff815829f2>] ? system_call_fast_compare_end+0xc/0x67
> 
> I understand there are some requirements on the allocation such that
> large blocks are not always available. But what is the proper way to
> determine the upper limit of the size such that the user can avoid
> generating warnings like this? (Also, the application really wants to
> be able to allocate large buffers, maybe tune swiotlb=?.)

It's debatable whether this should have generated a warning.  Why 
doesn't dma_alloc_coherent() simply fail silently?

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

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

On Thu, 2016-01-07 at 10:40 -0500, Alan Stern wrote:
> > I understand there are some requirements on the allocation such that
> > large blocks are not always available. But what is the proper way to
> > determine the upper limit of the size such that the user can avoid
> > generating warnings like this? (Also, the application really wants
> to
> > be able to allocate large buffers, maybe tune swiotlb=?.)
> 
> It's debatable whether this should have generated a warning.  Why 
> doesn't dma_alloc_coherent() simply fail silently?

I suspect many drivers to be unable to deal well with a failure.
Having this report makes "my device doesn't work" easier to solve
as a bug report.

Hence it seems to me that a driver which can handle a failure
with a good fallback should indicate this with a flag to the
VM layer.

	Regards
		Oliver
 

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

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

On Mon, 2016-01-11 at 11:15 -0500, Konrad Rzeszutek Wilk wrote:
> On Mon, Jan 11, 2016 at 12:08:26PM +0100, Oliver Neukum wrote:

> > I suspect many drivers to be unable to deal well with a failure.
> > Having this report makes "my device doesn't work" easier to solve
> > as a bug report.
> > 
> > Hence it seems to me that a driver which can handle a failure
> > with a good fallback should indicate this with a flag to the
> > VM layer.
> 
> s/VM/device layer?

Actually no, if a driver does something that cannot be satisfied at all,
that should be logged. What we would like to go away is the log entry
due to a failure to allocate memory under memory pressure or
fragmentation.

	Regards
		Oliver

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

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

On Wed, Nov 25, 2015 at 7:19 PM, Steinar H. Gunderson <sesse@google.com> wrote:
> Add a new interface for userspace to preallocate memory that can be
> used with usbfs. This gives two primary benefits:

Extended testing over one hour shows significant improved CPU usage
and stability:
The old CPU usage is about 12% +/- 5%. The new CPU usage is 4% +/- 1%.

The setup is an i7-4600U laptop running Kinect v2 with 260MB/s
isochronous and 20MB/s bulk transfers.

The improvement in reducing jitter is particularly preferable to
Kinect v2 driver because the isochronous data is structured and one
packet lost means discarding a lot of data.

Regards,
Lingzhu

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

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

On Fri, Jan 08, 2016 at 11:00:54PM -0500, Lingzhu Xiang wrote:
> Extended testing over one hour shows significant improved CPU usage
> and stability:
> The old CPU usage is about 12% +/- 5%. The new CPU usage is 4% +/- 1%.
> 
> The setup is an i7-4600U laptop running Kinect v2 with 260MB/s
> isochronous and 20MB/s bulk transfers.
> 
> The improvement in reducing jitter is particularly preferable to
> Kinect v2 driver because the isochronous data is structured and one
> packet lost means discarding a lot of data.

Thanks for testing this! It's not unexpected, but it's good to see others
are also seeing performance improvements.

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

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

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

This is a completely broken usage of the mmap interface.  if you use
mmap on a device file you must use the actual mmap for the data
transfer.

Our interface for zero copy reads/writes is O_DIRECT, and that requires
not special memory allocation, just proper alignment.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

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

On Tue, Jan 05, 2016 at 10:49:49PM -0800, Christoph Hellwig wrote:
> This is a completely broken usage of the mmap interface.  if you use
> mmap on a device file you must use the actual mmap for the data
> transfer.

Really? V4L does exactly the same thing, from what I can see. It's just a way
of allocating memory with specific properties, roughly similar to hugetlbfs.

> Our interface for zero copy reads/writes is O_DIRECT, and that requires
> not special memory allocation, just proper alignment.

But that assumes you are using I/O using read()/write(). There's no way you
can shoehorn USB isochronous reads into the read() interface, O_DIRECT or not.

/* Steinar */
-- 
Software Engineer, Google Switzerland
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

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

Steinar H. Gunderson wrote:
> > Our interface for zero copy reads/writes is O_DIRECT, and that requires
> > not special memory allocation, just proper alignment.
> 
> But that assumes you are using I/O using read()/write(). There's no way you
> can shoehorn USB isochronous reads into the read() interface, O_DIRECT or not.

How about aio?


//Peter
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

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

On Wed, Jan 06, 2016 at 04:22:12PM +0100, Peter Stuge wrote:
>>> Our interface for zero copy reads/writes is O_DIRECT, and that requires
>>> not special memory allocation, just proper alignment.
>> But that assumes you are using I/O using read()/write(). There's no way you
>> can shoehorn USB isochronous reads into the read() interface, O_DIRECT or not.
> How about aio?

I don't really see how; a USB device does not look much like a file. (Where
would you stick the endpoint, for one? And how would you ever submit an URB
with multiple packets in it, which is essential?) It feels a bit like
trying to use UDP sockets with only read() and write().

In any case, the usbfs interface already exists and is stable. This is about
extending it; replacing it with something new from scratch to get zerocopy
would seem overkill.

/* Steinar */
-- 
Software Engineer, Google Switzerland
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

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

On Wed, 6 Jan 2016, Peter Stuge wrote:

> Steinar H. Gunderson wrote:
> > > Our interface for zero copy reads/writes is O_DIRECT, and that requires
> > > not special memory allocation, just proper alignment.
> > 
> > But that assumes you are using I/O using read()/write(). There's no way you
> > can shoehorn USB isochronous reads into the read() interface, O_DIRECT or not.
> 
> How about aio?

aio is not zerocopy.  And it also doesn't solve the memory allocation 
problem that originally affected both Steiner and Markus.

(Were you thinking of "asynchronous" instead of "isochronous"?)

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

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

On Wed, 6 Jan 2016, Steinar H. Gunderson wrote:

> On Tue, Jan 05, 2016 at 10:49:49PM -0800, Christoph Hellwig wrote:
> > This is a completely broken usage of the mmap interface.  if you use
> > mmap on a device file you must use the actual mmap for the data
> > transfer.
> 
> Really? V4L does exactly the same thing, from what I can see. It's just a way
> of allocating memory with specific properties, roughly similar to hugetlbfs.
> 
> > Our interface for zero copy reads/writes is O_DIRECT, and that requires
> > not special memory allocation, just proper alignment.
> 
> But that assumes you are using I/O using read()/write(). There's no way you
> can shoehorn USB isochronous reads into the read() interface, O_DIRECT or not.

Indeed, the I/O operations we are using with mmap here are not reads or 
writes; they are ioctls.  As far as I know, the kernel doesn't have any 
defined interface for zerocopy ioctls.

Furthermore, this approach _does_ use the mmap for data transfers.  I'm 
not sure what Christoph was referring to.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web