[PATCH] dma-buf: fix reservation_object_wait_timeout_rcu once more v2

[PATCH] dma-buf: fix reservation_object_wait_timeout_rcu once more v2

[Christian König]

We need to set shared_count even if we already have a fence to wait for.

v2: init i to -1 as well

Signed-off-by: Christian König <christian.koenig@amd.com>
Cc: stable@vger.kernel.org
Tested-by: Lyude Paul <lyude@redhat.com>
Reviewed-by: Lyude Paul <lyude@redhat.com>
---
 drivers/dma-buf/reservation.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/dma-buf/reservation.c b/drivers/dma-buf/reservation.c
index 461afa9febd4..314eb1071cce 100644
--- a/drivers/dma-buf/reservation.c
+++ b/drivers/dma-buf/reservation.c
@@ -484,13 +484,15 @@ long reservation_object_wait_timeout_rcu(struct reservation_object *obj,
 					 unsigned long timeout)
 {
 	struct dma_fence *fence;
-	unsigned seq, shared_count, i = 0;
+	unsigned seq, shared_count;
 	long ret = timeout ? timeout : 1;
+	int i;
 
 retry:
 	shared_count = 0;
 	seq = read_seqcount_begin(&obj->seq);
 	rcu_read_lock();
+	i = -1;
 
 	fence = rcu_dereference(obj->fence_excl);
 	if (fence && !test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)) {
@@ -506,14 +508,14 @@ long reservation_object_wait_timeout_rcu(struct reservation_object *obj,
 		fence = NULL;
 	}
 
-	if (!fence && wait_all) {
+	if (wait_all) {
 		struct reservation_object_list *fobj =
 						rcu_dereference(obj->fence);
 
 		if (fobj)
 			shared_count = fobj->shared_count;
 
-		for (i = 0; i < shared_count; ++i) {
+		for (i = 0; !fence && i < shared_count; ++i) {
 			struct dma_fence *lfence = rcu_dereference(fobj->shared[i]);
 
 			if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT,
-- 
2.14.1

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

Re: [PATCH] dma-buf: fix reservation_object_wait_timeout_rcu once more v2

[Chris Wilson]

Quoting Christian König (2018-01-22 20:00:03)
> We need to set shared_count even if we already have a fence to wait for.
> 
> v2: init i to -1 as well
> 
> Signed-off-by: Christian König <christian.koenig@amd.com>
> Cc: stable@vger.kernel.org
> Tested-by: Lyude Paul <lyude@redhat.com>
> Reviewed-by: Lyude Paul <lyude@redhat.com>
> ---
>  drivers/dma-buf/reservation.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/dma-buf/reservation.c b/drivers/dma-buf/reservation.c
> index 461afa9febd4..314eb1071cce 100644
> --- a/drivers/dma-buf/reservation.c
> +++ b/drivers/dma-buf/reservation.c
> @@ -484,13 +484,15 @@ long reservation_object_wait_timeout_rcu(struct reservation_object *obj,
>                                          unsigned long timeout)
>  {
>         struct dma_fence *fence;
> -       unsigned seq, shared_count, i = 0;
> +       unsigned seq, shared_count;
>         long ret = timeout ? timeout : 1;
> +       int i;
>  
>  retry:
>         shared_count = 0;
>         seq = read_seqcount_begin(&obj->seq);
>         rcu_read_lock();
> +       i = -1;

Could be before the seqlock, but

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

Are we at the point where just grabbing the snapshot of fences with your
new get_fences_rcu() returning a single array will be simpler? (It also
has the change in behaviour of not updating the snapshot across the long
lived wait.)

If you want to avoid the kmalloc, we could teach it populate the
caller's stack allocated array first.
-Chris
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] dma-buf: fix reservation_object_wait_timeout_rcu once more v2

[Christian König]

Am 22.01.2018 um 21:09 schrieb Chris Wilson:
> Quoting Christian König (2018-01-22 20:00:03)
>> We need to set shared_count even if we already have a fence to wait for.
>>
>> v2: init i to -1 as well
>>
>> Signed-off-by: Christian König <christian.koenig@amd.com>
>> Cc: stable@vger.kernel.org
>> Tested-by: Lyude Paul <lyude@redhat.com>
>> Reviewed-by: Lyude Paul <lyude@redhat.com>
>> ---
>>   drivers/dma-buf/reservation.c | 8 +++++---
>>   1 file changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/dma-buf/reservation.c b/drivers/dma-buf/reservation.c
>> index 461afa9febd4..314eb1071cce 100644
>> --- a/drivers/dma-buf/reservation.c
>> +++ b/drivers/dma-buf/reservation.c
>> @@ -484,13 +484,15 @@ long reservation_object_wait_timeout_rcu(struct reservation_object *obj,
>>                                           unsigned long timeout)
>>   {
>>          struct dma_fence *fence;
>> -       unsigned seq, shared_count, i = 0;
>> +       unsigned seq, shared_count;
>>          long ret = timeout ? timeout : 1;
>> +       int i;
>>   
>>   retry:
>>          shared_count = 0;
>>          seq = read_seqcount_begin(&obj->seq);
>>          rcu_read_lock();
>> +       i = -1;
> Could be before the seqlock, but
>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
>
> Are we at the point where just grabbing the snapshot of fences with your
> new get_fences_rcu() returning a single array will be simpler? (It also
> has the change in behaviour of not updating the snapshot across the long
> lived wait.)

Yeah, thought about that as well.

> If you want to avoid the kmalloc, we could teach it populate the
> caller's stack allocated array first.

Something like a helper function which just grabs all fences and puts 
them in an array of size n? And if the array is to small we return 
-ENOSPC and the caller needs to resize it.

Good idea, putting this on my TODO list, but no idea when I can get 
around to actually doing it.

Christian.

> -Chris

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