Re: [PATCH 2/8] dma-buf: fix shared fence list handling in reservation_object_copy_fences

["Christian König" <ckoenig.leichtzumerken@gmail.com>]

Am 06.08.19 um 21:06 schrieb Chris Wilson:
> Quoting Christian König (2019-08-06 16:01:28)
>> Add some helpers to correctly allocate/free reservation_object_lists.
>>
>> Otherwise we might forget to drop dma_fence references on list destruction.
>>
>> Signed-off-by: Christian König <christian.koenig@amd.com>
>> ---
>>   drivers/dma-buf/reservation.c | 65 +++++++++++++++++++++++++----------
>>   1 file changed, 46 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/dma-buf/reservation.c b/drivers/dma-buf/reservation.c
>> index d59207ca72d2..c0ba05936ab6 100644
>> --- a/drivers/dma-buf/reservation.c
>> +++ b/drivers/dma-buf/reservation.c
>> @@ -55,6 +55,47 @@ EXPORT_SYMBOL(reservation_seqcount_class);
>>   const char reservation_seqcount_string[] = "reservation_seqcount";
>>   EXPORT_SYMBOL(reservation_seqcount_string);
>>   
>> +/**
>> + * reservation_object_list_alloc - allocate fence list
>> + * @shared_max: number of fences we need space for
>> + *
>> + * Allocate a new reservation_object_list and make sure to correctly initialize
>> + * shared_max.
>> + */
>> +static struct reservation_object_list *
>> +reservation_object_list_alloc(unsigned int shared_max)
>> +{
>> +       struct reservation_object_list *list;
>> +
>> +       list = kmalloc(offsetof(typeof(*list), shared[shared_max]), GFP_KERNEL);
>> +       if (!list)
>> +               return NULL;
>> +
>> +       list->shared_max = (ksize(list) - offsetof(typeof(*list), shared)) /
>> +               sizeof(*list->shared);
>> +
>> +       return list;
>> +}
>> +
>> +/**
>> + * reservation_object_list_free - free fence list
>> + * @list: list to free
>> + *
>> + * Free a reservation_object_list and make sure to drop all references.
>> + */
>> +static void reservation_object_list_free(struct reservation_object_list *list)
>> +{
>> +       unsigned int i;
>> +
>> +       if (!list)
>> +               return;
>> +
>> +       for (i = 0; i < list->shared_count; ++i)
>> +               dma_fence_put(rcu_dereference_protected(list->shared[i], true));
>> +
>> +       kfree_rcu(list, rcu);
> So 2 out of 3 paths don't need another RCU grace period before freeing.
> Actually, that lack of RCU inside reservation_object_fini has caught me
> by surprise before. Not sure if that's worth treating as anything other
> than my own bug... But if we accept it is worth preventing here then the
> only odd one out is on a reservation_object_copy_fences() error path,
> where the extra delay shouldn't be an issue.
>
> So to double-RCU defer on reservation_object_fini() or not?

Yeah, I think in the _fini path using kfree might be legal because 
nobody else should have an extra reference to the object.

But the key point is I don't think an extra grace period would hurt us 
in any way,
Christian.

>
> For the rest of the mechanical changes,
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> -Chris

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel