[PATCH 1/2] dma-buf: make reservation_object_copy_fences rcu save
Christian König
deathsimple at vodafone.de
Mon Sep 11 14:45:44 UTC 2017
Am 11.09.2017 um 15:56 schrieb Maarten Lankhorst:
> Op 11-09-17 om 14:53 schreef Christian König:
>> Am 10.09.2017 um 09:30 schrieb Maarten Lankhorst:
>>> Op 04-09-17 om 21:02 schreef Christian König:
>>>> From: Christian König <christian.koenig at amd.com>
>>>>
>>>> Stop requiring that the src reservation object is locked for this operation.
>>>>
>>>> Signed-off-by: Christian König <christian.koenig at amd.com>
>>>> ---
>>>> drivers/dma-buf/reservation.c | 56 ++++++++++++++++++++++++++++++++-----------
>>>> 1 file changed, 42 insertions(+), 14 deletions(-)
>>>>
>>>> diff --git a/drivers/dma-buf/reservation.c b/drivers/dma-buf/reservation.c
>>>> index dec3a81..b44d9d7 100644
>>>> --- a/drivers/dma-buf/reservation.c
>>>> +++ b/drivers/dma-buf/reservation.c
>>>> @@ -266,8 +266,7 @@ EXPORT_SYMBOL(reservation_object_add_excl_fence);
>>>> * @dst: the destination reservation object
>>>> * @src: the source reservation object
>>>> *
>>>> -* Copy all fences from src to dst. Both src->lock as well as dst-lock must be
>>>> -* held.
>>>> +* Copy all fences from src to dst. dst-lock must be held.
>>>> */
>>>> int reservation_object_copy_fences(struct reservation_object *dst,
>>>> struct reservation_object *src)
>>> Could this be implemented using reservation_object_get_fences_rcu? You're essentially duplicating its functionality.
>> I've considered this as well, but reservation_object_get_fences_rcu() returns an array and here we need an reservation_object_list.
> Doesn't seem too hard, below is the result from fiddling with the allocation function..
> reservation_object_list is just a list of fences with some junk in front.
>
> Here you go, but only tested with the compiler. O:)
> -----8<-----
> drivers/dma-buf/reservation.c | 192 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++----------------------------------------------------------------
> 1 file changed, 109 insertions(+), 83 deletions(-)
To be honest that looks rather ugly to me for not much gain.
Additional to that we loose the optimization I've stolen from the wait
function.
Regards,
Christian.
>
> diff --git a/drivers/dma-buf/reservation.c b/drivers/dma-buf/reservation.c
> index dec3a815455d..72ee91f34132 100644
> --- a/drivers/dma-buf/reservation.c
> +++ b/drivers/dma-buf/reservation.c
> @@ -261,87 +261,43 @@ void reservation_object_add_excl_fence(struct reservation_object *obj,
> }
> EXPORT_SYMBOL(reservation_object_add_excl_fence);
>
> -/**
> -* reservation_object_copy_fences - Copy all fences from src to dst.
> -* @dst: the destination reservation object
> -* @src: the source reservation object
> -*
> -* Copy all fences from src to dst. Both src->lock as well as dst-lock must be
> -* held.
> -*/
> -int reservation_object_copy_fences(struct reservation_object *dst,
> - struct reservation_object *src)
> +static bool realloc_shared(void **ptr, struct dma_fence ***pshared, unsigned shared_max, bool alloc_list, bool unlocked)
> {
> - struct reservation_object_list *src_list, *dst_list;
> - struct dma_fence *old, *new;
> - size_t size;
> - unsigned i;
> -
> - src_list = reservation_object_get_list(src);
> -
> - if (src_list) {
> - size = offsetof(typeof(*src_list),
> - shared[src_list->shared_count]);
> - dst_list = kmalloc(size, GFP_KERNEL);
> - if (!dst_list)
> - return -ENOMEM;
> -
> - dst_list->shared_count = src_list->shared_count;
> - dst_list->shared_max = src_list->shared_count;
> - for (i = 0; i < src_list->shared_count; ++i)
> - dst_list->shared[i] =
> - dma_fence_get(src_list->shared[i]);
> - } else {
> - dst_list = NULL;
> - }
> -
> - kfree(dst->staged);
> - dst->staged = NULL;
> + size_t sz;
> + void *newptr;
>
> - src_list = reservation_object_get_list(dst);
> + if (alloc_list)
> + sz = offsetof(struct reservation_object_list, shared[shared_max]);
> + else
> + sz = shared_max * sizeof(struct dma_fence *);
>
> - old = reservation_object_get_excl(dst);
> - new = reservation_object_get_excl(src);
> + newptr = krealloc(*ptr, sz, unlocked ? GFP_KERNEL : GFP_NOWAIT | __GFP_NOWARN);
> + if (!newptr)
> + return false;
>
> - dma_fence_get(new);
> + *ptr = newptr;
> + if (alloc_list) {
> + struct reservation_object_list *fobj = newptr;
>
> - preempt_disable();
> - write_seqcount_begin(&dst->seq);
> - /* write_seqcount_begin provides the necessary memory barrier */
> - RCU_INIT_POINTER(dst->fence_excl, new);
> - RCU_INIT_POINTER(dst->fence, dst_list);
> - write_seqcount_end(&dst->seq);
> - preempt_enable();
> -
> - if (src_list)
> - kfree_rcu(src_list, rcu);
> - dma_fence_put(old);
> + fobj->shared_max = shared_max;
> + *pshared = fobj->shared;
> + } else
> + *pshared = newptr;
>
> - return 0;
> + return true;
> }
> -EXPORT_SYMBOL(reservation_object_copy_fences);
>
> -/**
> - * reservation_object_get_fences_rcu - Get an object's shared and exclusive
> - * fences without update side lock held
> - * @obj: the reservation object
> - * @pfence_excl: the returned exclusive fence (or NULL)
> - * @pshared_count: the number of shared fences returned
> - * @pshared: the array of shared fence ptrs returned (array is krealloc'd to
> - * the required size, and must be freed by caller)
> - *
> - * RETURNS
> - * Zero or -errno
> - */
> -int reservation_object_get_fences_rcu(struct reservation_object *obj,
> +static int __reservation_object_get_fences_rcu(struct reservation_object *obj,
> struct dma_fence **pfence_excl,
> unsigned *pshared_count,
> - struct dma_fence ***pshared)
> + struct dma_fence ***pshared,
> + struct reservation_object_list **list_shared)
> {
> struct dma_fence **shared = NULL;
> struct dma_fence *fence_excl;
> unsigned int shared_count;
> int ret = 1;
> + void *ptr = NULL;
>
> do {
> struct reservation_object_list *fobj;
> @@ -359,23 +315,16 @@ int reservation_object_get_fences_rcu(struct reservation_object *obj,
>
> fobj = rcu_dereference(obj->fence);
> if (fobj) {
> - struct dma_fence **nshared;
> - size_t sz = sizeof(*shared) * fobj->shared_max;
> -
> - nshared = krealloc(shared, sz,
> - GFP_NOWAIT | __GFP_NOWARN);
> - if (!nshared) {
> + if (!realloc_shared(&ptr, &shared, fobj->shared_max, list_shared, false)) {
> rcu_read_unlock();
> - nshared = krealloc(shared, sz, GFP_KERNEL);
> - if (nshared) {
> - shared = nshared;
> - continue;
> + if (!realloc_shared(&ptr, &shared, fobj->shared_max, list_shared, true)) {
> + ret = -ENOMEM;
> + break;
> }
>
> - ret = -ENOMEM;
> - break;
> + /* need to acquire rcu lock again and retry */
> + continue;
> }
> - shared = nshared;
> shared_count = fobj->shared_count;
>
> for (i = 0; i < shared_count; ++i) {
> @@ -398,16 +347,93 @@ int reservation_object_get_fences_rcu(struct reservation_object *obj,
> } while (ret);
>
> if (!shared_count) {
> - kfree(shared);
> + kfree(ptr);
> + ptr = NULL;
> shared = NULL;
> }
>
> - *pshared_count = shared_count;
> - *pshared = shared;
> - *pfence_excl = fence_excl;
> + if (!list_shared) {
> + *pshared_count = shared_count;
> + *pshared = shared;
> + } else {
> + *list_shared = ptr;
> + if (ptr)
> + (*list_shared)->shared_count = shared_count;
> + }
>
> + *pfence_excl = fence_excl;
> return ret;
> }
> +
> +
> +/**
> +* reservation_object_copy_fences - Copy all fences from src to dst.
> +* @dst: the destination reservation object
> +* @src: the source reservation object
> +*
> +* Copy all fences from src to dst. dst-lock must be held.
> +*/
> +int reservation_object_copy_fences(struct reservation_object *dst,
> + struct reservation_object *src)
> +{
> + struct reservation_object_list *old_list, *dst_list;
> + struct dma_fence *excl, *old;
> + unsigned i;
> + int ret;
> +
> + ret = __reservation_object_get_fences_rcu(src, &excl, NULL,
> + NULL, &dst_list);
> + if (ret)
> + return ret;
> +
> + kfree(dst->staged);
> + dst->staged = NULL;
> +
> + old = rcu_dereference_protected(dst->fence_excl,
> + reservation_object_held(dst));
> +
> + old_list = rcu_dereference_protected(dst->fence,
> + reservation_object_held(dst));
> +
> + preempt_disable();
> + write_seqcount_begin(&dst->seq);
> + /* write_seqcount_begin provides the necessary memory barrier */
> + RCU_INIT_POINTER(dst->fence_excl, excl);
> + RCU_INIT_POINTER(dst->fence, dst_list);
> + write_seqcount_end(&dst->seq);
> + preempt_enable();
> +
> + if (old_list) {
> + for (i = 0; i < old_list->shared_count; i++)
> + dma_fence_put(old_list->shared[i]);
> +
> + kfree_rcu(old_list, rcu);
> + }
> + dma_fence_put(old);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(reservation_object_copy_fences);
> +
> +/**
> + * reservation_object_get_fences_rcu - Get an object's shared and exclusive
> + * fences without update side lock held
> + * @obj: the reservation object
> + * @pfence_excl: the returned exclusive fence (or NULL)
> + * @pshared_count: the number of shared fences returned
> + * @pshared: the array of shared fence ptrs returned (array is krealloc'd to
> + * the required size, and must be freed by caller)
> + *
> + * RETURNS
> + * Zero or -errno
> + */
> +int reservation_object_get_fences_rcu(struct reservation_object *obj,
> + struct dma_fence **pfence_excl,
> + unsigned *pshared_count,
> + struct dma_fence ***pshared)
> +{
> + return __reservation_object_get_fences_rcu(obj, pfence_excl, pshared_count, pshared, NULL);
> +}
> EXPORT_SYMBOL_GPL(reservation_object_get_fences_rcu);
>
> /**
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
More information about the amd-gfx
mailing list