Re: [RFC 0/5] rework fences on struct sync_file

["Christian König" <christian.koenig@amd.com>]

Am 24.06.2016 um 16:59 schrieb Gustavo Padovan:
> 2016-06-24 Christian König <deathsimple@vodafone.de>:
>
>> Am 24.06.2016 um 15:17 schrieb Gustavo Padovan:
>>> Hi Christian,
>>>
>>> 2016-06-24 Christian König <christian.koenig@amd.com>:
>>>
>>>> Am 23.06.2016 um 17:29 schrieb Gustavo Padovan:
>>>>> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
>>>>>
>>>>> Hi all,
>>>>>
>>>>> This is an attempt to improve fence support on Sync File. The basic idea
>>>>> is to have only sync_file->fence and store all fences there, either as
>>>>> normal fences or fence_arrays. That way we can remove some potential
>>>>> duplication when using fence_array with sync_file: the duplication of the array
>>>>> of fences and the duplication of fence_add_callback() for all fences.
>>>>>
>>>>> Now when creating a new sync_file during the merge process sync_file_set_fence()
>>>>> will set sync_file->fence based on the number of fences for that sync_file. If
>>>>> there is more than one fence a fence_array is created. One important advantage
>>>>> approach is that we only add one fence callback now, no matter how many fences
>>>>> there are in a sync_file - the individual callbacks are added by fence_array.
>>>>>
>>>>> Two fence ops had to be created to help abstract the difference between handling
>>>>> fences and fences_arrays: .teardown() and .get_fences(). The former run needed
>>>>> on fence_array, and the latter just return a copy of all fences in the fence.
>>>>> I'm not so sure about adding those two, speacially .get_fences(). What do you
>>>>> think?
>>>> Clearly not a good idea to add this a fence ops, cause those are specialized
>>>> functions for only a certain fence implementation (the fence_array).
>>> Are you refering only to .get_fences()?
>> That comment was only for the get_fences() operation, but the teardown()
>> callback looks very suspicious to me as well.
>>
>> Can you explain once more why that should be necessary?
> When the sync_file owner exits we need to clean up it and that means releasing
> the fence too, however with fence_array we can't just call fence_put()
> as a extra reference to array->base for each fence is held when enabling
> signalling. Thus we need a prior step, that I called teardown(), to
> remove the callback for not signaled fences and put the extra
> references.
>
> Another way to do this would be:
>
> 	if (fence_is_array(sync_file->fence))
> 		fence_array_destroy(to_fence_array(sync_file->fence));
> 	else
> 		fence_put(sync_file_fence);
>
> This would avoid the extra ops, maybe we should go this way.

NAK on both approaches. The fence array grabs another reference on 
itself for each callback it registers, so this isn't necessary:

>         for (i = 0; i < array->num_fences; ++i) {
>                 cb[i].array = array;
>                 /*
>                  * As we may report that the fence is signaled before all
>                  * callbacks are complete, we need to take an additional
>                  * reference count on the array so that we do not free 
> it too
>                  * early. The core fence handling will only hold the 
> reference
>                  * until we signal the array as complete (but that is now
>                  * insufficient).
>                  */
>                 fence_get(&array->base);
>                 if (fence_add_callback(array->fences[i], &cb[i].cb,
>                                        fence_array_cb_func)) {
>                         fence_put(&array->base);
>                         if (atomic_dec_and_test(&array->num_pending))
>                                 return false;
>                 }
>         }

So you can just use fence_remove_callback() and then fence_put() without 
worrying about the reference.

Regards,
Christian.

>
> 	Gustavo