Re: [git:media_tree/master] [media] vb2: Push mmap_sem down to memops
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 j...@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: [a007a870]
vb2_dma_sg_put_userptr+0xf0/0x170 [videobuf2_dma_sg]
[ 82.290247]
but task is already holding lock:
[ 82.290249] (q-mmap_lock){+.+.+.}, at: [a006a9ec]
__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][8110bab9] lock_acquire+0xc9/0x290
[ 82.290267][81a9fc1e] mutex_lock_nested+0x4e/0x3f0
[ 82.290270][a00654a2] vb2_mmap+0x232/0x350
[videobuf2_core]
[ 82.290273][a0067ab5] vb2_fop_mmap+0x25/0x30
[videobuf2_core]
[ 82.290276][a002d11a] v4l2_mmap+0x5a/0x90 [videodev]
[ 82.290281][811f4bcb] mmap_region+0x3bb/0x5f0
[ 82.290285][811f511f] do_mmap_pgoff+0x31f/0x400
[ 82.290288][811dfbe0] vm_mmap_pgoff+0x90/0xc0
[ 82.290291][811f35af] SyS_mmap_pgoff+0x1df/0x290
[ 82.290294][8105ef42] SyS_mmap+0x22/0x30
[ 82.290297][81aa4517] system_call_fastpath+0x12/0x6f
[ 82.290300]
- #0 (mm-mmap_sem){++}:
[ 82.290303][8110adb3] __lock_acquire+0x1d53/0x1fe0
[ 82.290306][8110bab9] lock_acquire+0xc9/0x290
[ 82.290308][81aa1924] down_read+0x34/0x50
[ 82.290311][a007a870]
vb2_dma_sg_put_userptr+0xf0/0x170 [videobuf2_dma_sg]
[ 82.290314][a0066656] __vb2_queue_free+0x156/0x5f0
[videobuf2_core]
[ 82.290317][a006aa0f] __reqbufs.isra.13+0x9f/0x410
[videobuf2_core]
[ 82.290320][a006b084] vb2_ioctl_reqbufs+0x74/0xb0
[videobuf2_core]
[ 82.290323][a0033d83] v4l_reqbufs+0x43/0x50 [videodev]
[ 82.290327][a00329f4] __video_do_ioctl+0x274/0x310
[videodev]
[ 82.290331][a00349a8] video_usercopy+0x378/0x8f0
[videodev]
[ 82.290336][a0034f35] video_ioctl2+0x15/0x20 [videodev]
[ 82.290340][a002d6d0] v4l2_ioctl+0xd0/0xf0 [videodev]
[ 82.290343][81238258] do_vfs_ioctl+0x308/0x540
[ 82.290347][81238511] SyS_ioctl+0x81/0xa0
[ 82.290349][81aa4517] system_call_fastpath+0x12/0x6f
[ 82.290352]
other info that might help us debug this:
[ 82.290354] Possible unsafe locking scenario:
[ 82.290356]CPU0CPU1
[ 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: [a002d65f]
v4l2_ioctl+0x5f/0xf0 [videodev]
[
Re: [git:media_tree/master] [media] vb2: Push mmap_sem down to memops
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: [a007a870]
vb2_dma_sg_put_userptr+0xf0/0x170 [videobuf2_dma_sg]
[ 82.290247]
but task is already holding lock:
[ 82.290249] (q-mmap_lock){+.+.+.}, at: [a006a9ec]
__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][8110bab9] lock_acquire+0xc9/0x290
[ 82.290267][81a9fc1e] mutex_lock_nested+0x4e/0x3f0
[ 82.290270][a00654a2] vb2_mmap+0x232/0x350
[videobuf2_core]
[ 82.290273][a0067ab5] vb2_fop_mmap+0x25/0x30
[videobuf2_core]
[ 82.290276][a002d11a] v4l2_mmap+0x5a/0x90 [videodev]
[ 82.290281][811f4bcb] mmap_region+0x3bb/0x5f0
[ 82.290285][811f511f] do_mmap_pgoff+0x31f/0x400
[ 82.290288][811dfbe0] vm_mmap_pgoff+0x90/0xc0
[ 82.290291][811f35af] SyS_mmap_pgoff+0x1df/0x290
[ 82.290294][8105ef42] SyS_mmap+0x22/0x30
[ 82.290297][81aa4517] system_call_fastpath+0x12/0x6f
[ 82.290300]
- #0 (mm-mmap_sem){++}:
[ 82.290303][8110adb3] __lock_acquire+0x1d53/0x1fe0
[ 82.290306][8110bab9] lock_acquire+0xc9/0x290
[ 82.290308][81aa1924] down_read+0x34/0x50
[ 82.290311][a007a870] vb2_dma_sg_put_userptr+0xf0/0x170
[videobuf2_dma_sg]
[ 82.290314][a0066656] __vb2_queue_free+0x156/0x5f0
[videobuf2_core]
[ 82.290317][a006aa0f] __reqbufs.isra.13+0x9f/0x410
[videobuf2_core]
[ 82.290320][a006b084] vb2_ioctl_reqbufs+0x74/0xb0
[videobuf2_core]
[ 82.290323][a0033d83] v4l_reqbufs+0x43/0x50 [videodev]
[ 82.290327][a00329f4] __video_do_ioctl+0x274/0x310
[videodev]
[ 82.290331][a00349a8] video_usercopy+0x378/0x8f0
[videodev]
[ 82.290336][a0034f35] video_ioctl2+0x15/0x20 [videodev]
[ 82.290340][a002d6d0] v4l2_ioctl+0xd0/0xf0 [videodev]
[ 82.290343][81238258] do_vfs_ioctl+0x308/0x540
[ 82.290347][81238511] SyS_ioctl+0x81/0xa0
[ 82.290349][81aa4517] system_call_fastpath+0x12/0x6f
[ 82.290352]
other info that might help us debug this:
[ 82.290354] Possible unsafe locking scenario:
[ 82.290356]CPU0CPU1
[ 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: [a002d65f]
v4l2_ioctl+0x5f/0xf0 [videodev]
[ 82.290376] #1: (q-mmap_lock){+.+.+.}, at: [a006a9ec]
__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:
Re: [git:media_tree/master] [media] vb2: Push mmap_sem down to memops
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: [a007a870]
vb2_dma_sg_put_userptr+0xf0/0x170 [videobuf2_dma_sg]
[ 82.290247]
but task is already holding lock:
[ 82.290249] (q-mmap_lock){+.+.+.}, at: [a006a9ec]
__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][8110bab9] lock_acquire+0xc9/0x290
[ 82.290267][81a9fc1e] mutex_lock_nested+0x4e/0x3f0
[ 82.290270][a00654a2] vb2_mmap+0x232/0x350 [videobuf2_core]
[ 82.290273][a0067ab5] vb2_fop_mmap+0x25/0x30
[videobuf2_core]
[ 82.290276][a002d11a] v4l2_mmap+0x5a/0x90 [videodev]
[ 82.290281][811f4bcb] mmap_region+0x3bb/0x5f0
[ 82.290285][811f511f] do_mmap_pgoff+0x31f/0x400
[ 82.290288][811dfbe0] vm_mmap_pgoff+0x90/0xc0
[ 82.290291][811f35af] SyS_mmap_pgoff+0x1df/0x290
[ 82.290294][8105ef42] SyS_mmap+0x22/0x30
[ 82.290297][81aa4517] system_call_fastpath+0x12/0x6f
[ 82.290300]
- #0 (mm-mmap_sem){++}:
[ 82.290303][8110adb3] __lock_acquire+0x1d53/0x1fe0
[ 82.290306][8110bab9] lock_acquire+0xc9/0x290
[ 82.290308][81aa1924] down_read+0x34/0x50
[ 82.290311][a007a870] vb2_dma_sg_put_userptr+0xf0/0x170
[videobuf2_dma_sg]
[ 82.290314][a0066656] __vb2_queue_free+0x156/0x5f0
[videobuf2_core]
[ 82.290317][a006aa0f] __reqbufs.isra.13+0x9f/0x410
[videobuf2_core]
[ 82.290320][a006b084] vb2_ioctl_reqbufs+0x74/0xb0
[videobuf2_core]
[ 82.290323][a0033d83] v4l_reqbufs+0x43/0x50 [videodev]
[ 82.290327][a00329f4] __video_do_ioctl+0x274/0x310
[videodev]
[ 82.290331][a00349a8] video_usercopy+0x378/0x8f0 [videodev]
[ 82.290336][a0034f35] video_ioctl2+0x15/0x20 [videodev]
[ 82.290340][a002d6d0] v4l2_ioctl+0xd0/0xf0 [videodev]
[ 82.290343][81238258] do_vfs_ioctl+0x308/0x540
[ 82.290347][81238511] SyS_ioctl+0x81/0xa0
[ 82.290349][81aa4517] system_call_fastpath+0x12/0x6f
[ 82.290352]
other info that might help us debug this:
[ 82.290354] Possible unsafe locking scenario:
[ 82.290356]CPU0CPU1
[ 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: [a002d65f]
v4l2_ioctl+0x5f/0xf0 [videodev]
[ 82.290376] #1: (q-mmap_lock){+.+.+.}, at: [a006a9ec]
__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] 82c46890 8800b4bfb968 81a98687
0007
[ 82.290392] 82c46890 8800b4bfb9b8 8110785d
[ 82.290395] 8800b4bfba28 0001 8800d51ce718
0001
[ 82.290399] Call Trace:
[ 82.290402] [81a98687] dump_stack+0x4f/0x7b
[ 82.290405] [8110785d] print_circular_bug+0x1cd/0x230
[ 82.290407] [8110adb3] __lock_acquire+0x1d53/0x1fe0
[ 82.290411] [812142b9] ? kfree+0x169/0x570
[ 82.290414] [8110bab9] lock_acquire+0xc9/0x290
[ 82.290416] [a007a870] ? vb2_dma_sg_put_userptr+0xf0/0x170
[videobuf2_dma_sg]
[ 82.290419] [81aa1924] down_read+0x34/0x50
[ 82.290421] [a007a870] ? vb2_dma_sg_put_userptr+0xf0/0x170
[videobuf2_dma_sg]
[ 82.290424] [a007a870] vb2_dma_sg_put_userptr+0xf0/0x170
[videobuf2_dma_sg]
[ 82.290427] [a0066656] __vb2_queue_free+0x156/0x5f0
[videobuf2_core]
[ 82.290430] [a006aa0f] __reqbufs.isra.13+0x9f/0x410
[videobuf2_core]
[ 82.290434] [811c8a59] ? free_hot_cold_page+0x159/0x200
[ 82.290437] [a006b084]
