Re: [PATCH 1/8] dma-buf: remove shared fence staging in reservation object

Re: [PATCH 1/8] dma-buf: remove shared fence staging in reservation object

[Christian König]

Ping! Adding a few people directly and more mailing lists.

Can I get an acked-by/rb for this? It's only a cleanup and not much 
functional change.

Regards,
Christian.

Am 04.10.2018 um 15:12 schrieb Christian König:
> No need for that any more. Just replace the list when there isn't enough
> room any more for the additional fence.
>
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
>   drivers/dma-buf/reservation.c | 178 ++++++++++++++----------------------------
>   include/linux/reservation.h   |   4 -
>   2 files changed, 58 insertions(+), 124 deletions(-)
>
> diff --git a/drivers/dma-buf/reservation.c b/drivers/dma-buf/reservation.c
> index 6c95f61a32e7..5825fc336a13 100644
> --- a/drivers/dma-buf/reservation.c
> +++ b/drivers/dma-buf/reservation.c
> @@ -68,105 +68,23 @@ EXPORT_SYMBOL(reservation_seqcount_string);
>    */
>   int reservation_object_reserve_shared(struct reservation_object *obj)
>   {
> -	struct reservation_object_list *fobj, *old;
> -	u32 max;
> +	struct reservation_object_list *old, *new;
> +	unsigned int i, j, k, max;
>   
>   	old = reservation_object_get_list(obj);
>   
>   	if (old && old->shared_max) {
> -		if (old->shared_count < old->shared_max) {
> -			/* perform an in-place update */
> -			kfree(obj->staged);
> -			obj->staged = NULL;
> +		if (old->shared_count < old->shared_max)
>   			return 0;
> -		} else
> +		else
>   			max = old->shared_max * 2;
> -	} else
> -		max = 4;
> -
> -	/*
> -	 * resize obj->staged or allocate if it doesn't exist,
> -	 * noop if already correct size
> -	 */
> -	fobj = krealloc(obj->staged, offsetof(typeof(*fobj), shared[max]),
> -			GFP_KERNEL);
> -	if (!fobj)
> -		return -ENOMEM;
> -
> -	obj->staged = fobj;
> -	fobj->shared_max = max;
> -	return 0;
> -}
> -EXPORT_SYMBOL(reservation_object_reserve_shared);
> -
> -static void
> -reservation_object_add_shared_inplace(struct reservation_object *obj,
> -				      struct reservation_object_list *fobj,
> -				      struct dma_fence *fence)
> -{
> -	struct dma_fence *signaled = NULL;
> -	u32 i, signaled_idx;
> -
> -	dma_fence_get(fence);
> -
> -	preempt_disable();
> -	write_seqcount_begin(&obj->seq);
> -
> -	for (i = 0; i < fobj->shared_count; ++i) {
> -		struct dma_fence *old_fence;
> -
> -		old_fence = rcu_dereference_protected(fobj->shared[i],
> -						reservation_object_held(obj));
> -
> -		if (old_fence->context == fence->context) {
> -			/* memory barrier is added by write_seqcount_begin */
> -			RCU_INIT_POINTER(fobj->shared[i], fence);
> -			write_seqcount_end(&obj->seq);
> -			preempt_enable();
> -
> -			dma_fence_put(old_fence);
> -			return;
> -		}
> -
> -		if (!signaled && dma_fence_is_signaled(old_fence)) {
> -			signaled = old_fence;
> -			signaled_idx = i;
> -		}
> -	}
> -
> -	/*
> -	 * memory barrier is added by write_seqcount_begin,
> -	 * fobj->shared_count is protected by this lock too
> -	 */
> -	if (signaled) {
> -		RCU_INIT_POINTER(fobj->shared[signaled_idx], fence);
>   	} else {
> -		BUG_ON(fobj->shared_count >= fobj->shared_max);
> -		RCU_INIT_POINTER(fobj->shared[fobj->shared_count], fence);
> -		fobj->shared_count++;
> +		max = 4;
>   	}
>   
> -	write_seqcount_end(&obj->seq);
> -	preempt_enable();
> -
> -	dma_fence_put(signaled);
> -}
> -
> -static void
> -reservation_object_add_shared_replace(struct reservation_object *obj,
> -				      struct reservation_object_list *old,
> -				      struct reservation_object_list *fobj,
> -				      struct dma_fence *fence)
> -{
> -	unsigned i, j, k;
> -
> -	dma_fence_get(fence);
> -
> -	if (!old) {
> -		RCU_INIT_POINTER(fobj->shared[0], fence);
> -		fobj->shared_count = 1;
> -		goto done;
> -	}
> +	new = kmalloc(offsetof(typeof(*new), shared[max]), GFP_KERNEL);
> +	if (!new)
> +		return -ENOMEM;
>   
>   	/*
>   	 * no need to bump fence refcounts, rcu_read access
> @@ -174,46 +92,45 @@ reservation_object_add_shared_replace(struct reservation_object *obj,
>   	 * references from the old struct are carried over to
>   	 * the new.
>   	 */
> -	for (i = 0, j = 0, k = fobj->shared_max; i < old->shared_count; ++i) {
> -		struct dma_fence *check;
> +	for (i = 0, j = 0, k = max; i < (old ? old->shared_count : 0); ++i) {
> +		struct dma_fence *fence;
>   
> -		check = rcu_dereference_protected(old->shared[i],
> -						reservation_object_held(obj));
> -
> -		if (check->context == fence->context ||
> -		    dma_fence_is_signaled(check))
> -			RCU_INIT_POINTER(fobj->shared[--k], check);
> +		fence = rcu_dereference_protected(old->shared[i],
> +						  reservation_object_held(obj));
> +		if (dma_fence_is_signaled(fence))
> +			RCU_INIT_POINTER(new->shared[--k], fence);
>   		else
> -			RCU_INIT_POINTER(fobj->shared[j++], check);
> +			RCU_INIT_POINTER(new->shared[j++], fence);
>   	}
> -	fobj->shared_count = j;
> -	RCU_INIT_POINTER(fobj->shared[fobj->shared_count], fence);
> -	fobj->shared_count++;
> +	new->shared_count = j;
> +	new->shared_max = max;
>   
> -done:
>   	preempt_disable();
>   	write_seqcount_begin(&obj->seq);
>   	/*
>   	 * RCU_INIT_POINTER can be used here,
>   	 * seqcount provides the necessary barriers
>   	 */
> -	RCU_INIT_POINTER(obj->fence, fobj);
> +	RCU_INIT_POINTER(obj->fence, new);
>   	write_seqcount_end(&obj->seq);
>   	preempt_enable();
>   
>   	if (!old)
> -		return;
> +		return 0;
>   
>   	/* Drop the references to the signaled fences */
> -	for (i = k; i < fobj->shared_max; ++i) {
> -		struct dma_fence *f;
> +	for (i = k; i < new->shared_max; ++i) {
> +		struct dma_fence *fence;
>   
> -		f = rcu_dereference_protected(fobj->shared[i],
> -					      reservation_object_held(obj));
> -		dma_fence_put(f);
> +		fence = rcu_dereference_protected(new->shared[i],
> +						  reservation_object_held(obj));
> +		dma_fence_put(fence);
>   	}
>   	kfree_rcu(old, rcu);
> +
> +	return 0;
>   }
> +EXPORT_SYMBOL(reservation_object_reserve_shared);
>   
>   /**
>    * reservation_object_add_shared_fence - Add a fence to a shared slot
> @@ -226,15 +143,39 @@ reservation_object_add_shared_replace(struct reservation_object *obj,
>   void reservation_object_add_shared_fence(struct reservation_object *obj,
>   					 struct dma_fence *fence)
>   {
> -	struct reservation_object_list *old, *fobj = obj->staged;
> +	struct reservation_object_list *fobj;
> +	unsigned int i;
>   
> -	old = reservation_object_get_list(obj);
> -	obj->staged = NULL;
> +	dma_fence_get(fence);
> +
> +	fobj = reservation_object_get_list(obj);
>   
> -	if (!fobj)
> -		reservation_object_add_shared_inplace(obj, old, fence);
> -	else
> -		reservation_object_add_shared_replace(obj, old, fobj, fence);
> +	preempt_disable();
> +	write_seqcount_begin(&obj->seq);
> +
> +	for (i = 0; i < fobj->shared_count; ++i) {
> +		struct dma_fence *old_fence;
> +
> +		old_fence = rcu_dereference_protected(fobj->shared[i],
> +						      reservation_object_held(obj));
> +		if (old_fence->context == fence->context ||
> +		    dma_fence_is_signaled(old_fence)) {
> +			dma_fence_put(old_fence);
> +			goto replace;
> +		}
> +	}
> +
> +	BUG_ON(fobj->shared_count >= fobj->shared_max);
> +	fobj->shared_count++;
> +
> +replace:
> +	/*
> +	 * memory barrier is added by write_seqcount_begin,
> +	 * fobj->shared_count is protected by this lock too
> +	 */
> +	RCU_INIT_POINTER(fobj->shared[i], fence);
> +	write_seqcount_end(&obj->seq);
> +	preempt_enable();
>   }
>   EXPORT_SYMBOL(reservation_object_add_shared_fence);
>   
> @@ -343,9 +284,6 @@ int reservation_object_copy_fences(struct reservation_object *dst,
>   	new = dma_fence_get_rcu_safe(&src->fence_excl);
>   	rcu_read_unlock();
>   
> -	kfree(dst->staged);
> -	dst->staged = NULL;
> -
>   	src_list = reservation_object_get_list(dst);
>   	old = reservation_object_get_excl(dst);
>   
> diff --git a/include/linux/reservation.h b/include/linux/reservation.h
> index 02166e815afb..54cf6773a14c 100644
> --- a/include/linux/reservation.h
> +++ b/include/linux/reservation.h
> @@ -68,7 +68,6 @@ struct reservation_object_list {
>    * @seq: sequence count for managing RCU read-side synchronization
>    * @fence_excl: the exclusive fence, if there is one currently
>    * @fence: list of current shared fences
> - * @staged: staged copy of shared fences for RCU updates
>    */
>   struct reservation_object {
>   	struct ww_mutex lock;
> @@ -76,7 +75,6 @@ struct reservation_object {
>   
>   	struct dma_fence __rcu *fence_excl;
>   	struct reservation_object_list __rcu *fence;
> -	struct reservation_object_list *staged;
>   };
>   
>   #define reservation_object_held(obj) lockdep_is_held(&(obj)->lock.base)
> @@ -95,7 +93,6 @@ reservation_object_init(struct reservation_object *obj)
>   	__seqcount_init(&obj->seq, reservation_seqcount_string, &reservation_seqcount_class);
>   	RCU_INIT_POINTER(obj->fence, NULL);
>   	RCU_INIT_POINTER(obj->fence_excl, NULL);
> -	obj->staged = NULL;
>   }
>   
>   /**
> @@ -124,7 +121,6 @@ reservation_object_fini(struct reservation_object *obj)
>   
>   		kfree(fobj);
>   	}
> -	kfree(obj->staged);
>   
>   	ww_mutex_destroy(&obj->lock);
>   }

Re: [PATCH 1/8] dma-buf: remove shared fence staging in reservation object

[Christian König]

Ping once more! Adding a few more AMD people.

Any comments on this?

Thanks,
Christian.

Am 12.10.18 um 10:22 schrieb Christian König:
> Ping! Adding a few people directly and more mailing lists.
>
> Can I get an acked-by/rb for this? It's only a cleanup and not much 
> functional change.
>
> Regards,
> Christian.
>
> Am 04.10.2018 um 15:12 schrieb Christian König:
>> No need for that any more. Just replace the list when there isn't enough
>> room any more for the additional fence.
>>
>> Signed-off-by: Christian König <christian.koenig@amd.com>
>> ---
>>   drivers/dma-buf/reservation.c | 178 
>> ++++++++++++++----------------------------
>>   include/linux/reservation.h   |   4 -
>>   2 files changed, 58 insertions(+), 124 deletions(-)
>>
>> diff --git a/drivers/dma-buf/reservation.c 
>> b/drivers/dma-buf/reservation.c
>> index 6c95f61a32e7..5825fc336a13 100644
>> --- a/drivers/dma-buf/reservation.c
>> +++ b/drivers/dma-buf/reservation.c
>> @@ -68,105 +68,23 @@ EXPORT_SYMBOL(reservation_seqcount_string);
>>    */
>>   int reservation_object_reserve_shared(struct reservation_object *obj)
>>   {
>> -    struct reservation_object_list *fobj, *old;
>> -    u32 max;
>> +    struct reservation_object_list *old, *new;
>> +    unsigned int i, j, k, max;
>>         old = reservation_object_get_list(obj);
>>         if (old && old->shared_max) {
>> -        if (old->shared_count < old->shared_max) {
>> -            /* perform an in-place update */
>> -            kfree(obj->staged);
>> -            obj->staged = NULL;
>> +        if (old->shared_count < old->shared_max)
>>               return 0;
>> -        } else
>> +        else
>>               max = old->shared_max * 2;
>> -    } else
>> -        max = 4;
>> -
>> -    /*
>> -     * resize obj->staged or allocate if it doesn't exist,
>> -     * noop if already correct size
>> -     */
>> -    fobj = krealloc(obj->staged, offsetof(typeof(*fobj), shared[max]),
>> -            GFP_KERNEL);
>> -    if (!fobj)
>> -        return -ENOMEM;
>> -
>> -    obj->staged = fobj;
>> -    fobj->shared_max = max;
>> -    return 0;
>> -}
>> -EXPORT_SYMBOL(reservation_object_reserve_shared);
>> -
>> -static void
>> -reservation_object_add_shared_inplace(struct reservation_object *obj,
>> -                      struct reservation_object_list *fobj,
>> -                      struct dma_fence *fence)
>> -{
>> -    struct dma_fence *signaled = NULL;
>> -    u32 i, signaled_idx;
>> -
>> -    dma_fence_get(fence);
>> -
>> -    preempt_disable();
>> -    write_seqcount_begin(&obj->seq);
>> -
>> -    for (i = 0; i < fobj->shared_count; ++i) {
>> -        struct dma_fence *old_fence;
>> -
>> -        old_fence = rcu_dereference_protected(fobj->shared[i],
>> -                        reservation_object_held(obj));
>> -
>> -        if (old_fence->context == fence->context) {
>> -            /* memory barrier is added by write_seqcount_begin */
>> -            RCU_INIT_POINTER(fobj->shared[i], fence);
>> -            write_seqcount_end(&obj->seq);
>> -            preempt_enable();
>> -
>> -            dma_fence_put(old_fence);
>> -            return;
>> -        }
>> -
>> -        if (!signaled && dma_fence_is_signaled(old_fence)) {
>> -            signaled = old_fence;
>> -            signaled_idx = i;
>> -        }
>> -    }
>> -
>> -    /*
>> -     * memory barrier is added by write_seqcount_begin,
>> -     * fobj->shared_count is protected by this lock too
>> -     */
>> -    if (signaled) {
>> -        RCU_INIT_POINTER(fobj->shared[signaled_idx], fence);
>>       } else {
>> -        BUG_ON(fobj->shared_count >= fobj->shared_max);
>> - RCU_INIT_POINTER(fobj->shared[fobj->shared_count], fence);
>> -        fobj->shared_count++;
>> +        max = 4;
>>       }
>>   -    write_seqcount_end(&obj->seq);
>> -    preempt_enable();
>> -
>> -    dma_fence_put(signaled);
>> -}
>> -
>> -static void
>> -reservation_object_add_shared_replace(struct reservation_object *obj,
>> -                      struct reservation_object_list *old,
>> -                      struct reservation_object_list *fobj,
>> -                      struct dma_fence *fence)
>> -{
>> -    unsigned i, j, k;
>> -
>> -    dma_fence_get(fence);
>> -
>> -    if (!old) {
>> -        RCU_INIT_POINTER(fobj->shared[0], fence);
>> -        fobj->shared_count = 1;
>> -        goto done;
>> -    }
>> +    new = kmalloc(offsetof(typeof(*new), shared[max]), GFP_KERNEL);
>> +    if (!new)
>> +        return -ENOMEM;
>>         /*
>>        * no need to bump fence refcounts, rcu_read access
>> @@ -174,46 +92,45 @@ reservation_object_add_shared_replace(struct 
>> reservation_object *obj,
>>        * references from the old struct are carried over to
>>        * the new.
>>        */
>> -    for (i = 0, j = 0, k = fobj->shared_max; i < old->shared_count; 
>> ++i) {
>> -        struct dma_fence *check;
>> +    for (i = 0, j = 0, k = max; i < (old ? old->shared_count : 0); 
>> ++i) {
>> +        struct dma_fence *fence;
>>   -        check = rcu_dereference_protected(old->shared[i],
>> -                        reservation_object_held(obj));
>> -
>> -        if (check->context == fence->context ||
>> -            dma_fence_is_signaled(check))
>> -            RCU_INIT_POINTER(fobj->shared[--k], check);
>> +        fence = rcu_dereference_protected(old->shared[i],
>> +                          reservation_object_held(obj));
>> +        if (dma_fence_is_signaled(fence))
>> +            RCU_INIT_POINTER(new->shared[--k], fence);
>>           else
>> -            RCU_INIT_POINTER(fobj->shared[j++], check);
>> +            RCU_INIT_POINTER(new->shared[j++], fence);
>>       }
>> -    fobj->shared_count = j;
>> -    RCU_INIT_POINTER(fobj->shared[fobj->shared_count], fence);
>> -    fobj->shared_count++;
>> +    new->shared_count = j;
>> +    new->shared_max = max;
>>   -done:
>>       preempt_disable();
>>       write_seqcount_begin(&obj->seq);
>>       /*
>>        * RCU_INIT_POINTER can be used here,
>>        * seqcount provides the necessary barriers
>>        */
>> -    RCU_INIT_POINTER(obj->fence, fobj);
>> +    RCU_INIT_POINTER(obj->fence, new);
>>       write_seqcount_end(&obj->seq);
>>       preempt_enable();
>>         if (!old)
>> -        return;
>> +        return 0;
>>         /* Drop the references to the signaled fences */
>> -    for (i = k; i < fobj->shared_max; ++i) {
>> -        struct dma_fence *f;
>> +    for (i = k; i < new->shared_max; ++i) {
>> +        struct dma_fence *fence;
>>   -        f = rcu_dereference_protected(fobj->shared[i],
>> -                          reservation_object_held(obj));
>> -        dma_fence_put(f);
>> +        fence = rcu_dereference_protected(new->shared[i],
>> +                          reservation_object_held(obj));
>> +        dma_fence_put(fence);
>>       }
>>       kfree_rcu(old, rcu);
>> +
>> +    return 0;
>>   }
>> +EXPORT_SYMBOL(reservation_object_reserve_shared);
>>     /**
>>    * reservation_object_add_shared_fence - Add a fence to a shared slot
>> @@ -226,15 +143,39 @@ reservation_object_add_shared_replace(struct 
>> reservation_object *obj,
>>   void reservation_object_add_shared_fence(struct reservation_object 
>> *obj,
>>                        struct dma_fence *fence)
>>   {
>> -    struct reservation_object_list *old, *fobj = obj->staged;
>> +    struct reservation_object_list *fobj;
>> +    unsigned int i;
>>   -    old = reservation_object_get_list(obj);
>> -    obj->staged = NULL;
>> +    dma_fence_get(fence);
>> +
>> +    fobj = reservation_object_get_list(obj);
>>   -    if (!fobj)
>> -        reservation_object_add_shared_inplace(obj, old, fence);
>> -    else
>> -        reservation_object_add_shared_replace(obj, old, fobj, fence);
>> +    preempt_disable();
>> +    write_seqcount_begin(&obj->seq);
>> +
>> +    for (i = 0; i < fobj->shared_count; ++i) {
>> +        struct dma_fence *old_fence;
>> +
>> +        old_fence = rcu_dereference_protected(fobj->shared[i],
>> +                              reservation_object_held(obj));
>> +        if (old_fence->context == fence->context ||
>> +            dma_fence_is_signaled(old_fence)) {
>> +            dma_fence_put(old_fence);
>> +            goto replace;
>> +        }
>> +    }
>> +
>> +    BUG_ON(fobj->shared_count >= fobj->shared_max);
>> +    fobj->shared_count++;
>> +
>> +replace:
>> +    /*
>> +     * memory barrier is added by write_seqcount_begin,
>> +     * fobj->shared_count is protected by this lock too
>> +     */
>> +    RCU_INIT_POINTER(fobj->shared[i], fence);
>> +    write_seqcount_end(&obj->seq);
>> +    preempt_enable();
>>   }
>>   EXPORT_SYMBOL(reservation_object_add_shared_fence);
>>   @@ -343,9 +284,6 @@ int reservation_object_copy_fences(struct 
>> reservation_object *dst,
>>       new = dma_fence_get_rcu_safe(&src->fence_excl);
>>       rcu_read_unlock();
>>   -    kfree(dst->staged);
>> -    dst->staged = NULL;
>> -
>>       src_list = reservation_object_get_list(dst);
>>       old = reservation_object_get_excl(dst);
>>   diff --git a/include/linux/reservation.h b/include/linux/reservation.h
>> index 02166e815afb..54cf6773a14c 100644
>> --- a/include/linux/reservation.h
>> +++ b/include/linux/reservation.h
>> @@ -68,7 +68,6 @@ struct reservation_object_list {
>>    * @seq: sequence count for managing RCU read-side synchronization
>>    * @fence_excl: the exclusive fence, if there is one currently
>>    * @fence: list of current shared fences
>> - * @staged: staged copy of shared fences for RCU updates
>>    */
>>   struct reservation_object {
>>       struct ww_mutex lock;
>> @@ -76,7 +75,6 @@ struct reservation_object {
>>         struct dma_fence __rcu *fence_excl;
>>       struct reservation_object_list __rcu *fence;
>> -    struct reservation_object_list *staged;
>>   };
>>     #define reservation_object_held(obj) 
>> lockdep_is_held(&(obj)->lock.base)
>> @@ -95,7 +93,6 @@ reservation_object_init(struct reservation_object 
>> *obj)
>>       __seqcount_init(&obj->seq, reservation_seqcount_string, 
>> &reservation_seqcount_class);
>>       RCU_INIT_POINTER(obj->fence, NULL);
>>       RCU_INIT_POINTER(obj->fence_excl, NULL);
>> -    obj->staged = NULL;
>>   }
>>     /**
>> @@ -124,7 +121,6 @@ reservation_object_fini(struct reservation_object 
>> *obj)
>>             kfree(fobj);
>>       }
>> -    kfree(obj->staged);
>>         ww_mutex_destroy(&obj->lock);
>>   }
>

Re: [PATCH 1/8] dma-buf: remove shared fence staging in reservation object

[Michel Dänzer]

On 2018-10-23 2:20 p.m., Christian König wrote:
> Ping once more! Adding a few more AMD people.
> 
> Any comments on this?

Patches 1 & 3 are a bit over my head I'm afraid.


Patches 2, 4, 6-8 are

Reviewed-by: Michel Dänzer <michel.daenzer@amd.com>


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer

RE: [PATCH 1/8] dma-buf: remove shared fence staging in reservation object

[Huang, Ray]

Christian, I will take a look at them later.

Thanks,
Ray

> -----Original Message-----
> From: Christian König [mailto:ckoenig.leichtzumerken@gmail.com]
> Sent: Tuesday, October 23, 2018 8:20 PM
> To: amd-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; linux-
> media@vger.kernel.org; linaro-mm-sig@lists.linaro.org; Huang, Ray
> <Ray.Huang@amd.com>; Daenzer, Michel <Michel.Daenzer@amd.com>
> Cc: Daniel Vetter <daniel@ffwll.ch>; Chris Wilson <chris@chris-wilson.co.uk>;
> Zhang, Jerry <Jerry.Zhang@amd.com>
> Subject: Re: [PATCH 1/8] dma-buf: remove shared fence staging in
> reservation object
> 
> Ping once more! Adding a few more AMD people.
> 
> Any comments on this?
> 
> Thanks,
> Christian.
> 
> Am 12.10.18 um 10:22 schrieb Christian König:
> > Ping! Adding a few people directly and more mailing lists.
> >
> > Can I get an acked-by/rb for this? It's only a cleanup and not much
> > functional change.
> >
> > Regards,
> > Christian.
> >
> > Am 04.10.2018 um 15:12 schrieb Christian König:
> >> No need for that any more. Just replace the list when there isn't
> >> enough room any more for the additional fence.
> >>
> >> Signed-off-by: Christian König <christian.koenig@amd.com>
> >> ---
> >>   drivers/dma-buf/reservation.c | 178
> >> ++++++++++++++----------------------------
> >>   include/linux/reservation.h   |   4 -
> >>   2 files changed, 58 insertions(+), 124 deletions(-)
> >>
> >> diff --git a/drivers/dma-buf/reservation.c
> >> b/drivers/dma-buf/reservation.c index 6c95f61a32e7..5825fc336a13
> >> 100644
> >> --- a/drivers/dma-buf/reservation.c
> >> +++ b/drivers/dma-buf/reservation.c
> >> @@ -68,105 +68,23 @@ EXPORT_SYMBOL(reservation_seqcount_string);
> >>    */
> >>   int reservation_object_reserve_shared(struct reservation_object
> >> *obj)
> >>   {
> >> -    struct reservation_object_list *fobj, *old;
> >> -    u32 max;
> >> +    struct reservation_object_list *old, *new;
> >> +    unsigned int i, j, k, max;
> >>         old = reservation_object_get_list(obj);
> >>         if (old && old->shared_max) {
> >> -        if (old->shared_count < old->shared_max) {
> >> -            /* perform an in-place update */
> >> -            kfree(obj->staged);
> >> -            obj->staged = NULL;
> >> +        if (old->shared_count < old->shared_max)
> >>               return 0;
> >> -        } else
> >> +        else
> >>               max = old->shared_max * 2;
> >> -    } else
> >> -        max = 4;
> >> -
> >> -    /*
> >> -     * resize obj->staged or allocate if it doesn't exist,
> >> -     * noop if already correct size
> >> -     */
> >> -    fobj = krealloc(obj->staged, offsetof(typeof(*fobj),
> >> shared[max]),
> >> -            GFP_KERNEL);
> >> -    if (!fobj)
> >> -        return -ENOMEM;
> >> -
> >> -    obj->staged = fobj;
> >> -    fobj->shared_max = max;
> >> -    return 0;
> >> -}
> >> -EXPORT_SYMBOL(reservation_object_reserve_shared);
> >> -
> >> -static void
> >> -reservation_object_add_shared_inplace(struct reservation_object
> >> *obj,
> >> -                      struct reservation_object_list *fobj,
> >> -                      struct dma_fence *fence) -{
> >> -    struct dma_fence *signaled = NULL;
> >> -    u32 i, signaled_idx;
> >> -
> >> -    dma_fence_get(fence);
> >> -
> >> -    preempt_disable();
> >> -    write_seqcount_begin(&obj->seq);
> >> -
> >> -    for (i = 0; i < fobj->shared_count; ++i) {
> >> -        struct dma_fence *old_fence;
> >> -
> >> -        old_fence = rcu_dereference_protected(fobj->shared[i],
> >> -                        reservation_object_held(obj));
> >> -
> >> -        if (old_fence->context == fence->context) {
> >> -            /* memory barrier is added by write_seqcount_begin */
> >> -            RCU_INIT_POINTER(fobj->shared[i], fence);
> >> -            write_seqcount_end(&obj->seq);
> >> -            preempt_enable();
> >> -
> >> -            dma_fence_put(old_fence);
> >> -            return;
> >> -        }
> >> -
> >> -        if (!signaled && dma_fence_is_signaled(old_fence)) {
> >> -            signaled = old_fence;
> >> -            signaled_idx = i;
> >> -        }
> >> -    }
> >> -
> >> -    /*
> >> -     * memory barrier is added by write_seqcount_begin,
> >> -     * fobj->shared_count is protected by this lock too
> >> -     */
> >> -    if (signaled) {
> >> -        RCU_INIT_POINTER(fobj->shared[signaled_idx], fence);
> >>       } else {
> >> -        BUG_ON(fobj->shared_count >= fobj->shared_max);
> >> - RCU_INIT_POINTER(fobj->shared[fobj->shared_count], fence);
> >> -        fobj->shared_count++;
> >> +        max = 4;
> >>       }
> >>   -    write_seqcount_end(&obj->seq);
> >> -    preempt_enable();
> >> -
> >> -    dma_fence_put(signaled);
> >> -}
> >> -
> >> -static void
> >> -reservation_object_add_shared_replace(struct reservation_object
> >> *obj,
> >> -                      struct reservation_object_list *old,
> >> -                      struct reservation_object_list *fobj,
> >> -                      struct dma_fence *fence) -{
> >> -    unsigned i, j, k;
> >> -
> >> -    dma_fence_get(fence);
> >> -
> >> -    if (!old) {
> >> -        RCU_INIT_POINTER(fobj->shared[0], fence);
> >> -        fobj->shared_count = 1;
> >> -        goto done;
> >> -    }
> >> +    new = kmalloc(offsetof(typeof(*new), shared[max]), GFP_KERNEL);
> >> +    if (!new)
> >> +        return -ENOMEM;
> >>         /*
> >>        * no need to bump fence refcounts, rcu_read access @@ -174,46
> >> +92,45 @@ reservation_object_add_shared_replace(struct
> >> reservation_object *obj,
> >>        * references from the old struct are carried over to
> >>        * the new.
> >>        */
> >> -    for (i = 0, j = 0, k = fobj->shared_max; i < old->shared_count;
> >> ++i) {
> >> -        struct dma_fence *check;
> >> +    for (i = 0, j = 0, k = max; i < (old ? old->shared_count : 0);
> >> ++i) {
> >> +        struct dma_fence *fence;
> >>   -        check = rcu_dereference_protected(old->shared[i],
> >> -                        reservation_object_held(obj));
> >> -
> >> -        if (check->context == fence->context ||
> >> -            dma_fence_is_signaled(check))
> >> -            RCU_INIT_POINTER(fobj->shared[--k], check);
> >> +        fence = rcu_dereference_protected(old->shared[i],
> >> +                          reservation_object_held(obj));
> >> +        if (dma_fence_is_signaled(fence))
> >> +            RCU_INIT_POINTER(new->shared[--k], fence);
> >>           else
> >> -            RCU_INIT_POINTER(fobj->shared[j++], check);
> >> +            RCU_INIT_POINTER(new->shared[j++], fence);
> >>       }
> >> -    fobj->shared_count = j;
> >> -    RCU_INIT_POINTER(fobj->shared[fobj->shared_count], fence);
> >> -    fobj->shared_count++;
> >> +    new->shared_count = j;
> >> +    new->shared_max = max;
> >>   -done:
> >>       preempt_disable();
> >>       write_seqcount_begin(&obj->seq);
> >>       /*
> >>        * RCU_INIT_POINTER can be used here,
> >>        * seqcount provides the necessary barriers
> >>        */
> >> -    RCU_INIT_POINTER(obj->fence, fobj);
> >> +    RCU_INIT_POINTER(obj->fence, new);
> >>       write_seqcount_end(&obj->seq);
> >>       preempt_enable();
> >>         if (!old)
> >> -        return;
> >> +        return 0;
> >>         /* Drop the references to the signaled fences */
> >> -    for (i = k; i < fobj->shared_max; ++i) {
> >> -        struct dma_fence *f;
> >> +    for (i = k; i < new->shared_max; ++i) {
> >> +        struct dma_fence *fence;
> >>   -        f = rcu_dereference_protected(fobj->shared[i],
> >> -                          reservation_object_held(obj));
> >> -        dma_fence_put(f);
> >> +        fence = rcu_dereference_protected(new->shared[i],
> >> +                          reservation_object_held(obj));
> >> +        dma_fence_put(fence);
> >>       }
> >>       kfree_rcu(old, rcu);
> >> +
> >> +    return 0;
> >>   }
> >> +EXPORT_SYMBOL(reservation_object_reserve_shared);
> >>     /**
> >>    * reservation_object_add_shared_fence - Add a fence to a shared
> >> slot @@ -226,15 +143,39 @@
> >> reservation_object_add_shared_replace(struct
> >> reservation_object *obj,
> >>   void reservation_object_add_shared_fence(struct reservation_object
> >> *obj,
> >>                        struct dma_fence *fence)
> >>   {
> >> -    struct reservation_object_list *old, *fobj = obj->staged;
> >> +    struct reservation_object_list *fobj;
> >> +    unsigned int i;
> >>   -    old = reservation_object_get_list(obj);
> >> -    obj->staged = NULL;
> >> +    dma_fence_get(fence);
> >> +
> >> +    fobj = reservation_object_get_list(obj);
> >>   -    if (!fobj)
> >> -        reservation_object_add_shared_inplace(obj, old, fence);
> >> -    else
> >> -        reservation_object_add_shared_replace(obj, old, fobj,
> >> fence);
> >> +    preempt_disable();
> >> +    write_seqcount_begin(&obj->seq);
> >> +
> >> +    for (i = 0; i < fobj->shared_count; ++i) {
> >> +        struct dma_fence *old_fence;
> >> +
> >> +        old_fence = rcu_dereference_protected(fobj->shared[i],
> >> +                              reservation_object_held(obj));
> >> +        if (old_fence->context == fence->context ||
> >> +            dma_fence_is_signaled(old_fence)) {
> >> +            dma_fence_put(old_fence);
> >> +            goto replace;
> >> +        }
> >> +    }
> >> +
> >> +    BUG_ON(fobj->shared_count >= fobj->shared_max);
> >> +    fobj->shared_count++;
> >> +
> >> +replace:
> >> +    /*
> >> +     * memory barrier is added by write_seqcount_begin,
> >> +     * fobj->shared_count is protected by this lock too
> >> +     */
> >> +    RCU_INIT_POINTER(fobj->shared[i], fence);
> >> +    write_seqcount_end(&obj->seq);
> >> +    preempt_enable();
> >>   }
> >>   EXPORT_SYMBOL(reservation_object_add_shared_fence);
> >>   @@ -343,9 +284,6 @@ int reservation_object_copy_fences(struct
> >> reservation_object *dst,
> >>       new = dma_fence_get_rcu_safe(&src->fence_excl);
> >>       rcu_read_unlock();
> >>   -    kfree(dst->staged);
> >> -    dst->staged = NULL;
> >> -
> >>       src_list = reservation_object_get_list(dst);
> >>       old = reservation_object_get_excl(dst);
> >>   diff --git a/include/linux/reservation.h
> >> b/include/linux/reservation.h index 02166e815afb..54cf6773a14c 100644
> >> --- a/include/linux/reservation.h
> >> +++ b/include/linux/reservation.h
> >> @@ -68,7 +68,6 @@ struct reservation_object_list {
> >>    * @seq: sequence count for managing RCU read-side synchronization
> >>    * @fence_excl: the exclusive fence, if there is one currently
> >>    * @fence: list of current shared fences
> >> - * @staged: staged copy of shared fences for RCU updates
> >>    */
> >>   struct reservation_object {
> >>       struct ww_mutex lock;
> >> @@ -76,7 +75,6 @@ struct reservation_object {
> >>         struct dma_fence __rcu *fence_excl;
> >>       struct reservation_object_list __rcu *fence;
> >> -    struct reservation_object_list *staged;
> >>   };
> >>     #define reservation_object_held(obj)
> >> lockdep_is_held(&(obj)->lock.base)
> >> @@ -95,7 +93,6 @@ reservation_object_init(struct reservation_object
> >> *obj)
> >>       __seqcount_init(&obj->seq, reservation_seqcount_string,
> >> &reservation_seqcount_class);
> >>       RCU_INIT_POINTER(obj->fence, NULL);
> >>       RCU_INIT_POINTER(obj->fence_excl, NULL);
> >> -    obj->staged = NULL;
> >>   }
> >>     /**
> >> @@ -124,7 +121,6 @@ reservation_object_fini(struct reservation_object
> >> *obj)
> >>             kfree(fobj);
> >>       }
> >> -    kfree(obj->staged);
> >>         ww_mutex_destroy(&obj->lock);
> >>   }
> >

RE: [PATCH 1/8] dma-buf: remove shared fence staging in reservation object

[Huang, Ray]

Series are Reviewed-by: Huang Rui <ray.huang@amd.com>

> -----Original Message-----
> From: Christian König [mailto:ckoenig.leichtzumerken@gmail.com]
> Sent: Tuesday, October 23, 2018 8:20 PM
> To: amd-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; linux-
> media@vger.kernel.org; linaro-mm-sig@lists.linaro.org; Huang, Ray
> <Ray.Huang@amd.com>; Daenzer, Michel <Michel.Daenzer@amd.com>
> Cc: Daniel Vetter <daniel@ffwll.ch>; Chris Wilson <chris@chris-wilson.co.uk>;
> Zhang, Jerry <Jerry.Zhang@amd.com>
> Subject: Re: [PATCH 1/8] dma-buf: remove shared fence staging in
> reservation object
> 
> Ping once more! Adding a few more AMD people.
> 
> Any comments on this?
> 
> Thanks,
> Christian.
> 
> Am 12.10.18 um 10:22 schrieb Christian König:
> > Ping! Adding a few people directly and more mailing lists.
> >
> > Can I get an acked-by/rb for this? It's only a cleanup and not much
> > functional change.
> >
> > Regards,
> > Christian.
> >
> > Am 04.10.2018 um 15:12 schrieb Christian König:
> >> No need for that any more. Just replace the list when there isn't
> >> enough room any more for the additional fence.
> >>
> >> Signed-off-by: Christian König <christian.koenig@amd.com>
> >> ---
> >>   drivers/dma-buf/reservation.c | 178
> >> ++++++++++++++----------------------------
> >>   include/linux/reservation.h   |   4 -
> >>   2 files changed, 58 insertions(+), 124 deletions(-)
> >>
> >> diff --git a/drivers/dma-buf/reservation.c
> >> b/drivers/dma-buf/reservation.c index 6c95f61a32e7..5825fc336a13
> >> 100644
> >> --- a/drivers/dma-buf/reservation.c
> >> +++ b/drivers/dma-buf/reservation.c
> >> @@ -68,105 +68,23 @@ EXPORT_SYMBOL(reservation_seqcount_string);
> >>    */
> >>   int reservation_object_reserve_shared(struct reservation_object
> >> *obj)
> >>   {
> >> -    struct reservation_object_list *fobj, *old;
> >> -    u32 max;
> >> +    struct reservation_object_list *old, *new;
> >> +    unsigned int i, j, k, max;
> >>         old = reservation_object_get_list(obj);
> >>         if (old && old->shared_max) {
> >> -        if (old->shared_count < old->shared_max) {
> >> -            /* perform an in-place update */
> >> -            kfree(obj->staged);
> >> -            obj->staged = NULL;
> >> +        if (old->shared_count < old->shared_max)
> >>               return 0;
> >> -        } else
> >> +        else
> >>               max = old->shared_max * 2;
> >> -    } else
> >> -        max = 4;
> >> -
> >> -    /*
> >> -     * resize obj->staged or allocate if it doesn't exist,
> >> -     * noop if already correct size
> >> -     */
> >> -    fobj = krealloc(obj->staged, offsetof(typeof(*fobj),
> >> shared[max]),
> >> -            GFP_KERNEL);
> >> -    if (!fobj)
> >> -        return -ENOMEM;
> >> -
> >> -    obj->staged = fobj;
> >> -    fobj->shared_max = max;
> >> -    return 0;
> >> -}
> >> -EXPORT_SYMBOL(reservation_object_reserve_shared);
> >> -
> >> -static void
> >> -reservation_object_add_shared_inplace(struct reservation_object
> >> *obj,
> >> -                      struct reservation_object_list *fobj,
> >> -                      struct dma_fence *fence) -{
> >> -    struct dma_fence *signaled = NULL;
> >> -    u32 i, signaled_idx;
> >> -
> >> -    dma_fence_get(fence);
> >> -
> >> -    preempt_disable();
> >> -    write_seqcount_begin(&obj->seq);
> >> -
> >> -    for (i = 0; i < fobj->shared_count; ++i) {
> >> -        struct dma_fence *old_fence;
> >> -
> >> -        old_fence = rcu_dereference_protected(fobj->shared[i],
> >> -                        reservation_object_held(obj));
> >> -
> >> -        if (old_fence->context == fence->context) {
> >> -            /* memory barrier is added by write_seqcount_begin */
> >> -            RCU_INIT_POINTER(fobj->shared[i], fence);
> >> -            write_seqcount_end(&obj->seq);
> >> -            preempt_enable();
> >> -
> >> -            dma_fence_put(old_fence);
> >> -            return;
> >> -        }
> >> -
> >> -        if (!signaled && dma_fence_is_signaled(old_fence)) {
> >> -            signaled = old_fence;
> >> -            signaled_idx = i;
> >> -        }
> >> -    }
> >> -
> >> -    /*
> >> -     * memory barrier is added by write_seqcount_begin,
> >> -     * fobj->shared_count is protected by this lock too
> >> -     */
> >> -    if (signaled) {
> >> -        RCU_INIT_POINTER(fobj->shared[signaled_idx], fence);
> >>       } else {
> >> -        BUG_ON(fobj->shared_count >= fobj->shared_max);
> >> - RCU_INIT_POINTER(fobj->shared[fobj->shared_count], fence);
> >> -        fobj->shared_count++;
> >> +        max = 4;
> >>       }
> >>   -    write_seqcount_end(&obj->seq);
> >> -    preempt_enable();
> >> -
> >> -    dma_fence_put(signaled);
> >> -}
> >> -
> >> -static void
> >> -reservation_object_add_shared_replace(struct reservation_object
> >> *obj,
> >> -                      struct reservation_object_list *old,
> >> -                      struct reservation_object_list *fobj,
> >> -                      struct dma_fence *fence) -{
> >> -    unsigned i, j, k;
> >> -
> >> -    dma_fence_get(fence);
> >> -
> >> -    if (!old) {
> >> -        RCU_INIT_POINTER(fobj->shared[0], fence);
> >> -        fobj->shared_count = 1;
> >> -        goto done;
> >> -    }
> >> +    new = kmalloc(offsetof(typeof(*new), shared[max]), GFP_KERNEL);
> >> +    if (!new)
> >> +        return -ENOMEM;
> >>         /*
> >>        * no need to bump fence refcounts, rcu_read access @@ -174,46
> >> +92,45 @@ reservation_object_add_shared_replace(struct
> >> reservation_object *obj,
> >>        * references from the old struct are carried over to
> >>        * the new.
> >>        */
> >> -    for (i = 0, j = 0, k = fobj->shared_max; i < old->shared_count;
> >> ++i) {
> >> -        struct dma_fence *check;
> >> +    for (i = 0, j = 0, k = max; i < (old ? old->shared_count : 0);
> >> ++i) {
> >> +        struct dma_fence *fence;
> >>   -        check = rcu_dereference_protected(old->shared[i],
> >> -                        reservation_object_held(obj));
> >> -
> >> -        if (check->context == fence->context ||
> >> -            dma_fence_is_signaled(check))
> >> -            RCU_INIT_POINTER(fobj->shared[--k], check);
> >> +        fence = rcu_dereference_protected(old->shared[i],
> >> +                          reservation_object_held(obj));
> >> +        if (dma_fence_is_signaled(fence))
> >> +            RCU_INIT_POINTER(new->shared[--k], fence);
> >>           else
> >> -            RCU_INIT_POINTER(fobj->shared[j++], check);
> >> +            RCU_INIT_POINTER(new->shared[j++], fence);
> >>       }
> >> -    fobj->shared_count = j;
> >> -    RCU_INIT_POINTER(fobj->shared[fobj->shared_count], fence);
> >> -    fobj->shared_count++;
> >> +    new->shared_count = j;
> >> +    new->shared_max = max;
> >>   -done:
> >>       preempt_disable();
> >>       write_seqcount_begin(&obj->seq);
> >>       /*
> >>        * RCU_INIT_POINTER can be used here,
> >>        * seqcount provides the necessary barriers
> >>        */
> >> -    RCU_INIT_POINTER(obj->fence, fobj);
> >> +    RCU_INIT_POINTER(obj->fence, new);
> >>       write_seqcount_end(&obj->seq);
> >>       preempt_enable();
> >>         if (!old)
> >> -        return;
> >> +        return 0;
> >>         /* Drop the references to the signaled fences */
> >> -    for (i = k; i < fobj->shared_max; ++i) {
> >> -        struct dma_fence *f;
> >> +    for (i = k; i < new->shared_max; ++i) {
> >> +        struct dma_fence *fence;
> >>   -        f = rcu_dereference_protected(fobj->shared[i],
> >> -                          reservation_object_held(obj));
> >> -        dma_fence_put(f);
> >> +        fence = rcu_dereference_protected(new->shared[i],
> >> +                          reservation_object_held(obj));
> >> +        dma_fence_put(fence);
> >>       }
> >>       kfree_rcu(old, rcu);
> >> +
> >> +    return 0;
> >>   }
> >> +EXPORT_SYMBOL(reservation_object_reserve_shared);
> >>     /**
> >>    * reservation_object_add_shared_fence - Add a fence to a shared
> >> slot @@ -226,15 +143,39 @@
> >> reservation_object_add_shared_replace(struct
> >> reservation_object *obj,
> >>   void reservation_object_add_shared_fence(struct reservation_object
> >> *obj,
> >>                        struct dma_fence *fence)
> >>   {
> >> -    struct reservation_object_list *old, *fobj = obj->staged;
> >> +    struct reservation_object_list *fobj;
> >> +    unsigned int i;
> >>   -    old = reservation_object_get_list(obj);
> >> -    obj->staged = NULL;
> >> +    dma_fence_get(fence);
> >> +
> >> +    fobj = reservation_object_get_list(obj);
> >>   -    if (!fobj)
> >> -        reservation_object_add_shared_inplace(obj, old, fence);
> >> -    else
> >> -        reservation_object_add_shared_replace(obj, old, fobj,
> >> fence);
> >> +    preempt_disable();
> >> +    write_seqcount_begin(&obj->seq);
> >> +
> >> +    for (i = 0; i < fobj->shared_count; ++i) {
> >> +        struct dma_fence *old_fence;
> >> +
> >> +        old_fence = rcu_dereference_protected(fobj->shared[i],
> >> +                              reservation_object_held(obj));
> >> +        if (old_fence->context == fence->context ||
> >> +            dma_fence_is_signaled(old_fence)) {
> >> +            dma_fence_put(old_fence);
> >> +            goto replace;
> >> +        }
> >> +    }
> >> +
> >> +    BUG_ON(fobj->shared_count >= fobj->shared_max);
> >> +    fobj->shared_count++;
> >> +
> >> +replace:
> >> +    /*
> >> +     * memory barrier is added by write_seqcount_begin,
> >> +     * fobj->shared_count is protected by this lock too
> >> +     */
> >> +    RCU_INIT_POINTER(fobj->shared[i], fence);
> >> +    write_seqcount_end(&obj->seq);
> >> +    preempt_enable();
> >>   }
> >>   EXPORT_SYMBOL(reservation_object_add_shared_fence);
> >>   @@ -343,9 +284,6 @@ int reservation_object_copy_fences(struct
> >> reservation_object *dst,
> >>       new = dma_fence_get_rcu_safe(&src->fence_excl);
> >>       rcu_read_unlock();
> >>   -    kfree(dst->staged);
> >> -    dst->staged = NULL;
> >> -
> >>       src_list = reservation_object_get_list(dst);
> >>       old = reservation_object_get_excl(dst);
> >>   diff --git a/include/linux/reservation.h
> >> b/include/linux/reservation.h index 02166e815afb..54cf6773a14c 100644
> >> --- a/include/linux/reservation.h
> >> +++ b/include/linux/reservation.h
> >> @@ -68,7 +68,6 @@ struct reservation_object_list {
> >>    * @seq: sequence count for managing RCU read-side synchronization
> >>    * @fence_excl: the exclusive fence, if there is one currently
> >>    * @fence: list of current shared fences
> >> - * @staged: staged copy of shared fences for RCU updates
> >>    */
> >>   struct reservation_object {
> >>       struct ww_mutex lock;
> >> @@ -76,7 +75,6 @@ struct reservation_object {
> >>         struct dma_fence __rcu *fence_excl;
> >>       struct reservation_object_list __rcu *fence;
> >> -    struct reservation_object_list *staged;
> >>   };
> >>     #define reservation_object_held(obj)
> >> lockdep_is_held(&(obj)->lock.base)
> >> @@ -95,7 +93,6 @@ reservation_object_init(struct reservation_object
> >> *obj)
> >>       __seqcount_init(&obj->seq, reservation_seqcount_string,
> >> &reservation_seqcount_class);
> >>       RCU_INIT_POINTER(obj->fence, NULL);
> >>       RCU_INIT_POINTER(obj->fence_excl, NULL);
> >> -    obj->staged = NULL;
> >>   }
> >>     /**
> >> @@ -124,7 +121,6 @@ reservation_object_fini(struct reservation_object
> >> *obj)
> >>             kfree(fobj);
> >>       }
> >> -    kfree(obj->staged);
> >>         ww_mutex_destroy(&obj->lock);
> >>   }
> >