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

From: Chris Wilson <chris@chris-wilson.co.uk>
To: "Christian König" <ckoenig.leichtzumerken@gmail.com>,
	dri-devel@lists.freedesktop.org, intel-gfx@lists.freedesktop.org,
	linaro-mm-sig@lists.linaro.org
Subject: Re: [PATCH 2/8] dma-buf: fix shared fence list handling in reservation_object_copy_fences
Date: Tue, 06 Aug 2019 20:06:07 +0100	[thread overview]
Message-ID: <156511836757.6198.137641870638609300@skylake-alporthouse-com> (raw)
In-Reply-To: <20190806150134.104222-2-christian.koenig@amd.com>

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?

For the rest of the mechanical changes,
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx