[PATCH 01/23] drm/i915: Drop vm.ref for duplicate vma on construction

[PATCH 01/23] drm/i915: Drop vm.ref for duplicate vma on construction

[Chris Wilson]

As we allow for parallel threads to create vma instances in parallel,
and we only filter out the duplicates upon reacquiring the spinlock for
the rbtree, we have to free the loser of the constructors' race. When
freeing, we should also drop any resource references acquired for the
redundant vma.

Fixes: 2850748ef876 ("drm/i915: Pull i915_vma_pin under the vm->mutex")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: <stable@vger.kernel.org> # v5.5+
---
 drivers/gpu/drm/i915/i915_vma.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c
index 1f63c4a1f055..7fe1f317cd2b 100644
--- a/drivers/gpu/drm/i915/i915_vma.c
+++ b/drivers/gpu/drm/i915/i915_vma.c
@@ -198,6 +198,7 @@ vma_create(struct drm_i915_gem_object *obj,
 		cmp = i915_vma_compare(pos, vm, view);
 		if (cmp == 0) {
 			spin_unlock(&obj->vma.lock);
+			i915_vm_put(vm);
 			i915_vma_free(vma);
 			return pos;
 		}
-- 
2.20.1

Re: [Intel-gfx] [PATCH 01/23] drm/i915: Drop vm.ref for duplicate vma on construction

[Tvrtko Ursulin]

On 02/07/2020 09:32, Chris Wilson wrote:
> As we allow for parallel threads to create vma instances in parallel,
> and we only filter out the duplicates upon reacquiring the spinlock for
> the rbtree, we have to free the loser of the constructors' race. When
> freeing, we should also drop any resource references acquired for the
> redundant vma.
> 
> Fixes: 2850748ef876 ("drm/i915: Pull i915_vma_pin under the vm->mutex")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: <stable@vger.kernel.org> # v5.5+
> ---
>   drivers/gpu/drm/i915/i915_vma.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c
> index 1f63c4a1f055..7fe1f317cd2b 100644
> --- a/drivers/gpu/drm/i915/i915_vma.c
> +++ b/drivers/gpu/drm/i915/i915_vma.c
> @@ -198,6 +198,7 @@ vma_create(struct drm_i915_gem_object *obj,
>   		cmp = i915_vma_compare(pos, vm, view);
>   		if (cmp == 0) {
>   			spin_unlock(&obj->vma.lock);
> +			i915_vm_put(vm);
>   			i915_vma_free(vma);
>   			return pos;
>   		}
> 

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

Tvrtko

Re: [Intel-gfx] [PATCH 01/23] drm/i915: Drop vm.ref for duplicate vma on construction

[Andi Shyti]

Hi Chris,

> diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c
> index 1f63c4a1f055..7fe1f317cd2b 100644
> --- a/drivers/gpu/drm/i915/i915_vma.c
> +++ b/drivers/gpu/drm/i915/i915_vma.c
> @@ -198,6 +198,7 @@ vma_create(struct drm_i915_gem_object *obj,
>  		cmp = i915_vma_compare(pos, vm, view);
>  		if (cmp == 0) {
>  			spin_unlock(&obj->vma.lock);
> +			i915_vm_put(vm);
>  			i915_vma_free(vma);

You are forgettin one return without dereferencing it.

would this be a solution:

@@ -106,6 +106,7 @@ vma_create(struct drm_i915_gem_object *obj,
 {
        struct i915_vma *vma;
        struct rb_node *rb, **p;
+       struct i915_vma *pos = ERR_PTR(-E2BIG);
 
        /* The aliasing_ppgtt should never be used directly! */
        GEM_BUG_ON(vm == &vm->gt->ggtt->alias->vm);
@@ -185,7 +186,6 @@ vma_create(struct drm_i915_gem_object *obj,
        rb = NULL;
        p = &obj->vma.tree.rb_node;
        while (*p) {
-               struct i915_vma *pos;
                long cmp;
 
                rb = *p;
@@ -197,12 +197,8 @@ vma_create(struct drm_i915_gem_object *obj,
                 * and dispose of ours.
                 */
                cmp = i915_vma_compare(pos, vm, view);
-               if (cmp == 0) {
-                       spin_unlock(&obj->vma.lock);
-                       i915_vm_put(vm);
-                       i915_vma_free(vma);
-                       return pos;
-               }
+               if (!cmp)
+                       goto err_unlock;
 
                if (cmp < 0)
                        p = &rb->rb_right;
@@ -230,8 +226,9 @@ vma_create(struct drm_i915_gem_object *obj,
 err_unlock:
        spin_unlock(&obj->vma.lock);
 err_vma:
+       i915_vm_put(vm);
        i915_vma_free(vma);
-       return ERR_PTR(-E2BIG);
+       return pos;
 }

Andi

Re: [Intel-gfx] [PATCH 01/23] drm/i915: Drop vm.ref for duplicate vma on construction

[Chris Wilson]

Quoting Andi Shyti (2020-07-02 21:25:45)
> Hi Chris,
> 
> > diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c
> > index 1f63c4a1f055..7fe1f317cd2b 100644
> > --- a/drivers/gpu/drm/i915/i915_vma.c
> > +++ b/drivers/gpu/drm/i915/i915_vma.c
> > @@ -198,6 +198,7 @@ vma_create(struct drm_i915_gem_object *obj,
> >               cmp = i915_vma_compare(pos, vm, view);
> >               if (cmp == 0) {
> >                       spin_unlock(&obj->vma.lock);
> > +                     i915_vm_put(vm);
> >                       i915_vma_free(vma);
> 
> You are forgettin one return without dereferencing it.
> 
> would this be a solution:
> 
> @@ -106,6 +106,7 @@ vma_create(struct drm_i915_gem_object *obj,
>  {
>         struct i915_vma *vma;
>         struct rb_node *rb, **p;
> +       struct i915_vma *pos = ERR_PTR(-E2BIG);
>  
>         /* The aliasing_ppgtt should never be used directly! */
>         GEM_BUG_ON(vm == &vm->gt->ggtt->alias->vm);
> @@ -185,7 +186,6 @@ vma_create(struct drm_i915_gem_object *obj,
>         rb = NULL;
>         p = &obj->vma.tree.rb_node;
>         while (*p) {
> -               struct i915_vma *pos;
>                 long cmp;
>  
>                 rb = *p;
> @@ -197,12 +197,8 @@ vma_create(struct drm_i915_gem_object *obj,
>                  * and dispose of ours.
>                  */
>                 cmp = i915_vma_compare(pos, vm, view);
> -               if (cmp == 0) {
> -                       spin_unlock(&obj->vma.lock);
> -                       i915_vm_put(vm);
> -                       i915_vma_free(vma);
> -                       return pos;
> -               }
> +               if (!cmp)
> +                       goto err_unlock;

Yeah, but you might as well do

if (cmp < 0)
 	p = right;
else if (cmp > 0)
 	p = left;
else
	goto err_unlock;
 
>                 if (cmp < 0)
>                         p = &rb->rb_right;
> @@ -230,8 +226,9 @@ vma_create(struct drm_i915_gem_object *obj,
>  err_unlock:
>         spin_unlock(&obj->vma.lock);
>  err_vma:
> +       i915_vm_put(vm);
>         i915_vma_free(vma);
> -       return ERR_PTR(-E2BIG);
> +       return pos;
>  }
> 
> Andi

Ta, going to send that as a patch?
-Chris

Re: [Intel-gfx] [PATCH 01/23] drm/i915: Drop vm.ref for duplicate vma on construction

[Andi Shyti]

Hi Chris,

> Ta, going to send that as a patch?

mine was a suggestion, it was easier to build the diff than
explain myself :)

If you want I can send it, though.

Andi