* Re: [git:media_tree/master] [media] vb2: Push mmap_sem down to memops
[not found] <E1Yo8Jc-0000K9-Sd@www.linuxtv.org>
@ 2015-06-11 8:52 ` Hans Verkuil
2015-06-18 10:12 ` Jan Kara
0 siblings, 1 reply; 3+ messages in thread
From: Hans Verkuil @ 2015-06-11 8:52 UTC (permalink / raw)
To: linux-media, Jan Kara, Mauro Carvalho Chehab
Jan,
This patch causes a regressing in videobuf2-dma-sg with a potential deadlock:
[ 82.290231] ======================================================
[ 82.290232] [ INFO: possible circular locking dependency detected ]
[ 82.290235] 4.1.0-rc3-tb1 #12 Not tainted
[ 82.290236] -------------------------------------------------------
[ 82.290238] qv4l2/1262 is trying to acquire lock:
[ 82.290240] (&mm->mmap_sem){++++++}, at: [<ffffffffa007a870>] vb2_dma_sg_put_userptr+0xf0/0x170 [videobuf2_dma_sg]
[ 82.290247]
but task is already holding lock:
[ 82.290249] (&q->mmap_lock){+.+.+.}, at: [<ffffffffa006a9ec>] __reqbufs.isra.13+0x7c/0x410 [videobuf2_core]
[ 82.290255]
which lock already depends on the new lock.
[ 82.290257]
the existing dependency chain (in reverse order) is:
[ 82.290259]
-> #1 (&q->mmap_lock){+.+.+.}:
[ 82.290262] [<ffffffff8110bab9>] lock_acquire+0xc9/0x290
[ 82.290267] [<ffffffff81a9fc1e>] mutex_lock_nested+0x4e/0x3f0
[ 82.290270] [<ffffffffa00654a2>] vb2_mmap+0x232/0x350 [videobuf2_core]
[ 82.290273] [<ffffffffa0067ab5>] vb2_fop_mmap+0x25/0x30 [videobuf2_core]
[ 82.290276] [<ffffffffa002d11a>] v4l2_mmap+0x5a/0x90 [videodev]
[ 82.290281] [<ffffffff811f4bcb>] mmap_region+0x3bb/0x5f0
[ 82.290285] [<ffffffff811f511f>] do_mmap_pgoff+0x31f/0x400
[ 82.290288] [<ffffffff811dfbe0>] vm_mmap_pgoff+0x90/0xc0
[ 82.290291] [<ffffffff811f35af>] SyS_mmap_pgoff+0x1df/0x290
[ 82.290294] [<ffffffff8105ef42>] SyS_mmap+0x22/0x30
[ 82.290297] [<ffffffff81aa4517>] system_call_fastpath+0x12/0x6f
[ 82.290300]
-> #0 (&mm->mmap_sem){++++++}:
[ 82.290303] [<ffffffff8110adb3>] __lock_acquire+0x1d53/0x1fe0
[ 82.290306] [<ffffffff8110bab9>] lock_acquire+0xc9/0x290
[ 82.290308] [<ffffffff81aa1924>] down_read+0x34/0x50
[ 82.290311] [<ffffffffa007a870>] vb2_dma_sg_put_userptr+0xf0/0x170 [videobuf2_dma_sg]
[ 82.290314] [<ffffffffa0066656>] __vb2_queue_free+0x156/0x5f0 [videobuf2_core]
[ 82.290317] [<ffffffffa006aa0f>] __reqbufs.isra.13+0x9f/0x410 [videobuf2_core]
[ 82.290320] [<ffffffffa006b084>] vb2_ioctl_reqbufs+0x74/0xb0 [videobuf2_core]
[ 82.290323] [<ffffffffa0033d83>] v4l_reqbufs+0x43/0x50 [videodev]
[ 82.290327] [<ffffffffa00329f4>] __video_do_ioctl+0x274/0x310 [videodev]
[ 82.290331] [<ffffffffa00349a8>] video_usercopy+0x378/0x8f0 [videodev]
[ 82.290336] [<ffffffffa0034f35>] video_ioctl2+0x15/0x20 [videodev]
[ 82.290340] [<ffffffffa002d6d0>] v4l2_ioctl+0xd0/0xf0 [videodev]
[ 82.290343] [<ffffffff81238258>] do_vfs_ioctl+0x308/0x540
[ 82.290347] [<ffffffff81238511>] SyS_ioctl+0x81/0xa0
[ 82.290349] [<ffffffff81aa4517>] system_call_fastpath+0x12/0x6f
[ 82.290352]
other info that might help us debug this:
[ 82.290354] Possible unsafe locking scenario:
[ 82.290356] CPU0 CPU1
[ 82.290357] ---- ----
[ 82.290358] lock(&q->mmap_lock);
[ 82.290360] lock(&mm->mmap_sem);
[ 82.290362] lock(&q->mmap_lock);
[ 82.290365] lock(&mm->mmap_sem);
[ 82.290367]
*** DEADLOCK ***
[ 82.290369] 2 locks held by qv4l2/1262:
[ 82.290370] #0: (&s->lock){+.+.+.}, at: [<ffffffffa002d65f>] v4l2_ioctl+0x5f/0xf0 [videodev]
[ 82.290376] #1: (&q->mmap_lock){+.+.+.}, at: [<ffffffffa006a9ec>] __reqbufs.isra.13+0x7c/0x410 [videobuf2_core]
[ 82.290382]
stack backtrace:
[ 82.290385] CPU: 1 PID: 1262 Comm: qv4l2 Not tainted 4.1.0-rc3-tb1 #12
[ 82.290387] Hardware name: /DH67CF, BIOS BLH6710H.86A.0105.2011.0301.1654 03/01/2011
[ 82.290388] ffffffff82c46890 ffff8800b4bfb968 ffffffff81a98687 0000000000000007
[ 82.290392] ffffffff82c46890 ffff8800b4bfb9b8 ffffffff8110785d 0000000000000000
[ 82.290395] ffff8800b4bfba28 0000000000000001 ffff8800d51ce718 0000000000000001
[ 82.290399] Call Trace:
[ 82.290402] [<ffffffff81a98687>] dump_stack+0x4f/0x7b
[ 82.290405] [<ffffffff8110785d>] print_circular_bug+0x1cd/0x230
[ 82.290407] [<ffffffff8110adb3>] __lock_acquire+0x1d53/0x1fe0
[ 82.290411] [<ffffffff812142b9>] ? kfree+0x169/0x570
[ 82.290414] [<ffffffff8110bab9>] lock_acquire+0xc9/0x290
[ 82.290416] [<ffffffffa007a870>] ? vb2_dma_sg_put_userptr+0xf0/0x170 [videobuf2_dma_sg]
[ 82.290419] [<ffffffff81aa1924>] down_read+0x34/0x50
[ 82.290421] [<ffffffffa007a870>] ? vb2_dma_sg_put_userptr+0xf0/0x170 [videobuf2_dma_sg]
[ 82.290424] [<ffffffffa007a870>] vb2_dma_sg_put_userptr+0xf0/0x170 [videobuf2_dma_sg]
[ 82.290427] [<ffffffffa0066656>] __vb2_queue_free+0x156/0x5f0 [videobuf2_core]
[ 82.290430] [<ffffffffa006aa0f>] __reqbufs.isra.13+0x9f/0x410 [videobuf2_core]
[ 82.290434] [<ffffffff811c8a59>] ? free_hot_cold_page+0x159/0x200
[ 82.290437] [<ffffffffa006b084>] vb2_ioctl_reqbufs+0x74/0xb0 [videobuf2_core]
[ 82.290441] [<ffffffffa0033d83>] v4l_reqbufs+0x43/0x50 [videodev]
[ 82.290445] [<ffffffffa00329f4>] __video_do_ioctl+0x274/0x310 [videodev]
[ 82.290449] [<ffffffffa0032780>] ? v4l_querycap+0x70/0x70 [videodev]
[ 82.290453] [<ffffffffa00349a8>] video_usercopy+0x378/0x8f0 [videodev]
[ 82.290456] [<ffffffff81108b11>] ? mark_held_locks+0x71/0xa0
[ 82.290458] [<ffffffff81108d4d>] ? trace_hardirqs_on+0xd/0x10
[ 82.290461] [<ffffffff81a9f98e>] ? mutex_lock_interruptible_nested+0x25e/0x4a0
[ 82.290464] [<ffffffffa002d65f>] ? v4l2_ioctl+0x5f/0xf0 [videodev]
[ 82.290468] [<ffffffffa002d65f>] ? v4l2_ioctl+0x5f/0xf0 [videodev]
[ 82.290472] [<ffffffffa0034f35>] video_ioctl2+0x15/0x20 [videodev]
[ 82.290475] [<ffffffffa002d6d0>] v4l2_ioctl+0xd0/0xf0 [videodev]
[ 82.290478] [<ffffffff81238258>] do_vfs_ioctl+0x308/0x540
[ 82.290481] [<ffffffff8124476c>] ? __fget_light+0x6c/0xa0
[ 82.290484] [<ffffffff81238511>] SyS_ioctl+0x81/0xa0
[ 82.290487] [<ffffffff81aa4517>] system_call_fastpath+0x12/0x6f
The problem is that the mmap_sem is now taken in vb2_dma_sg_put_userptr() when
that didn't happen before. This is fine when called from __qbuf_userptr, but
not when called from __vb2_queue_free. I will see if I have time to dig into
this during the weekend and solve it, but if not, then this has to be reverted
and the get_vaddr_frames() patch series postponed since it depends on this one.
Regards,
Hans
On 05/01/15 12:17, Mauro Carvalho Chehab wrote:
> This is an automatic generated email to let you know that the following patch were queued at the
> http://git.linuxtv.org/cgit.cgi/media_tree.git tree:
>
> Subject: [media] vb2: Push mmap_sem down to memops
> Author: Jan Kara <jack@suse.cz>
> Date: Tue Mar 17 08:56:31 2015 -0300
>
> Currently vb2 core acquires mmap_sem just around call to
> __qbuf_userptr(). However since commit f035eb4e976ef5 (videobuf2: fix
> lockdep warning) it isn't necessary to acquire it so early as we no
> longer have to drop queue mutex before acquiring mmap_sem. So push
> acquisition of mmap_sem down into .get_userptr and .put_userptr memops
> so that the semaphore is acquired for a shorter time and it is clearer
> what it is needed for.
>
> Signed-off-by: Jan Kara <jack@suse.cz>
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
>
> drivers/media/v4l2-core/videobuf2-core.c | 2 --
> drivers/media/v4l2-core/videobuf2-dma-contig.c | 7 +++++++
> drivers/media/v4l2-core/videobuf2-dma-sg.c | 6 ++++++
> drivers/media/v4l2-core/videobuf2-vmalloc.c | 6 +++++-
> 4 files changed, 18 insertions(+), 3 deletions(-)
>
> ---
>
> http://git.linuxtv.org/cgit.cgi/media_tree.git/commit/?id=48b25a3a713b90988b6882d318f7c0a6bed9aabc
>
> diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
> index 66ada01..20cdbc0 100644
> --- a/drivers/media/v4l2-core/videobuf2-core.c
> +++ b/drivers/media/v4l2-core/videobuf2-core.c
> @@ -1657,9 +1657,7 @@ static int __buf_prepare(struct vb2_buffer *vb, const struct v4l2_buffer *b)
> ret = __qbuf_mmap(vb, b);
> break;
> case V4L2_MEMORY_USERPTR:
> - down_read(¤t->mm->mmap_sem);
> ret = __qbuf_userptr(vb, b);
> - up_read(¤t->mm->mmap_sem);
> break;
> case V4L2_MEMORY_DMABUF:
> ret = __qbuf_dmabuf(vb, b);
> diff --git a/drivers/media/v4l2-core/videobuf2-dma-contig.c b/drivers/media/v4l2-core/videobuf2-dma-contig.c
> index 644dec7..620c4aa 100644
> --- a/drivers/media/v4l2-core/videobuf2-dma-contig.c
> +++ b/drivers/media/v4l2-core/videobuf2-dma-contig.c
> @@ -532,7 +532,9 @@ static void vb2_dc_put_userptr(void *buf_priv)
> sg_free_table(sgt);
> kfree(sgt);
> }
> + down_read(¤t->mm->mmap_sem);
> vb2_put_vma(buf->vma);
> + up_read(¤t->mm->mmap_sem);
> kfree(buf);
> }
>
> @@ -616,6 +618,7 @@ static void *vb2_dc_get_userptr(void *alloc_ctx, unsigned long vaddr,
> goto fail_buf;
> }
>
> + down_read(¤t->mm->mmap_sem);
> /* current->mm->mmap_sem is taken by videobuf2 core */
> vma = find_vma(current->mm, vaddr);
> if (!vma) {
> @@ -642,6 +645,7 @@ static void *vb2_dc_get_userptr(void *alloc_ctx, unsigned long vaddr,
> if (ret) {
> unsigned long pfn;
> if (vb2_dc_get_user_pfn(start, n_pages, vma, &pfn) == 0) {
> + up_read(¤t->mm->mmap_sem);
> buf->dma_addr = vb2_dc_pfn_to_dma(buf->dev, pfn);
> buf->size = size;
> kfree(pages);
> @@ -651,6 +655,7 @@ static void *vb2_dc_get_userptr(void *alloc_ctx, unsigned long vaddr,
> pr_err("failed to get user pages\n");
> goto fail_vma;
> }
> + up_read(¤t->mm->mmap_sem);
>
> sgt = kzalloc(sizeof(*sgt), GFP_KERNEL);
> if (!sgt) {
> @@ -713,10 +718,12 @@ fail_get_user_pages:
> while (n_pages)
> put_page(pages[--n_pages]);
>
> + down_read(¤t->mm->mmap_sem);
> fail_vma:
> vb2_put_vma(buf->vma);
>
> fail_pages:
> + up_read(¤t->mm->mmap_sem);
> kfree(pages); /* kfree is NULL-proof */
>
> fail_buf:
> diff --git a/drivers/media/v4l2-core/videobuf2-dma-sg.c b/drivers/media/v4l2-core/videobuf2-dma-sg.c
> index 45c708e..afd4b51 100644
> --- a/drivers/media/v4l2-core/videobuf2-dma-sg.c
> +++ b/drivers/media/v4l2-core/videobuf2-dma-sg.c
> @@ -263,6 +263,7 @@ static void *vb2_dma_sg_get_userptr(void *alloc_ctx, unsigned long vaddr,
> if (!buf->pages)
> goto userptr_fail_alloc_pages;
>
> + down_read(¤t->mm->mmap_sem);
> vma = find_vma(current->mm, vaddr);
> if (!vma) {
> dprintk(1, "no vma for address %lu\n", vaddr);
> @@ -301,6 +302,7 @@ static void *vb2_dma_sg_get_userptr(void *alloc_ctx, unsigned long vaddr,
> 1, /* force */
> buf->pages,
> NULL);
> + up_read(¤t->mm->mmap_sem);
>
> if (num_pages_from_user != buf->num_pages)
> goto userptr_fail_get_user_pages;
> @@ -328,8 +330,10 @@ userptr_fail_get_user_pages:
> if (!vma_is_io(buf->vma))
> while (--num_pages_from_user >= 0)
> put_page(buf->pages[num_pages_from_user]);
> + down_read(¤t->mm->mmap_sem);
> vb2_put_vma(buf->vma);
> userptr_fail_find_vma:
> + up_read(¤t->mm->mmap_sem);
> kfree(buf->pages);
> userptr_fail_alloc_pages:
> kfree(buf);
> @@ -362,7 +366,9 @@ static void vb2_dma_sg_put_userptr(void *buf_priv)
> put_page(buf->pages[i]);
> }
> kfree(buf->pages);
> + down_read(¤t->mm->mmap_sem);
> vb2_put_vma(buf->vma);
> + up_read(¤t->mm->mmap_sem);
> kfree(buf);
> }
>
> diff --git a/drivers/media/v4l2-core/videobuf2-vmalloc.c b/drivers/media/v4l2-core/videobuf2-vmalloc.c
> index 657ab30..0ba40be 100644
> --- a/drivers/media/v4l2-core/videobuf2-vmalloc.c
> +++ b/drivers/media/v4l2-core/videobuf2-vmalloc.c
> @@ -89,7 +89,7 @@ static void *vb2_vmalloc_get_userptr(void *alloc_ctx, unsigned long vaddr,
> offset = vaddr & ~PAGE_MASK;
> buf->size = size;
>
> -
> + down_read(¤t->mm->mmap_sem);
> vma = find_vma(current->mm, vaddr);
> if (vma && (vma->vm_flags & VM_PFNMAP) && (vma->vm_pgoff)) {
> if (vb2_get_contig_userptr(vaddr, size, &vma, &physp))
> @@ -121,6 +121,7 @@ static void *vb2_vmalloc_get_userptr(void *alloc_ctx, unsigned long vaddr,
> if (!buf->vaddr)
> goto fail_get_user_pages;
> }
> + up_read(¤t->mm->mmap_sem);
>
> buf->vaddr += offset;
> return buf;
> @@ -133,6 +134,7 @@ fail_get_user_pages:
> kfree(buf->pages);
>
> fail_pages_array_alloc:
> + up_read(¤t->mm->mmap_sem);
> kfree(buf);
>
> return NULL;
> @@ -144,6 +146,7 @@ static void vb2_vmalloc_put_userptr(void *buf_priv)
> unsigned long vaddr = (unsigned long)buf->vaddr & PAGE_MASK;
> unsigned int i;
>
> + down_read(¤t->mm->mmap_sem);
> if (buf->pages) {
> if (vaddr)
> vm_unmap_ram((void *)vaddr, buf->n_pages);
> @@ -157,6 +160,7 @@ static void vb2_vmalloc_put_userptr(void *buf_priv)
> vb2_put_vma(buf->vma);
> iounmap((__force void __iomem *)buf->vaddr);
> }
> + up_read(¤t->mm->mmap_sem);
> kfree(buf);
> }
>
>
> _______________________________________________
> linuxtv-commits mailing list
> linuxtv-commits@linuxtv.org
> http://www.linuxtv.org/cgi-bin/mailman/listinfo/linuxtv-commits
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [git:media_tree/master] [media] vb2: Push mmap_sem down to memops
2015-06-11 8:52 ` [git:media_tree/master] [media] vb2: Push mmap_sem down to memops Hans Verkuil
@ 2015-06-18 10:12 ` Jan Kara
2015-06-18 12:44 ` Mauro Carvalho Chehab
0 siblings, 1 reply; 3+ messages in thread
From: Jan Kara @ 2015-06-18 10:12 UTC (permalink / raw)
To: Hans Verkuil; +Cc: linux-media, Jan Kara, Mauro Carvalho Chehab
On Thu 11-06-15 10:52:22, Hans Verkuil wrote:
> Jan,
>
> This patch causes a regressing in videobuf2-dma-sg with a potential deadlock:
>
> [ 82.290231] ======================================================
> [ 82.290232] [ INFO: possible circular locking dependency detected ]
> [ 82.290235] 4.1.0-rc3-tb1 #12 Not tainted
> [ 82.290236] -------------------------------------------------------
> [ 82.290238] qv4l2/1262 is trying to acquire lock:
> [ 82.290240] (&mm->mmap_sem){++++++}, at: [<ffffffffa007a870>] vb2_dma_sg_put_userptr+0xf0/0x170 [videobuf2_dma_sg]
> [ 82.290247]
> but task is already holding lock:
> [ 82.290249] (&q->mmap_lock){+.+.+.}, at: [<ffffffffa006a9ec>] __reqbufs.isra.13+0x7c/0x410 [videobuf2_core]
> [ 82.290255]
> which lock already depends on the new lock.
Thanks for the report! So I finally got to this after returning from
vacation and dealing with more urgent stuff. Looking more at the code it
seems to me that this particular code path going through
vb2_ioctl_reqbufs() didn't have previously any mmap_sem protection. Thus we
could call vma->vm_ops->close() from vb2_put_vma() without holding mmap_sem
which is against the locking protocol.
My patch unintentionally fixed this but that introduced the lock inversion
lockdep complains about. Now the full series removes the need for getting
VMA reference in videobuf2 code so we don't need mmap_sem in put_userptr
callbacks at all. So when the whole series is applied we are fine again.
So how shall we proceed? Just slap this patch at the beginning of the
series to make sure it gets merged at once so the lock inversion isn't
there for long? Or alternatively I can just remove mmap_sem from
put_userptr() in this series. That will somewhat increase the amount of
calls to vma->vm_ops->close() without mmap_sem util the whole series is
applied. Any opinions guys?
Honza
> [ 82.290257]
> the existing dependency chain (in reverse order) is:
> [ 82.290259]
> -> #1 (&q->mmap_lock){+.+.+.}:
> [ 82.290262] [<ffffffff8110bab9>] lock_acquire+0xc9/0x290
> [ 82.290267] [<ffffffff81a9fc1e>] mutex_lock_nested+0x4e/0x3f0
> [ 82.290270] [<ffffffffa00654a2>] vb2_mmap+0x232/0x350 [videobuf2_core]
> [ 82.290273] [<ffffffffa0067ab5>] vb2_fop_mmap+0x25/0x30 [videobuf2_core]
> [ 82.290276] [<ffffffffa002d11a>] v4l2_mmap+0x5a/0x90 [videodev]
> [ 82.290281] [<ffffffff811f4bcb>] mmap_region+0x3bb/0x5f0
> [ 82.290285] [<ffffffff811f511f>] do_mmap_pgoff+0x31f/0x400
> [ 82.290288] [<ffffffff811dfbe0>] vm_mmap_pgoff+0x90/0xc0
> [ 82.290291] [<ffffffff811f35af>] SyS_mmap_pgoff+0x1df/0x290
> [ 82.290294] [<ffffffff8105ef42>] SyS_mmap+0x22/0x30
> [ 82.290297] [<ffffffff81aa4517>] system_call_fastpath+0x12/0x6f
> [ 82.290300]
> -> #0 (&mm->mmap_sem){++++++}:
> [ 82.290303] [<ffffffff8110adb3>] __lock_acquire+0x1d53/0x1fe0
> [ 82.290306] [<ffffffff8110bab9>] lock_acquire+0xc9/0x290
> [ 82.290308] [<ffffffff81aa1924>] down_read+0x34/0x50
> [ 82.290311] [<ffffffffa007a870>] vb2_dma_sg_put_userptr+0xf0/0x170 [videobuf2_dma_sg]
> [ 82.290314] [<ffffffffa0066656>] __vb2_queue_free+0x156/0x5f0 [videobuf2_core]
> [ 82.290317] [<ffffffffa006aa0f>] __reqbufs.isra.13+0x9f/0x410 [videobuf2_core]
> [ 82.290320] [<ffffffffa006b084>] vb2_ioctl_reqbufs+0x74/0xb0 [videobuf2_core]
> [ 82.290323] [<ffffffffa0033d83>] v4l_reqbufs+0x43/0x50 [videodev]
> [ 82.290327] [<ffffffffa00329f4>] __video_do_ioctl+0x274/0x310 [videodev]
> [ 82.290331] [<ffffffffa00349a8>] video_usercopy+0x378/0x8f0 [videodev]
> [ 82.290336] [<ffffffffa0034f35>] video_ioctl2+0x15/0x20 [videodev]
> [ 82.290340] [<ffffffffa002d6d0>] v4l2_ioctl+0xd0/0xf0 [videodev]
> [ 82.290343] [<ffffffff81238258>] do_vfs_ioctl+0x308/0x540
> [ 82.290347] [<ffffffff81238511>] SyS_ioctl+0x81/0xa0
> [ 82.290349] [<ffffffff81aa4517>] system_call_fastpath+0x12/0x6f
> [ 82.290352]
> other info that might help us debug this:
>
> [ 82.290354] Possible unsafe locking scenario:
>
> [ 82.290356] CPU0 CPU1
> [ 82.290357] ---- ----
> [ 82.290358] lock(&q->mmap_lock);
> [ 82.290360] lock(&mm->mmap_sem);
> [ 82.290362] lock(&q->mmap_lock);
> [ 82.290365] lock(&mm->mmap_sem);
> [ 82.290367]
> *** DEADLOCK ***
>
> [ 82.290369] 2 locks held by qv4l2/1262:
> [ 82.290370] #0: (&s->lock){+.+.+.}, at: [<ffffffffa002d65f>] v4l2_ioctl+0x5f/0xf0 [videodev]
> [ 82.290376] #1: (&q->mmap_lock){+.+.+.}, at: [<ffffffffa006a9ec>] __reqbufs.isra.13+0x7c/0x410 [videobuf2_core]
> [ 82.290382]
> stack backtrace:
> [ 82.290385] CPU: 1 PID: 1262 Comm: qv4l2 Not tainted 4.1.0-rc3-tb1 #12
> [ 82.290387] Hardware name: /DH67CF, BIOS BLH6710H.86A.0105.2011.0301.1654 03/01/2011
> [ 82.290388] ffffffff82c46890 ffff8800b4bfb968 ffffffff81a98687 0000000000000007
> [ 82.290392] ffffffff82c46890 ffff8800b4bfb9b8 ffffffff8110785d 0000000000000000
> [ 82.290395] ffff8800b4bfba28 0000000000000001 ffff8800d51ce718 0000000000000001
> [ 82.290399] Call Trace:
> [ 82.290402] [<ffffffff81a98687>] dump_stack+0x4f/0x7b
> [ 82.290405] [<ffffffff8110785d>] print_circular_bug+0x1cd/0x230
> [ 82.290407] [<ffffffff8110adb3>] __lock_acquire+0x1d53/0x1fe0
> [ 82.290411] [<ffffffff812142b9>] ? kfree+0x169/0x570
> [ 82.290414] [<ffffffff8110bab9>] lock_acquire+0xc9/0x290
> [ 82.290416] [<ffffffffa007a870>] ? vb2_dma_sg_put_userptr+0xf0/0x170 [videobuf2_dma_sg]
> [ 82.290419] [<ffffffff81aa1924>] down_read+0x34/0x50
> [ 82.290421] [<ffffffffa007a870>] ? vb2_dma_sg_put_userptr+0xf0/0x170 [videobuf2_dma_sg]
> [ 82.290424] [<ffffffffa007a870>] vb2_dma_sg_put_userptr+0xf0/0x170 [videobuf2_dma_sg]
> [ 82.290427] [<ffffffffa0066656>] __vb2_queue_free+0x156/0x5f0 [videobuf2_core]
> [ 82.290430] [<ffffffffa006aa0f>] __reqbufs.isra.13+0x9f/0x410 [videobuf2_core]
> [ 82.290434] [<ffffffff811c8a59>] ? free_hot_cold_page+0x159/0x200
> [ 82.290437] [<ffffffffa006b084>] vb2_ioctl_reqbufs+0x74/0xb0 [videobuf2_core]
> [ 82.290441] [<ffffffffa0033d83>] v4l_reqbufs+0x43/0x50 [videodev]
> [ 82.290445] [<ffffffffa00329f4>] __video_do_ioctl+0x274/0x310 [videodev]
> [ 82.290449] [<ffffffffa0032780>] ? v4l_querycap+0x70/0x70 [videodev]
> [ 82.290453] [<ffffffffa00349a8>] video_usercopy+0x378/0x8f0 [videodev]
> [ 82.290456] [<ffffffff81108b11>] ? mark_held_locks+0x71/0xa0
> [ 82.290458] [<ffffffff81108d4d>] ? trace_hardirqs_on+0xd/0x10
> [ 82.290461] [<ffffffff81a9f98e>] ? mutex_lock_interruptible_nested+0x25e/0x4a0
> [ 82.290464] [<ffffffffa002d65f>] ? v4l2_ioctl+0x5f/0xf0 [videodev]
> [ 82.290468] [<ffffffffa002d65f>] ? v4l2_ioctl+0x5f/0xf0 [videodev]
> [ 82.290472] [<ffffffffa0034f35>] video_ioctl2+0x15/0x20 [videodev]
> [ 82.290475] [<ffffffffa002d6d0>] v4l2_ioctl+0xd0/0xf0 [videodev]
> [ 82.290478] [<ffffffff81238258>] do_vfs_ioctl+0x308/0x540
> [ 82.290481] [<ffffffff8124476c>] ? __fget_light+0x6c/0xa0
> [ 82.290484] [<ffffffff81238511>] SyS_ioctl+0x81/0xa0
> [ 82.290487] [<ffffffff81aa4517>] system_call_fastpath+0x12/0x6f
>
> The problem is that the mmap_sem is now taken in vb2_dma_sg_put_userptr() when
> that didn't happen before. This is fine when called from __qbuf_userptr, but
> not when called from __vb2_queue_free. I will see if I have time to dig into
> this during the weekend and solve it, but if not, then this has to be reverted
> and the get_vaddr_frames() patch series postponed since it depends on this one.
>
> Regards,
>
> Hans
>
> On 05/01/15 12:17, Mauro Carvalho Chehab wrote:
> > This is an automatic generated email to let you know that the following patch were queued at the
> > http://git.linuxtv.org/cgit.cgi/media_tree.git tree:
> >
> > Subject: [media] vb2: Push mmap_sem down to memops
> > Author: Jan Kara <jack@suse.cz>
> > Date: Tue Mar 17 08:56:31 2015 -0300
> >
> > Currently vb2 core acquires mmap_sem just around call to
> > __qbuf_userptr(). However since commit f035eb4e976ef5 (videobuf2: fix
> > lockdep warning) it isn't necessary to acquire it so early as we no
> > longer have to drop queue mutex before acquiring mmap_sem. So push
> > acquisition of mmap_sem down into .get_userptr and .put_userptr memops
> > so that the semaphore is acquired for a shorter time and it is clearer
> > what it is needed for.
> >
> > Signed-off-by: Jan Kara <jack@suse.cz>
> > Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> > Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
> >
> > drivers/media/v4l2-core/videobuf2-core.c | 2 --
> > drivers/media/v4l2-core/videobuf2-dma-contig.c | 7 +++++++
> > drivers/media/v4l2-core/videobuf2-dma-sg.c | 6 ++++++
> > drivers/media/v4l2-core/videobuf2-vmalloc.c | 6 +++++-
> > 4 files changed, 18 insertions(+), 3 deletions(-)
> >
> > ---
> >
> > http://git.linuxtv.org/cgit.cgi/media_tree.git/commit/?id=48b25a3a713b90988b6882d318f7c0a6bed9aabc
> >
> > diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
> > index 66ada01..20cdbc0 100644
> > --- a/drivers/media/v4l2-core/videobuf2-core.c
> > +++ b/drivers/media/v4l2-core/videobuf2-core.c
> > @@ -1657,9 +1657,7 @@ static int __buf_prepare(struct vb2_buffer *vb, const struct v4l2_buffer *b)
> > ret = __qbuf_mmap(vb, b);
> > break;
> > case V4L2_MEMORY_USERPTR:
> > - down_read(¤t->mm->mmap_sem);
> > ret = __qbuf_userptr(vb, b);
> > - up_read(¤t->mm->mmap_sem);
> > break;
> > case V4L2_MEMORY_DMABUF:
> > ret = __qbuf_dmabuf(vb, b);
> > diff --git a/drivers/media/v4l2-core/videobuf2-dma-contig.c b/drivers/media/v4l2-core/videobuf2-dma-contig.c
> > index 644dec7..620c4aa 100644
> > --- a/drivers/media/v4l2-core/videobuf2-dma-contig.c
> > +++ b/drivers/media/v4l2-core/videobuf2-dma-contig.c
> > @@ -532,7 +532,9 @@ static void vb2_dc_put_userptr(void *buf_priv)
> > sg_free_table(sgt);
> > kfree(sgt);
> > }
> > + down_read(¤t->mm->mmap_sem);
> > vb2_put_vma(buf->vma);
> > + up_read(¤t->mm->mmap_sem);
> > kfree(buf);
> > }
> >
> > @@ -616,6 +618,7 @@ static void *vb2_dc_get_userptr(void *alloc_ctx, unsigned long vaddr,
> > goto fail_buf;
> > }
> >
> > + down_read(¤t->mm->mmap_sem);
> > /* current->mm->mmap_sem is taken by videobuf2 core */
> > vma = find_vma(current->mm, vaddr);
> > if (!vma) {
> > @@ -642,6 +645,7 @@ static void *vb2_dc_get_userptr(void *alloc_ctx, unsigned long vaddr,
> > if (ret) {
> > unsigned long pfn;
> > if (vb2_dc_get_user_pfn(start, n_pages, vma, &pfn) == 0) {
> > + up_read(¤t->mm->mmap_sem);
> > buf->dma_addr = vb2_dc_pfn_to_dma(buf->dev, pfn);
> > buf->size = size;
> > kfree(pages);
> > @@ -651,6 +655,7 @@ static void *vb2_dc_get_userptr(void *alloc_ctx, unsigned long vaddr,
> > pr_err("failed to get user pages\n");
> > goto fail_vma;
> > }
> > + up_read(¤t->mm->mmap_sem);
> >
> > sgt = kzalloc(sizeof(*sgt), GFP_KERNEL);
> > if (!sgt) {
> > @@ -713,10 +718,12 @@ fail_get_user_pages:
> > while (n_pages)
> > put_page(pages[--n_pages]);
> >
> > + down_read(¤t->mm->mmap_sem);
> > fail_vma:
> > vb2_put_vma(buf->vma);
> >
> > fail_pages:
> > + up_read(¤t->mm->mmap_sem);
> > kfree(pages); /* kfree is NULL-proof */
> >
> > fail_buf:
> > diff --git a/drivers/media/v4l2-core/videobuf2-dma-sg.c b/drivers/media/v4l2-core/videobuf2-dma-sg.c
> > index 45c708e..afd4b51 100644
> > --- a/drivers/media/v4l2-core/videobuf2-dma-sg.c
> > +++ b/drivers/media/v4l2-core/videobuf2-dma-sg.c
> > @@ -263,6 +263,7 @@ static void *vb2_dma_sg_get_userptr(void *alloc_ctx, unsigned long vaddr,
> > if (!buf->pages)
> > goto userptr_fail_alloc_pages;
> >
> > + down_read(¤t->mm->mmap_sem);
> > vma = find_vma(current->mm, vaddr);
> > if (!vma) {
> > dprintk(1, "no vma for address %lu\n", vaddr);
> > @@ -301,6 +302,7 @@ static void *vb2_dma_sg_get_userptr(void *alloc_ctx, unsigned long vaddr,
> > 1, /* force */
> > buf->pages,
> > NULL);
> > + up_read(¤t->mm->mmap_sem);
> >
> > if (num_pages_from_user != buf->num_pages)
> > goto userptr_fail_get_user_pages;
> > @@ -328,8 +330,10 @@ userptr_fail_get_user_pages:
> > if (!vma_is_io(buf->vma))
> > while (--num_pages_from_user >= 0)
> > put_page(buf->pages[num_pages_from_user]);
> > + down_read(¤t->mm->mmap_sem);
> > vb2_put_vma(buf->vma);
> > userptr_fail_find_vma:
> > + up_read(¤t->mm->mmap_sem);
> > kfree(buf->pages);
> > userptr_fail_alloc_pages:
> > kfree(buf);
> > @@ -362,7 +366,9 @@ static void vb2_dma_sg_put_userptr(void *buf_priv)
> > put_page(buf->pages[i]);
> > }
> > kfree(buf->pages);
> > + down_read(¤t->mm->mmap_sem);
> > vb2_put_vma(buf->vma);
> > + up_read(¤t->mm->mmap_sem);
> > kfree(buf);
> > }
> >
> > diff --git a/drivers/media/v4l2-core/videobuf2-vmalloc.c b/drivers/media/v4l2-core/videobuf2-vmalloc.c
> > index 657ab30..0ba40be 100644
> > --- a/drivers/media/v4l2-core/videobuf2-vmalloc.c
> > +++ b/drivers/media/v4l2-core/videobuf2-vmalloc.c
> > @@ -89,7 +89,7 @@ static void *vb2_vmalloc_get_userptr(void *alloc_ctx, unsigned long vaddr,
> > offset = vaddr & ~PAGE_MASK;
> > buf->size = size;
> >
> > -
> > + down_read(¤t->mm->mmap_sem);
> > vma = find_vma(current->mm, vaddr);
> > if (vma && (vma->vm_flags & VM_PFNMAP) && (vma->vm_pgoff)) {
> > if (vb2_get_contig_userptr(vaddr, size, &vma, &physp))
> > @@ -121,6 +121,7 @@ static void *vb2_vmalloc_get_userptr(void *alloc_ctx, unsigned long vaddr,
> > if (!buf->vaddr)
> > goto fail_get_user_pages;
> > }
> > + up_read(¤t->mm->mmap_sem);
> >
> > buf->vaddr += offset;
> > return buf;
> > @@ -133,6 +134,7 @@ fail_get_user_pages:
> > kfree(buf->pages);
> >
> > fail_pages_array_alloc:
> > + up_read(¤t->mm->mmap_sem);
> > kfree(buf);
> >
> > return NULL;
> > @@ -144,6 +146,7 @@ static void vb2_vmalloc_put_userptr(void *buf_priv)
> > unsigned long vaddr = (unsigned long)buf->vaddr & PAGE_MASK;
> > unsigned int i;
> >
> > + down_read(¤t->mm->mmap_sem);
> > if (buf->pages) {
> > if (vaddr)
> > vm_unmap_ram((void *)vaddr, buf->n_pages);
> > @@ -157,6 +160,7 @@ static void vb2_vmalloc_put_userptr(void *buf_priv)
> > vb2_put_vma(buf->vma);
> > iounmap((__force void __iomem *)buf->vaddr);
> > }
> > + up_read(¤t->mm->mmap_sem);
> > kfree(buf);
> > }
> >
> >
> > _______________________________________________
> > linuxtv-commits mailing list
> > linuxtv-commits@linuxtv.org
> > http://www.linuxtv.org/cgi-bin/mailman/listinfo/linuxtv-commits
> >
--
Jan Kara <jack@suse.cz>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [git:media_tree/master] [media] vb2: Push mmap_sem down to memops
2015-06-18 10:12 ` Jan Kara
@ 2015-06-18 12:44 ` Mauro Carvalho Chehab
0 siblings, 0 replies; 3+ messages in thread
From: Mauro Carvalho Chehab @ 2015-06-18 12:44 UTC (permalink / raw)
To: Jan Kara; +Cc: Hans Verkuil, linux-media, Andrew Morton
Hi Jan,
Please keep Andrew in the loop. The patch series is on his tree.
Regards,
Mauro
Em Thu, 18 Jun 2015 12:12:08 +0200
Jan Kara <jack@suse.cz> escreveu:
> On Thu 11-06-15 10:52:22, Hans Verkuil wrote:
> > Jan,
> >
> > This patch causes a regressing in videobuf2-dma-sg with a potential deadlock:
> >
> > [ 82.290231] ======================================================
> > [ 82.290232] [ INFO: possible circular locking dependency detected ]
> > [ 82.290235] 4.1.0-rc3-tb1 #12 Not tainted
> > [ 82.290236] -------------------------------------------------------
> > [ 82.290238] qv4l2/1262 is trying to acquire lock:
> > [ 82.290240] (&mm->mmap_sem){++++++}, at: [<ffffffffa007a870>] vb2_dma_sg_put_userptr+0xf0/0x170 [videobuf2_dma_sg]
> > [ 82.290247]
> > but task is already holding lock:
> > [ 82.290249] (&q->mmap_lock){+.+.+.}, at: [<ffffffffa006a9ec>] __reqbufs.isra.13+0x7c/0x410 [videobuf2_core]
> > [ 82.290255]
> > which lock already depends on the new lock.
>
> Thanks for the report! So I finally got to this after returning from
> vacation and dealing with more urgent stuff. Looking more at the code it
> seems to me that this particular code path going through
> vb2_ioctl_reqbufs() didn't have previously any mmap_sem protection. Thus we
> could call vma->vm_ops->close() from vb2_put_vma() without holding mmap_sem
> which is against the locking protocol.
>
> My patch unintentionally fixed this but that introduced the lock inversion
> lockdep complains about. Now the full series removes the need for getting
> VMA reference in videobuf2 code so we don't need mmap_sem in put_userptr
> callbacks at all. So when the whole series is applied we are fine again.
>
> So how shall we proceed? Just slap this patch at the beginning of the
> series to make sure it gets merged at once so the lock inversion isn't
> there for long? Or alternatively I can just remove mmap_sem from
> put_userptr() in this series. That will somewhat increase the amount of
> calls to vma->vm_ops->close() without mmap_sem util the whole series is
> applied. Any opinions guys?
>
> Honza
> > [ 82.290257]
> > the existing dependency chain (in reverse order) is:
> > [ 82.290259]
> > -> #1 (&q->mmap_lock){+.+.+.}:
> > [ 82.290262] [<ffffffff8110bab9>] lock_acquire+0xc9/0x290
> > [ 82.290267] [<ffffffff81a9fc1e>] mutex_lock_nested+0x4e/0x3f0
> > [ 82.290270] [<ffffffffa00654a2>] vb2_mmap+0x232/0x350 [videobuf2_core]
> > [ 82.290273] [<ffffffffa0067ab5>] vb2_fop_mmap+0x25/0x30 [videobuf2_core]
> > [ 82.290276] [<ffffffffa002d11a>] v4l2_mmap+0x5a/0x90 [videodev]
> > [ 82.290281] [<ffffffff811f4bcb>] mmap_region+0x3bb/0x5f0
> > [ 82.290285] [<ffffffff811f511f>] do_mmap_pgoff+0x31f/0x400
> > [ 82.290288] [<ffffffff811dfbe0>] vm_mmap_pgoff+0x90/0xc0
> > [ 82.290291] [<ffffffff811f35af>] SyS_mmap_pgoff+0x1df/0x290
> > [ 82.290294] [<ffffffff8105ef42>] SyS_mmap+0x22/0x30
> > [ 82.290297] [<ffffffff81aa4517>] system_call_fastpath+0x12/0x6f
> > [ 82.290300]
> > -> #0 (&mm->mmap_sem){++++++}:
> > [ 82.290303] [<ffffffff8110adb3>] __lock_acquire+0x1d53/0x1fe0
> > [ 82.290306] [<ffffffff8110bab9>] lock_acquire+0xc9/0x290
> > [ 82.290308] [<ffffffff81aa1924>] down_read+0x34/0x50
> > [ 82.290311] [<ffffffffa007a870>] vb2_dma_sg_put_userptr+0xf0/0x170 [videobuf2_dma_sg]
> > [ 82.290314] [<ffffffffa0066656>] __vb2_queue_free+0x156/0x5f0 [videobuf2_core]
> > [ 82.290317] [<ffffffffa006aa0f>] __reqbufs.isra.13+0x9f/0x410 [videobuf2_core]
> > [ 82.290320] [<ffffffffa006b084>] vb2_ioctl_reqbufs+0x74/0xb0 [videobuf2_core]
> > [ 82.290323] [<ffffffffa0033d83>] v4l_reqbufs+0x43/0x50 [videodev]
> > [ 82.290327] [<ffffffffa00329f4>] __video_do_ioctl+0x274/0x310 [videodev]
> > [ 82.290331] [<ffffffffa00349a8>] video_usercopy+0x378/0x8f0 [videodev]
> > [ 82.290336] [<ffffffffa0034f35>] video_ioctl2+0x15/0x20 [videodev]
> > [ 82.290340] [<ffffffffa002d6d0>] v4l2_ioctl+0xd0/0xf0 [videodev]
> > [ 82.290343] [<ffffffff81238258>] do_vfs_ioctl+0x308/0x540
> > [ 82.290347] [<ffffffff81238511>] SyS_ioctl+0x81/0xa0
> > [ 82.290349] [<ffffffff81aa4517>] system_call_fastpath+0x12/0x6f
> > [ 82.290352]
> > other info that might help us debug this:
> >
> > [ 82.290354] Possible unsafe locking scenario:
> >
> > [ 82.290356] CPU0 CPU1
> > [ 82.290357] ---- ----
> > [ 82.290358] lock(&q->mmap_lock);
> > [ 82.290360] lock(&mm->mmap_sem);
> > [ 82.290362] lock(&q->mmap_lock);
> > [ 82.290365] lock(&mm->mmap_sem);
> > [ 82.290367]
> > *** DEADLOCK ***
> >
> > [ 82.290369] 2 locks held by qv4l2/1262:
> > [ 82.290370] #0: (&s->lock){+.+.+.}, at: [<ffffffffa002d65f>] v4l2_ioctl+0x5f/0xf0 [videodev]
> > [ 82.290376] #1: (&q->mmap_lock){+.+.+.}, at: [<ffffffffa006a9ec>] __reqbufs.isra.13+0x7c/0x410 [videobuf2_core]
> > [ 82.290382]
> > stack backtrace:
> > [ 82.290385] CPU: 1 PID: 1262 Comm: qv4l2 Not tainted 4.1.0-rc3-tb1 #12
> > [ 82.290387] Hardware name: /DH67CF, BIOS BLH6710H.86A.0105.2011.0301.1654 03/01/2011
> > [ 82.290388] ffffffff82c46890 ffff8800b4bfb968 ffffffff81a98687 0000000000000007
> > [ 82.290392] ffffffff82c46890 ffff8800b4bfb9b8 ffffffff8110785d 0000000000000000
> > [ 82.290395] ffff8800b4bfba28 0000000000000001 ffff8800d51ce718 0000000000000001
> > [ 82.290399] Call Trace:
> > [ 82.290402] [<ffffffff81a98687>] dump_stack+0x4f/0x7b
> > [ 82.290405] [<ffffffff8110785d>] print_circular_bug+0x1cd/0x230
> > [ 82.290407] [<ffffffff8110adb3>] __lock_acquire+0x1d53/0x1fe0
> > [ 82.290411] [<ffffffff812142b9>] ? kfree+0x169/0x570
> > [ 82.290414] [<ffffffff8110bab9>] lock_acquire+0xc9/0x290
> > [ 82.290416] [<ffffffffa007a870>] ? vb2_dma_sg_put_userptr+0xf0/0x170 [videobuf2_dma_sg]
> > [ 82.290419] [<ffffffff81aa1924>] down_read+0x34/0x50
> > [ 82.290421] [<ffffffffa007a870>] ? vb2_dma_sg_put_userptr+0xf0/0x170 [videobuf2_dma_sg]
> > [ 82.290424] [<ffffffffa007a870>] vb2_dma_sg_put_userptr+0xf0/0x170 [videobuf2_dma_sg]
> > [ 82.290427] [<ffffffffa0066656>] __vb2_queue_free+0x156/0x5f0 [videobuf2_core]
> > [ 82.290430] [<ffffffffa006aa0f>] __reqbufs.isra.13+0x9f/0x410 [videobuf2_core]
> > [ 82.290434] [<ffffffff811c8a59>] ? free_hot_cold_page+0x159/0x200
> > [ 82.290437] [<ffffffffa006b084>] vb2_ioctl_reqbufs+0x74/0xb0 [videobuf2_core]
> > [ 82.290441] [<ffffffffa0033d83>] v4l_reqbufs+0x43/0x50 [videodev]
> > [ 82.290445] [<ffffffffa00329f4>] __video_do_ioctl+0x274/0x310 [videodev]
> > [ 82.290449] [<ffffffffa0032780>] ? v4l_querycap+0x70/0x70 [videodev]
> > [ 82.290453] [<ffffffffa00349a8>] video_usercopy+0x378/0x8f0 [videodev]
> > [ 82.290456] [<ffffffff81108b11>] ? mark_held_locks+0x71/0xa0
> > [ 82.290458] [<ffffffff81108d4d>] ? trace_hardirqs_on+0xd/0x10
> > [ 82.290461] [<ffffffff81a9f98e>] ? mutex_lock_interruptible_nested+0x25e/0x4a0
> > [ 82.290464] [<ffffffffa002d65f>] ? v4l2_ioctl+0x5f/0xf0 [videodev]
> > [ 82.290468] [<ffffffffa002d65f>] ? v4l2_ioctl+0x5f/0xf0 [videodev]
> > [ 82.290472] [<ffffffffa0034f35>] video_ioctl2+0x15/0x20 [videodev]
> > [ 82.290475] [<ffffffffa002d6d0>] v4l2_ioctl+0xd0/0xf0 [videodev]
> > [ 82.290478] [<ffffffff81238258>] do_vfs_ioctl+0x308/0x540
> > [ 82.290481] [<ffffffff8124476c>] ? __fget_light+0x6c/0xa0
> > [ 82.290484] [<ffffffff81238511>] SyS_ioctl+0x81/0xa0
> > [ 82.290487] [<ffffffff81aa4517>] system_call_fastpath+0x12/0x6f
> >
> > The problem is that the mmap_sem is now taken in vb2_dma_sg_put_userptr() when
> > that didn't happen before. This is fine when called from __qbuf_userptr, but
> > not when called from __vb2_queue_free. I will see if I have time to dig into
> > this during the weekend and solve it, but if not, then this has to be reverted
> > and the get_vaddr_frames() patch series postponed since it depends on this one.
> >
> > Regards,
> >
> > Hans
> >
> > On 05/01/15 12:17, Mauro Carvalho Chehab wrote:
> > > This is an automatic generated email to let you know that the following patch were queued at the
> > > http://git.linuxtv.org/cgit.cgi/media_tree.git tree:
> > >
> > > Subject: [media] vb2: Push mmap_sem down to memops
> > > Author: Jan Kara <jack@suse.cz>
> > > Date: Tue Mar 17 08:56:31 2015 -0300
> > >
> > > Currently vb2 core acquires mmap_sem just around call to
> > > __qbuf_userptr(). However since commit f035eb4e976ef5 (videobuf2: fix
> > > lockdep warning) it isn't necessary to acquire it so early as we no
> > > longer have to drop queue mutex before acquiring mmap_sem. So push
> > > acquisition of mmap_sem down into .get_userptr and .put_userptr memops
> > > so that the semaphore is acquired for a shorter time and it is clearer
> > > what it is needed for.
> > >
> > > Signed-off-by: Jan Kara <jack@suse.cz>
> > > Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> > > Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
> > >
> > > drivers/media/v4l2-core/videobuf2-core.c | 2 --
> > > drivers/media/v4l2-core/videobuf2-dma-contig.c | 7 +++++++
> > > drivers/media/v4l2-core/videobuf2-dma-sg.c | 6 ++++++
> > > drivers/media/v4l2-core/videobuf2-vmalloc.c | 6 +++++-
> > > 4 files changed, 18 insertions(+), 3 deletions(-)
> > >
> > > ---
> > >
> > > http://git.linuxtv.org/cgit.cgi/media_tree.git/commit/?id=48b25a3a713b90988b6882d318f7c0a6bed9aabc
> > >
> > > diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
> > > index 66ada01..20cdbc0 100644
> > > --- a/drivers/media/v4l2-core/videobuf2-core.c
> > > +++ b/drivers/media/v4l2-core/videobuf2-core.c
> > > @@ -1657,9 +1657,7 @@ static int __buf_prepare(struct vb2_buffer *vb, const struct v4l2_buffer *b)
> > > ret = __qbuf_mmap(vb, b);
> > > break;
> > > case V4L2_MEMORY_USERPTR:
> > > - down_read(¤t->mm->mmap_sem);
> > > ret = __qbuf_userptr(vb, b);
> > > - up_read(¤t->mm->mmap_sem);
> > > break;
> > > case V4L2_MEMORY_DMABUF:
> > > ret = __qbuf_dmabuf(vb, b);
> > > diff --git a/drivers/media/v4l2-core/videobuf2-dma-contig.c b/drivers/media/v4l2-core/videobuf2-dma-contig.c
> > > index 644dec7..620c4aa 100644
> > > --- a/drivers/media/v4l2-core/videobuf2-dma-contig.c
> > > +++ b/drivers/media/v4l2-core/videobuf2-dma-contig.c
> > > @@ -532,7 +532,9 @@ static void vb2_dc_put_userptr(void *buf_priv)
> > > sg_free_table(sgt);
> > > kfree(sgt);
> > > }
> > > + down_read(¤t->mm->mmap_sem);
> > > vb2_put_vma(buf->vma);
> > > + up_read(¤t->mm->mmap_sem);
> > > kfree(buf);
> > > }
> > >
> > > @@ -616,6 +618,7 @@ static void *vb2_dc_get_userptr(void *alloc_ctx, unsigned long vaddr,
> > > goto fail_buf;
> > > }
> > >
> > > + down_read(¤t->mm->mmap_sem);
> > > /* current->mm->mmap_sem is taken by videobuf2 core */
> > > vma = find_vma(current->mm, vaddr);
> > > if (!vma) {
> > > @@ -642,6 +645,7 @@ static void *vb2_dc_get_userptr(void *alloc_ctx, unsigned long vaddr,
> > > if (ret) {
> > > unsigned long pfn;
> > > if (vb2_dc_get_user_pfn(start, n_pages, vma, &pfn) == 0) {
> > > + up_read(¤t->mm->mmap_sem);
> > > buf->dma_addr = vb2_dc_pfn_to_dma(buf->dev, pfn);
> > > buf->size = size;
> > > kfree(pages);
> > > @@ -651,6 +655,7 @@ static void *vb2_dc_get_userptr(void *alloc_ctx, unsigned long vaddr,
> > > pr_err("failed to get user pages\n");
> > > goto fail_vma;
> > > }
> > > + up_read(¤t->mm->mmap_sem);
> > >
> > > sgt = kzalloc(sizeof(*sgt), GFP_KERNEL);
> > > if (!sgt) {
> > > @@ -713,10 +718,12 @@ fail_get_user_pages:
> > > while (n_pages)
> > > put_page(pages[--n_pages]);
> > >
> > > + down_read(¤t->mm->mmap_sem);
> > > fail_vma:
> > > vb2_put_vma(buf->vma);
> > >
> > > fail_pages:
> > > + up_read(¤t->mm->mmap_sem);
> > > kfree(pages); /* kfree is NULL-proof */
> > >
> > > fail_buf:
> > > diff --git a/drivers/media/v4l2-core/videobuf2-dma-sg.c b/drivers/media/v4l2-core/videobuf2-dma-sg.c
> > > index 45c708e..afd4b51 100644
> > > --- a/drivers/media/v4l2-core/videobuf2-dma-sg.c
> > > +++ b/drivers/media/v4l2-core/videobuf2-dma-sg.c
> > > @@ -263,6 +263,7 @@ static void *vb2_dma_sg_get_userptr(void *alloc_ctx, unsigned long vaddr,
> > > if (!buf->pages)
> > > goto userptr_fail_alloc_pages;
> > >
> > > + down_read(¤t->mm->mmap_sem);
> > > vma = find_vma(current->mm, vaddr);
> > > if (!vma) {
> > > dprintk(1, "no vma for address %lu\n", vaddr);
> > > @@ -301,6 +302,7 @@ static void *vb2_dma_sg_get_userptr(void *alloc_ctx, unsigned long vaddr,
> > > 1, /* force */
> > > buf->pages,
> > > NULL);
> > > + up_read(¤t->mm->mmap_sem);
> > >
> > > if (num_pages_from_user != buf->num_pages)
> > > goto userptr_fail_get_user_pages;
> > > @@ -328,8 +330,10 @@ userptr_fail_get_user_pages:
> > > if (!vma_is_io(buf->vma))
> > > while (--num_pages_from_user >= 0)
> > > put_page(buf->pages[num_pages_from_user]);
> > > + down_read(¤t->mm->mmap_sem);
> > > vb2_put_vma(buf->vma);
> > > userptr_fail_find_vma:
> > > + up_read(¤t->mm->mmap_sem);
> > > kfree(buf->pages);
> > > userptr_fail_alloc_pages:
> > > kfree(buf);
> > > @@ -362,7 +366,9 @@ static void vb2_dma_sg_put_userptr(void *buf_priv)
> > > put_page(buf->pages[i]);
> > > }
> > > kfree(buf->pages);
> > > + down_read(¤t->mm->mmap_sem);
> > > vb2_put_vma(buf->vma);
> > > + up_read(¤t->mm->mmap_sem);
> > > kfree(buf);
> > > }
> > >
> > > diff --git a/drivers/media/v4l2-core/videobuf2-vmalloc.c b/drivers/media/v4l2-core/videobuf2-vmalloc.c
> > > index 657ab30..0ba40be 100644
> > > --- a/drivers/media/v4l2-core/videobuf2-vmalloc.c
> > > +++ b/drivers/media/v4l2-core/videobuf2-vmalloc.c
> > > @@ -89,7 +89,7 @@ static void *vb2_vmalloc_get_userptr(void *alloc_ctx, unsigned long vaddr,
> > > offset = vaddr & ~PAGE_MASK;
> > > buf->size = size;
> > >
> > > -
> > > + down_read(¤t->mm->mmap_sem);
> > > vma = find_vma(current->mm, vaddr);
> > > if (vma && (vma->vm_flags & VM_PFNMAP) && (vma->vm_pgoff)) {
> > > if (vb2_get_contig_userptr(vaddr, size, &vma, &physp))
> > > @@ -121,6 +121,7 @@ static void *vb2_vmalloc_get_userptr(void *alloc_ctx, unsigned long vaddr,
> > > if (!buf->vaddr)
> > > goto fail_get_user_pages;
> > > }
> > > + up_read(¤t->mm->mmap_sem);
> > >
> > > buf->vaddr += offset;
> > > return buf;
> > > @@ -133,6 +134,7 @@ fail_get_user_pages:
> > > kfree(buf->pages);
> > >
> > > fail_pages_array_alloc:
> > > + up_read(¤t->mm->mmap_sem);
> > > kfree(buf);
> > >
> > > return NULL;
> > > @@ -144,6 +146,7 @@ static void vb2_vmalloc_put_userptr(void *buf_priv)
> > > unsigned long vaddr = (unsigned long)buf->vaddr & PAGE_MASK;
> > > unsigned int i;
> > >
> > > + down_read(¤t->mm->mmap_sem);
> > > if (buf->pages) {
> > > if (vaddr)
> > > vm_unmap_ram((void *)vaddr, buf->n_pages);
> > > @@ -157,6 +160,7 @@ static void vb2_vmalloc_put_userptr(void *buf_priv)
> > > vb2_put_vma(buf->vma);
> > > iounmap((__force void __iomem *)buf->vaddr);
> > > }
> > > + up_read(¤t->mm->mmap_sem);
> > > kfree(buf);
> > > }
> > >
> > >
> > > _______________________________________________
> > > linuxtv-commits mailing list
> > > linuxtv-commits@linuxtv.org
> > > http://www.linuxtv.org/cgi-bin/mailman/listinfo/linuxtv-commits
> > >
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2015-06-18 12:44 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <E1Yo8Jc-0000K9-Sd@www.linuxtv.org>
2015-06-11 8:52 ` [git:media_tree/master] [media] vb2: Push mmap_sem down to memops Hans Verkuil
2015-06-18 10:12 ` Jan Kara
2015-06-18 12:44 ` Mauro Carvalho Chehab
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).