[PATCH] dma-buf/fence-array: enable_signaling from wq

[Rob Clark]

On Thu, Nov 3, 2016 at 5:41 PM, Chris Wilson <chris at 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