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@xxxxxxx>
> 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@xxxxxxx>
> Signed-off-by: Hans Verkuil <hans.verkuil@xxxxxxxxx>
> Signed-off-by: Mauro Carvalho Chehab <mchehab@xxxxxxxxxxxxxxx>
>
> 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@xxxxxxxxxxx
> http://www.linuxtv.org/cgi-bin/mailman/listinfo/linuxtv-commits
>
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
