Re: [git:media_tree/master] [media] vb2: Push mmap_sem down to memops

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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(&current->mm->mmap_sem);
>  		ret = __qbuf_userptr(vb, b);
> -		up_read(&current->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(&current->mm->mmap_sem);
>  	vb2_put_vma(buf->vma);
> +	up_read(&current->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(&current->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(&current->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(&current->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(&current->mm->mmap_sem);
>  fail_vma:
>  	vb2_put_vma(buf->vma);
>  
>  fail_pages:
> +	up_read(&current->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(&current->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(&current->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(&current->mm->mmap_sem);
>  	vb2_put_vma(buf->vma);
>  userptr_fail_find_vma:
> +	up_read(&current->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(&current->mm->mmap_sem);
>  	vb2_put_vma(buf->vma);
> +	up_read(&current->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(&current->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(&current->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(&current->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(&current->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(&current->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




[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux