| Message ID | 1478205074-8956-1-git-send-email-robdclark@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Nov 03, 2016 at 04:31:14PM -0400, Rob Clark wrote: > Currently with fence-array, we have a potential deadlock situation. If we > fence_add_callback() on an array-fence, the array-fence's lock is aquired > first, and in it's ->enable_signaling() callback, it will install cb's on > it's array-member fences, so the array-member's lock is acquired second. > > But in the signal path, the array-member's lock is acquired first, and the > array-fence's lock acquired second. Could mention that this is only an issue if the same fence->lock appeared more than once in the array. Which is possible. > diff --git a/drivers/dma-buf/dma-fence-array.c b/drivers/dma-buf/dma-fence-array.c > index 67eb7c8..274bbb5 100644 > --- a/drivers/dma-buf/dma-fence-array.c > +++ b/drivers/dma-buf/dma-fence-array.c > @@ -43,9 +43,10 @@ static void dma_fence_array_cb_func(struct dma_fence *f, > dma_fence_put(&array->base); > } > > -static bool dma_fence_array_enable_signaling(struct dma_fence *fence) > +static void enable_signaling_worker(struct work_struct *w) > { > - struct dma_fence_array *array = to_dma_fence_array(fence); > + struct dma_fence_array *array = > + container_of(w, struct dma_fence_array, enable_signaling_worker); > struct dma_fence_array_cb *cb = (void *)(&array[1]); > unsigned i; > > @@ -63,11 +64,18 @@ static bool dma_fence_array_enable_signaling(struct dma_fence *fence) > if (dma_fence_add_callback(array->fences[i], &cb[i].cb, > dma_fence_array_cb_func)) { > dma_fence_put(&array->base); > - if (atomic_dec_and_test(&array->num_pending)) > - return false; > + if (atomic_dec_and_test(&array->num_pending)) { > + dma_fence_signal(&array->base); > + return; > + } > } > } > +} > > +static bool dma_fence_array_enable_signaling(struct dma_fence *fence) > +{ > + struct dma_fence_array *array = to_dma_fence_array(fence); > + queue_work(system_unbound_wq, &array->enable_signaling_worker); I think you need a dma_fence_get(fence) here with a corresponding put in the worker. Sadly I still can't poke a hole in the lockdep warning. What I might suggest is that we try static bool is_signaled(struct dma_fence *fence) { if (test_bit(SIGNALED_BIT, &fence->flags)) return true; return fence->ops->signaled && fence->ops->signaled(fence); } static bool dma_fence_array_enable_signaling(struct dma_fence *fence) { struct dma_fence_array *array = to_dma_fence_array(fence); int num_pending = atomic_read(&array->num_pending); int i; for (i = 0; i < array->num_fences; i++) if (is_signaled(array->fences[i]) && !--num_pending) { atomic_set(&array->num_pending, 0); return false; } dma_fence_get(&array->base); queue_work(system_unbound_wq, &array->enable_signaling_worker); } -Chris
On Thu, Nov 3, 2016 at 5:41 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote: > On Thu, Nov 03, 2016 at 04:31:14PM -0400, Rob Clark wrote: >> Currently with fence-array, we have a potential deadlock situation. If we >> fence_add_callback() on an array-fence, the array-fence's lock is aquired >> first, and in it's ->enable_signaling() callback, it will install cb's on >> it's array-member fences, so the array-member's lock is acquired second. >> >> But in the signal path, the array-member's lock is acquired first, and the >> array-fence's lock acquired second. > > Could mention that this is only an issue if the same fence->lock > appeared more than once in the array. Which is possible. hmm, lockdep will care more about lock classes than lock instances.. >> diff --git a/drivers/dma-buf/dma-fence-array.c b/drivers/dma-buf/dma-fence-array.c >> index 67eb7c8..274bbb5 100644 >> --- a/drivers/dma-buf/dma-fence-array.c >> +++ b/drivers/dma-buf/dma-fence-array.c >> @@ -43,9 +43,10 @@ static void dma_fence_array_cb_func(struct dma_fence *f, >> dma_fence_put(&array->base); >> } >> >> -static bool dma_fence_array_enable_signaling(struct dma_fence *fence) >> +static void enable_signaling_worker(struct work_struct *w) >> { >> - struct dma_fence_array *array = to_dma_fence_array(fence); >> + struct dma_fence_array *array = >> + container_of(w, struct dma_fence_array, enable_signaling_worker); >> struct dma_fence_array_cb *cb = (void *)(&array[1]); >> unsigned i; >> >> @@ -63,11 +64,18 @@ static bool dma_fence_array_enable_signaling(struct dma_fence *fence) >> if (dma_fence_add_callback(array->fences[i], &cb[i].cb, >> dma_fence_array_cb_func)) { >> dma_fence_put(&array->base); >> - if (atomic_dec_and_test(&array->num_pending)) >> - return false; >> + if (atomic_dec_and_test(&array->num_pending)) { >> + dma_fence_signal(&array->base); >> + return; >> + } >> } >> } >> +} >> >> +static bool dma_fence_array_enable_signaling(struct dma_fence *fence) >> +{ >> + struct dma_fence_array *array = to_dma_fence_array(fence); >> + queue_work(system_unbound_wq, &array->enable_signaling_worker); > > I think you need a dma_fence_get(fence) here with a corresponding put in > the worker. Sadly I still can't poke a hole in the lockdep warning. oh, right, extra get/put makes sense.. fwiw it should be easy enough to trigger with some code that merges a sw-sync fence w/ other fences.. > What I might suggest is that we try > > static bool is_signaled(struct dma_fence *fence) > { > if (test_bit(SIGNALED_BIT, &fence->flags)) > return true; > > return fence->ops->signaled && fence->ops->signaled(fence); > } > > static bool dma_fence_array_enable_signaling(struct dma_fence *fence) > { > struct dma_fence_array *array = to_dma_fence_array(fence); > int num_pending = atomic_read(&array->num_pending); > int i; > > for (i = 0; i < array->num_fences; i++) > if (is_signaled(array->fences[i]) && !--num_pending) { > atomic_set(&array->num_pending, 0); > return false; > } > > dma_fence_get(&array->base); > queue_work(system_unbound_wq, &array->enable_signaling_worker); > } hmm, I guess just to try to avoid the wq? BR, -R > -Chris > > -- > Chris Wilson, Intel Open Source Technology Centre
On Thu, Nov 03, 2016 at 07:34:02PM -0400, Rob Clark wrote: > On Thu, Nov 3, 2016 at 5:41 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote: > > static bool dma_fence_array_enable_signaling(struct dma_fence *fence) > > { > > struct dma_fence_array *array = to_dma_fence_array(fence); > > int num_pending = atomic_read(&array->num_pending); > > int i; > > > > for (i = 0; i < array->num_fences; i++) > > if (is_signaled(array->fences[i]) && !--num_pending) { > > atomic_set(&array->num_pending, 0); > > return false; > > } > > > > dma_fence_get(&array->base); > > queue_work(system_unbound_wq, &array->enable_signaling_worker); > > } > > hmm, I guess just to try to avoid the wq? Yeah, not all fences are capable of reporting the current status, and some others may only report signaled() after enable_signaling, but for i915/nouveau even msm can report the current status without the extra step. For those it seems worth skipping the wq. -Chris
diff --git a/drivers/dma-buf/dma-fence-array.c b/drivers/dma-buf/dma-fence-array.c index 67eb7c8..274bbb5 100644 --- a/drivers/dma-buf/dma-fence-array.c +++ b/drivers/dma-buf/dma-fence-array.c @@ -43,9 +43,10 @@ static void dma_fence_array_cb_func(struct dma_fence *f, dma_fence_put(&array->base); } -static bool dma_fence_array_enable_signaling(struct dma_fence *fence) +static void enable_signaling_worker(struct work_struct *w) { - struct dma_fence_array *array = to_dma_fence_array(fence); + struct dma_fence_array *array = + container_of(w, struct dma_fence_array, enable_signaling_worker); struct dma_fence_array_cb *cb = (void *)(&array[1]); unsigned i; @@ -63,11 +64,18 @@ static bool dma_fence_array_enable_signaling(struct dma_fence *fence) if (dma_fence_add_callback(array->fences[i], &cb[i].cb, dma_fence_array_cb_func)) { dma_fence_put(&array->base); - if (atomic_dec_and_test(&array->num_pending)) - return false; + if (atomic_dec_and_test(&array->num_pending)) { + dma_fence_signal(&array->base); + return; + } } } +} +static bool dma_fence_array_enable_signaling(struct dma_fence *fence) +{ + struct dma_fence_array *array = to_dma_fence_array(fence); + queue_work(system_unbound_wq, &array->enable_signaling_worker); return true; } @@ -141,6 +149,8 @@ struct dma_fence_array *dma_fence_array_create(int num_fences, atomic_set(&array->num_pending, signal_on_any ? 1 : num_fences); array->fences = fences; + INIT_WORK(&array->enable_signaling_worker, enable_signaling_worker); + return array; } EXPORT_SYMBOL(dma_fence_array_create); diff --git a/include/linux/dma-fence-array.h b/include/linux/dma-fence-array.h index 5900945..f48e8f4 100644 --- a/include/linux/dma-fence-array.h +++ b/include/linux/dma-fence-array.h @@ -35,6 +35,8 @@ struct dma_fence_array_cb { /** * struct dma_fence_array - fence to represent an array of fences * @base: fence base class + * @enable_signaling_worker: &work_struct for deferring enable_signaling + * to context not holding fence->lock * @lock: spinlock for fence handling * @num_fences: number of fences in the array * @num_pending: fences in the array still pending @@ -43,6 +45,8 @@ struct dma_fence_array_cb { struct dma_fence_array { struct dma_fence base; + struct work_struct enable_signaling_worker; + spinlock_t lock; unsigned num_fences; atomic_t num_pending;
Currently with fence-array, we have a potential deadlock situation. If we fence_add_callback() on an array-fence, the array-fence's lock is aquired first, and in it's ->enable_signaling() callback, it will install cb's on it's array-member fences, so the array-member's lock is acquired second. But in the signal path, the array-member's lock is acquired first, and the array-fence's lock acquired second. To solve that, punt adding the callbacks on the array member fences to a worker. lockdep splat: ====================================================== [ INFO: possible circular locking dependency detected ] 4.7.0-rc7+ #489 Not tainted ------------------------------------------------------- surfaceflinger/2034 is trying to acquire lock: (&(&array->lock)->rlock){......}, at: [<ffff00000858cddc>] fence_signal+0x5c/0xf8 but task is already holding lock: (&(&obj->child_list_lock)->rlock){......}, at: [<ffff0000085902f8>] sw_sync_ioctl+0x228/0x3b0 which lock already depends on the new lock. the existing dependency chain (in reverse order) is: -> #1 (&(&obj->child_list_lock)->rlock){......}: [<ffff000008108924>] __lock_acquire+0x173c/0x18d8 [<ffff000008108e0c>] lock_acquire+0x4c/0x68 [<ffff000008ac6a6c>] _raw_spin_lock_irqsave+0x54/0x70 [<ffff00000858d05c>] fence_add_callback+0x3c/0x100 [<ffff00000858f100>] fence_array_enable_signaling+0x80/0x170 [<ffff00000858d0d8>] fence_add_callback+0xb8/0x100 [<ffff00000858f504>] sync_file_poll+0xd4/0xf0 [<ffff0000081fd3a0>] do_sys_poll+0x220/0x438 [<ffff0000081fd8d0>] SyS_ppoll+0x1b0/0x1d8 [<ffff000008084f30>] el0_svc_naked+0x24/0x28 -> #0 (&(&array->lock)->rlock){......}: [<ffff000008104768>] print_circular_bug+0x80/0x2e0 [<ffff0000081089ac>] __lock_acquire+0x17c4/0x18d8 [<ffff000008108e0c>] lock_acquire+0x4c/0x68 [<ffff000008ac6a6c>] _raw_spin_lock_irqsave+0x54/0x70 [<ffff00000858cddc>] fence_signal+0x5c/0xf8 [<ffff00000858f268>] fence_array_cb_func+0x78/0x88 [<ffff00000858cb28>] fence_signal_locked+0x80/0xe0 [<ffff0000085903c8>] sw_sync_ioctl+0x2f8/0x3b0 [<ffff0000081faf6c>] do_vfs_ioctl+0xa4/0x790 [<ffff0000081fb6e4>] SyS_ioctl+0x8c/0xa0 [<ffff000008084f30>] el0_svc_naked+0x24/0x28 other info that might help us debug this: Possible unsafe locking scenario: CPU0 CPU1 ---- ---- lock(&(&obj->child_list_lock)->rlock); lock(&(&array->lock)->rlock); lock(&(&obj->child_list_lock)->rlock); lock(&(&array->lock)->rlock); *** DEADLOCK *** 1 lock held by surfaceflinger/2034: #0: (&(&obj->child_list_lock)->rlock){......}, at: [<ffff0000085902f8>] sw_sync_ioctl+0x228/0x3b0 Signed-off-by: Rob Clark <robdclark@gmail.com> --- drivers/dma-buf/dma-fence-array.c | 18 ++++++++++++++---- include/linux/dma-fence-array.h | 4 ++++ 2 files changed, 18 insertions(+), 4 deletions(-)