[1/2] dma-buf: make reservation_object_copy_fences rcu save

Commit Message

Christian König Sept. 4, 2017, 7:02 p.m. UTC

From: Christian König <[e-mail]>

Stop requiring that the src reservation object is locked for this operation.

Signed-off-by: Christian König <[e-mail]>
---
 drivers/dma-buf/reservation.c | 56 ++++++++++++++++++++++++++++++++-----------
 1 file changed, 42 insertions(+), 14 deletions(-)

Comments

Christian König Sept. 7, 2017, 7:13 a.m. UTC | #1

Ping? David can you take a look?

Alex is on vacation and that is a rather important bug fix.

Thanks,
Christian.

Am 04.09.2017 um 21:02 schrieb Christian König:
> From: Christian König <[e-mail]>
>
> Stop requiring that the src reservation object is locked for this operation.
>
> Signed-off-by: Christian König <[e-mail]>
> ---
>   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)
> @@ -277,33 +276,62 @@ int reservation_object_copy_fences(struct reservation_object *dst,
>   	size_t size;
>   	unsigned i;
>   
> -	src_list = reservation_object_get_list(src);
> +	rcu_read_lock();
> +	src_list = rcu_dereference(src->fence);
>   
> +retry:
>   	if (src_list) {
> -		size = offsetof(typeof(*src_list),
> -				shared[src_list->shared_count]);
> +		unsigned shared_count = src_list->shared_count;
> +
> +		size = offsetof(typeof(*src_list), shared[shared_count]);
> +		rcu_read_unlock();
> +
>   		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]);
> +		rcu_read_lock();
> +		src_list = rcu_dereference(src->fence);
> +		if (!src_list || src_list->shared_count > shared_count) {
> +			kfree(dst_list);
> +			goto retry;
> +		}
> +
> +		dst_list->shared_count = 0;
> +		dst_list->shared_max = shared_count;
> +		for (i = 0; i < src_list->shared_count; ++i) {
> +			struct dma_fence *fence;
> +
> +			fence = rcu_dereference(src_list->shared[i]);
> +			if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT,
> +				     &fence->flags))
> +				continue;
> +
> +			if (!dma_fence_get_rcu(fence)) {
> +				kfree(dst_list);
> +				src_list = rcu_dereference(src->fence);
> +				goto retry;
> +			}
> +
> +			if (dma_fence_is_signaled(fence)) {
> +				dma_fence_put(fence);
> +				continue;
> +			}
> +
> +			dst_list->shared[dst_list->shared_count++] = fence;
> +		}
>   	} else {
>   		dst_list = NULL;
>   	}
>   
> +	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);
> -	new = reservation_object_get_excl(src);
> -
> -	dma_fence_get(new);
>   
>   	preempt_disable();
>   	write_seqcount_begin(&dst->seq);

Chunming Zhou Sept. 7, 2017, 7:24 a.m. UTC | #2

On 2017年09月07日 15:13, Christian König wrote:
> Ping? David can you take a look?
>
> Alex is on vacation and that is a rather important bug fix.
>
> Thanks,
> Christian.
>
> Am 04.09.2017 um 21:02 schrieb Christian König:
>> From: Christian König <[e-mail]>
>>
>> Stop requiring that the src reservation object is locked for this 
>> operation.
>>
>> Signed-off-by: Christian König <[e-mail]>
>> ---
>>   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)
>> @@ -277,33 +276,62 @@ int reservation_object_copy_fences(struct 
>> reservation_object *dst,
>>       size_t size;
>>       unsigned i;
>>   -    src_list = reservation_object_get_list(src);
>> +    rcu_read_lock();
>> +    src_list = rcu_dereference(src->fence);
>>   +retry:
>>       if (src_list) {
>> -        size = offsetof(typeof(*src_list),
>> -                shared[src_list->shared_count]);
>> +        unsigned shared_count = src_list->shared_count;
>> +
>> +        size = offsetof(typeof(*src_list), shared[shared_count]);
>> +        rcu_read_unlock();
>> +
>>           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]);
>> +        rcu_read_lock();
>> +        src_list = rcu_dereference(src->fence);
>> +        if (!src_list || src_list->shared_count > shared_count) {
>> +            kfree(dst_list);
>> +            goto retry;
>> +        }
>> +
>> +        dst_list->shared_count = 0;
>> +        dst_list->shared_max = shared_count;
>> +        for (i = 0; i < src_list->shared_count; ++i) {
>> +            struct dma_fence *fence;
>> +
>> +            fence = rcu_dereference(src_list->shared[i]);
>> +            if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT,
>> +                     &fence->flags))
seems here is duplicated with the below dma_fence_is_signaled, can it be 
removed?

And I'm not sure the locking, but it looks good, so Acked-by: Chunming 
Zhou <[e-mail]>
>> +                continue;
>> +
>> +            if (!dma_fence_get_rcu(fence)) {
>> +                kfree(dst_list);
>> +                src_list = rcu_dereference(src->fence);
>> +                goto retry;
>> +            }
>> +
>> +            if (dma_fence_is_signaled(fence)) {
>> +                dma_fence_put(fence);
>> +                continue;
>> +            }
>> +
>> +            dst_list->shared[dst_list->shared_count++] = fence;
>> +        }
>>       } else {
>>           dst_list = NULL;
>>       }
>>   +    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);
>> -    new = reservation_object_get_excl(src);
>> -
>> -    dma_fence_get(new);
>>         preempt_disable();
>>       write_seqcount_begin(&dst->seq);
>
>

Daniel Vetter Sept. 8, 2017, 6:17 a.m. UTC | #3

On Thu, Sep 07, 2017 at 09:13:32AM +0200, Christian König wrote:
> Ping? David can you take a look?
> 
> Alex is on vacation and that is a rather important bug fix.

Works better when you cc Gustavo/Maarten/Sumits/Chris I think, for
anything dma-buf review needing.
-Daniel

> 
> Thanks,
> Christian.
> 
> Am 04.09.2017 um 21:02 schrieb Christian König:
> > From: Christian König <[e-mail]>
> > 
> > Stop requiring that the src reservation object is locked for this operation.
> > 
> > Signed-off-by: Christian König <[e-mail]>
> > ---
> >   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)
> > @@ -277,33 +276,62 @@ int reservation_object_copy_fences(struct reservation_object *dst,
> >   	size_t size;
> >   	unsigned i;
> > -	src_list = reservation_object_get_list(src);
> > +	rcu_read_lock();
> > +	src_list = rcu_dereference(src->fence);
> > +retry:
> >   	if (src_list) {
> > -		size = offsetof(typeof(*src_list),
> > -				shared[src_list->shared_count]);
> > +		unsigned shared_count = src_list->shared_count;
> > +
> > +		size = offsetof(typeof(*src_list), shared[shared_count]);
> > +		rcu_read_unlock();
> > +
> >   		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]);
> > +		rcu_read_lock();
> > +		src_list = rcu_dereference(src->fence);
> > +		if (!src_list || src_list->shared_count > shared_count) {
> > +			kfree(dst_list);
> > +			goto retry;
> > +		}
> > +
> > +		dst_list->shared_count = 0;
> > +		dst_list->shared_max = shared_count;
> > +		for (i = 0; i < src_list->shared_count; ++i) {
> > +			struct dma_fence *fence;
> > +
> > +			fence = rcu_dereference(src_list->shared[i]);
> > +			if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT,
> > +				     &fence->flags))
> > +				continue;
> > +
> > +			if (!dma_fence_get_rcu(fence)) {
> > +				kfree(dst_list);
> > +				src_list = rcu_dereference(src->fence);
> > +				goto retry;
> > +			}
> > +
> > +			if (dma_fence_is_signaled(fence)) {
> > +				dma_fence_put(fence);
> > +				continue;
> > +			}
> > +
> > +			dst_list->shared[dst_list->shared_count++] = fence;
> > +		}
> >   	} else {
> >   		dst_list = NULL;
> >   	}
> > +	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);
> > -	new = reservation_object_get_excl(src);
> > -
> > -	dma_fence_get(new);
> >   	preempt_disable();
> >   	write_seqcount_begin(&dst->seq);
> 
> 
> _______________________________________________
> dri-devel mailing list
> [e-mail]
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Maarten Lankhorst Sept. 10, 2017, 7:30 a.m. UTC | #4

Op 04-09-17 om 21:02 schreef Christian König:
> From: Christian König <[e-mail]>
>
> Stop requiring that the src reservation object is locked for this operation.
>
> Signed-off-by: Christian König <[e-mail]>
> ---
>  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.

Cheers,
Maarten

Christian König Sept. 11, 2017, 12:53 p.m. UTC | #5

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 <[e-mail]>
>>
>> Stop requiring that the src reservation object is locked for this operation.
>>
>> Signed-off-by: Christian König <[e-mail]>
>> ---
>>   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.

Regards,
Christian.

>
> Cheers,
> Maarten
> _______________________________________________
> amd-gfx mailing list
> [e-mail]
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Patch

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)
@@ -277,33 +276,62 @@  int reservation_object_copy_fences(struct reservation_object *dst,
 	size_t size;
 	unsigned i;
 
-	src_list = reservation_object_get_list(src);
+	rcu_read_lock();
+	src_list = rcu_dereference(src->fence);
 
+retry:
 	if (src_list) {
-		size = offsetof(typeof(*src_list),
-				shared[src_list->shared_count]);
+		unsigned shared_count = src_list->shared_count;
+
+		size = offsetof(typeof(*src_list), shared[shared_count]);
+		rcu_read_unlock();
+
 		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]);
+		rcu_read_lock();
+		src_list = rcu_dereference(src->fence);
+		if (!src_list || src_list->shared_count > shared_count) {
+			kfree(dst_list);
+			goto retry;
+		}
+
+		dst_list->shared_count = 0;
+		dst_list->shared_max = shared_count;
+		for (i = 0; i < src_list->shared_count; ++i) {
+			struct dma_fence *fence;
+
+			fence = rcu_dereference(src_list->shared[i]);
+			if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT,
+				     &fence->flags))
+				continue;
+
+			if (!dma_fence_get_rcu(fence)) {
+				kfree(dst_list);
+				src_list = rcu_dereference(src->fence);
+				goto retry;
+			}
+
+			if (dma_fence_is_signaled(fence)) {
+				dma_fence_put(fence);
+				continue;
+			}
+
+			dst_list->shared[dst_list->shared_count++] = fence;
+		}
 	} else {
 		dst_list = NULL;
 	}
 
+	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);
-	new = reservation_object_get_excl(src);
-
-	dma_fence_get(new);
 
 	preempt_disable();
 	write_seqcount_begin(&dst->seq);