[PATCH] drm/i915: Prune the reservation shared fence array

[PATCH] drm/i915: Prune the reservation shared fence array

[Chris Wilson]

The shared fence array is not autopruning and may continue to grow as an
object is shared between new timelines. Take the opportunity when we
think the object is idle (we have to confirm that any external fence is
also signaled) to decouple all the fences.

We apply a similar trick after waiting on an object, see commit
e54ca9774777 ("drm/i915: Remove completed fences after a wait")

v2: No longer need to handle the batch pool as a special case.
v3: Need to trylock from within i915_vma_retire as this may be called
form the shrinker - and we may later try to allocate underneath the
reservation lock, so a deadlock is possible.

References: https://bugs.freedesktop.org/show_bug.cgi?id=102936
Fixes: d07f0e59b2c7 ("drm/i915: Move GEM activity tracking into a common struct reservation_object")
Fixes: 80b204bce8f2 ("drm/i915: Enable multiple timelines")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_vma.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c
index 7cdf34800549..b7ac84db1d2e 100644
--- a/drivers/gpu/drm/i915/i915_vma.c
+++ b/drivers/gpu/drm/i915/i915_vma.c
@@ -54,6 +54,13 @@ i915_vma_retire(struct i915_gem_active *active,
 	if (--obj->active_count)
 		return;
 
+	/* Prune the shared fence arrays iff completely idle (inc. external) */
+	if (reservation_object_trylock(obj->resv)) {
+		if (reservation_object_test_signaled_rcu(obj->resv, true))
+			reservation_object_add_excl_fence(obj->resv, NULL);
+		reservation_object_unlock(obj->resv);
+	}
+
 	/* Bump our place on the bound list to keep it roughly in LRU order
 	 * so that we don't steal from recently used but inactive objects
 	 * (unless we are forced to ofc!)
-- 
2.15.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: failure for drm/i915: Prune the reservation shared fence array (rev3)

[Patchwork]

== Series Details ==

Series: drm/i915: Prune the reservation shared fence array (rev3)
URL   : https://patchwork.freedesktop.org/series/15254/
State : failure

== Summary ==

Series 15254v3 drm/i915: Prune the reservation shared fence array
https://patchwork.freedesktop.org/api/1.0/series/15254/revisions/3/mbox/

Test gem_exec_reloc:
        Subgroup basic-gtt-read:
                pass       -> INCOMPLETE (fi-byt-j1900)
        Subgroup basic-cpu-active:
                pass       -> FAIL       (fi-gdg-551) fdo#102582 +2

fdo#102582 https://bugs.freedesktop.org/show_bug.cgi?id=102582

fi-bdw-5557u     total:289  pass:268  dwarn:0   dfail:0   fail:0   skip:21  time:443s
fi-bdw-gvtdvm    total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:452s
fi-blb-e6850     total:289  pass:223  dwarn:1   dfail:0   fail:0   skip:65  time:380s
fi-bsw-n3050     total:289  pass:243  dwarn:0   dfail:0   fail:0   skip:46  time:541s
fi-bwr-2160      total:289  pass:183  dwarn:0   dfail:0   fail:0   skip:106 time:274s
fi-bxt-dsi       total:289  pass:259  dwarn:0   dfail:0   fail:0   skip:30  time:501s
fi-bxt-j4205     total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:500s
fi-byt-j1900     total:86   pass:67   dwarn:0   dfail:0   fail:0   skip:18 
fi-byt-n2820     total:289  pass:250  dwarn:0   dfail:0   fail:0   skip:39  time:490s
fi-elk-e7500     total:289  pass:229  dwarn:0   dfail:0   fail:0   skip:60  time:425s
fi-gdg-551       total:289  pass:175  dwarn:1   dfail:0   fail:4   skip:109 time:262s
fi-glk-1         total:289  pass:261  dwarn:0   dfail:0   fail:0   skip:28  time:539s
fi-glk-dsi       total:289  pass:258  dwarn:0   dfail:0   fail:1   skip:30  time:491s
fi-hsw-4770      total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:436s
fi-hsw-4770r     total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:431s
fi-ilk-650       total:289  pass:228  dwarn:0   dfail:0   fail:0   skip:61  time:429s
fi-ivb-3520m     total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:487s
fi-ivb-3770      total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:457s
fi-kbl-7500u     total:289  pass:263  dwarn:1   dfail:0   fail:1   skip:24  time:476s
fi-kbl-7567u     total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:472s
fi-kbl-r         total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:531s
fi-pnv-d510      total:289  pass:222  dwarn:1   dfail:0   fail:0   skip:66  time:565s
fi-skl-6260u     total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:457s
fi-skl-6600u     total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:543s
fi-skl-6700hq    total:289  pass:263  dwarn:0   dfail:0   fail:0   skip:26  time:560s
fi-skl-6700k     total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:519s
fi-skl-6770hq    total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:489s
fi-skl-gvtdvm    total:289  pass:266  dwarn:0   dfail:0   fail:0   skip:23  time:460s
fi-snb-2520m     total:289  pass:250  dwarn:0   dfail:0   fail:0   skip:39  time:558s
fi-snb-2600      total:289  pass:249  dwarn:0   dfail:0   fail:0   skip:40  time:417s
fi-cnl-y failed to connect after reboot

748f2c6b4046b23a623b4af3799563ef3110bb0d drm-tip: 2017y-11m-08d-07h-50m-13s UTC integration manifest
c9d1525b26e4 drm/i915: Prune the reservation shared fence array

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7004/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Prune the reservation shared fence array

[Joonas Lahtinen]

On Tue, 2017-11-07 at 22:06 +0000, Chris Wilson wrote:
> The shared fence array is not autopruning and may continue to grow as an
> object is shared between new timelines. Take the opportunity when we
> think the object is idle (we have to confirm that any external fence is
> also signaled) to decouple all the fences.
> 
> We apply a similar trick after waiting on an object, see commit
> e54ca9774777 ("drm/i915: Remove completed fences after a wait")
> 
> v2: No longer need to handle the batch pool as a special case.
> v3: Need to trylock from within i915_vma_retire as this may be called
> form the shrinker - and we may later try to allocate underneath the
> reservation lock, so a deadlock is possible.
> 
> References: https://bugs.freedesktop.org/show_bug.cgi?id=102936
> Fixes: d07f0e59b2c7 ("drm/i915: Move GEM activity tracking into a common struct reservation_object")
> Fixes: 80b204bce8f2 ("drm/i915: Enable multiple timelines")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

<SNIP>

> +++ b/drivers/gpu/drm/i915/i915_vma.c
> @@ -54,6 +54,13 @@ i915_vma_retire(struct i915_gem_active *active,
>  	if (--obj->active_count)
>  		return;
>  
> +	/* Prune the shared fence arrays iff completely idle (inc. external) */
> +	if (reservation_object_trylock(obj->resv)) {
> +		if (reservation_object_test_signaled_rcu(obj->resv, true))
> +			reservation_object_add_excl_fence(obj->resv, NULL);
> +		reservation_object_unlock(obj->resv);
> +	}

Feels bit like this could also be a feature of reservation objects.

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Prune the reservation shared fence array

[Chris Wilson]

Quoting Joonas Lahtinen (2017-11-08 15:09:47)
> On Tue, 2017-11-07 at 22:06 +0000, Chris Wilson wrote:
> > The shared fence array is not autopruning and may continue to grow as an
> > object is shared between new timelines. Take the opportunity when we
> > think the object is idle (we have to confirm that any external fence is
> > also signaled) to decouple all the fences.
> > 
> > We apply a similar trick after waiting on an object, see commit
> > e54ca9774777 ("drm/i915: Remove completed fences after a wait")
> > 
> > v2: No longer need to handle the batch pool as a special case.
> > v3: Need to trylock from within i915_vma_retire as this may be called
> > form the shrinker - and we may later try to allocate underneath the
> > reservation lock, so a deadlock is possible.
> > 
> > References: https://bugs.freedesktop.org/show_bug.cgi?id=102936
> > Fixes: d07f0e59b2c7 ("drm/i915: Move GEM activity tracking into a common struct reservation_object")
> > Fixes: 80b204bce8f2 ("drm/i915: Enable multiple timelines")
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> 
> <SNIP>
> 
> > +++ b/drivers/gpu/drm/i915/i915_vma.c
> > @@ -54,6 +54,13 @@ i915_vma_retire(struct i915_gem_active *active,
> >       if (--obj->active_count)
> >               return;
> >  
> > +     /* Prune the shared fence arrays iff completely idle (inc. external) */
> > +     if (reservation_object_trylock(obj->resv)) {
> > +             if (reservation_object_test_signaled_rcu(obj->resv, true))
> > +                     reservation_object_add_excl_fence(obj->resv, NULL);
> > +             reservation_object_unlock(obj->resv);
> > +     }
> 
> Feels bit like this could also be a feature of reservation objects.

Yeah, we shouldn't need it so badly when the "don't keep signaled
fences in the resv.object" lands. Until then, it's quite easy to tie up
large chunks of kernel memory via stale fences, e.g. gem_ctx_thrash.

When the improvement to resv.object lands, it will still be wise to keep
this around to free the residual fences -- it just won't be as large as
an impact!
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Prune the reservation shared fence array

[Chris Wilson]

On Tue, Nov 15, 2016 at 04:38:19PM +0200, Joonas Lahtinen wrote:
> On ma, 2016-11-14 at 08:53 +0000, Chris Wilson wrote:
> > +++ b/drivers/gpu/drm/i915/i915_vma.c
> > @@ -53,6 +53,12 @@ i915_vma_retire(struct i915_gem_active *active,
> >  	if (--obj->active_count)
> >  		return;
> >  
> > +	/* Prune the shared fence arrays iff completely idle (inc. external) */
> > +	ww_mutex_lock(&obj->resv->lock, NULL);
> > +	if (reservation_object_test_signaled_rcu(obj->resv, true))
> > +		reservation_object_add_excl_fence(obj->resv, NULL);
> > +	ww_mutex_unlock(&obj->resv->lock);
> > +
> 
> This is not the first instance of "resv->lock" usage, but I think we
> should not be doing that, and add reservation_* functions instead...

It's also a bit meh:


diff --git a/include/linux/reservation.h b/include/linux/reservation.h
index ed238961e1bf..4dcbdbba0ed1 100644
--- a/include/linux/reservation.h
+++ b/include/linux/reservation.h
@@ -129,6 +129,35 @@ reservation_object_fini(struct reservation_object *obj)
 }
 
 /**
+ * reservation_object_lock - lock the reservation object
+ * @obj: the reservation object
+ * @ctx: the locking context
+ *
+ * Locks the reservation object for exclusive access and modification.
+ * As the reservation object may be locked by multiple parties in an
+ * undefined order, a #ww_acquire_ctx is passed to unwind if a cycle
+ * is detected. See ww_mutex_lock() and ww_acquire_init()
+ */
+static inline int
+reservation_object_lock(struct reservation_object *obj,
+                       struct ww_acquire_ctx *ctx)
+{
+       return ww_mutex_lock(obj->lock, ctx);
+}
:...skipping...
diff --git a/include/linux/reservation.h b/include/linux/reservation.h
index ed238961e1bf..4dcbdbba0ed1 100644
--- a/include/linux/reservation.h
+++ b/include/linux/reservation.h
@@ -129,6 +129,35 @@ reservation_object_fini(struct reservation_object *obj)
 }
 
 /**
+ * reservation_object_lock - lock the reservation object
+ * @obj: the reservation object
+ * @ctx: the locking context
+ *
+ * Locks the reservation object for exclusive access and modification.
+ * As the reservation object may be locked by multiple parties in an
+ * undefined order, a #ww_acquire_ctx is passed to unwind if a cycle
+ * is detected. See ww_mutex_lock() and ww_acquire_init()
+ */
+static inline int
+reservation_object_lock(struct reservation_object *obj,
+                       struct ww_acquire_ctx *ctx)
+{
+       return ww_mutex_lock(obj->lock, ctx);
+}
+
+/**
+ * reservation_object_unlock - unlock the reservation object
+ * @obj: the reservation object
+ *
+ * Unlocks the reservation object following exclusive access.
+ */
+static inline void
+reservation_object_unlock(struct reservation_object *obj)
+{
+       ww_mutex_unlock(obj->lock);
+}
+
+/**
  * reservation_object_get_excl - get the reservation object's
  * exclusive fence, with update-side lock held
  * @obj: the reservation object


Nothing much is gained over using ww_mutex_lock(). Especially in the more
complicated multiple lock scenarios.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Prune the reservation shared fence array

[Chris Wilson]

On Tue, Nov 15, 2016 at 04:38:19PM +0200, Joonas Lahtinen wrote:
> On ma, 2016-11-14 at 08:53 +0000, Chris Wilson wrote:
> > +++ b/drivers/gpu/drm/i915/i915_vma.c
> > @@ -53,6 +53,12 @@ i915_vma_retire(struct i915_gem_active *active,
> >  	if (--obj->active_count)
> >  		return;
> >  
> > +	/* Prune the shared fence arrays iff completely idle (inc. external) */
> > +	ww_mutex_lock(&obj->resv->lock, NULL);
> > +	if (reservation_object_test_signaled_rcu(obj->resv, true))
> > +		reservation_object_add_excl_fence(obj->resv, NULL);
> > +	ww_mutex_unlock(&obj->resv->lock);
> > +
> 
> This is not the first instance of "resv->lock" usage, but I think we
> should not be doing that, and add reservation_* functions instead...

But in the short term wrt to how we consume many, many more megabytes
than we used to in gem_ctx_*....
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Prune the reservation shared fence array

[Joonas Lahtinen]

On ma, 2016-11-14 at 08:53 +0000, Chris Wilson wrote:
> +++ b/drivers/gpu/drm/i915/i915_vma.c
> @@ -53,6 +53,12 @@ i915_vma_retire(struct i915_gem_active *active,
>  	if (--obj->active_count)
>  		return;
>  
> +	/* Prune the shared fence arrays iff completely idle (inc. external) */
> +	ww_mutex_lock(&obj->resv->lock, NULL);
> +	if (reservation_object_test_signaled_rcu(obj->resv, true))
> +		reservation_object_add_excl_fence(obj->resv, NULL);
> +	ww_mutex_unlock(&obj->resv->lock);
> +

This is not the first instance of "resv->lock" usage, but I think we
should not be doing that, and add reservation_* functions instead...

Regards, Joonas

-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH] drm/i915: Prune the reservation shared fence array

[Chris Wilson]

The shared fence array is not autopruning and may continue to grow as an
object is shared between new timelines. Take the opportunity when we
think the object is idle (we have to confirm that any external fence is
also signaled) to decouple all the fences.

Fixes: d07f0e59b2c7 ("drm/i915: Move GEM activity tracking into a common struct reservation_object")
Fixes: 80b204bce8f2 ("drm/i915: Enable multiple timelines")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_vma.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c
index 6d456582edf4..44585300fdff 100644
--- a/drivers/gpu/drm/i915/i915_vma.c
+++ b/drivers/gpu/drm/i915/i915_vma.c
@@ -53,6 +53,12 @@ i915_vma_retire(struct i915_gem_active *active,
 	if (--obj->active_count)
 		return;
 
+	/* Prune the shared fence arrays iff completely idle (inc. external) */
+	ww_mutex_lock(&obj->resv->lock, NULL);
+	if (reservation_object_test_signaled_rcu(obj->resv, true))
+		reservation_object_add_excl_fence(obj->resv, NULL);
+	ww_mutex_unlock(&obj->resv->lock);
+
 	/* Bump our place on the bound list to keep it roughly in LRU order
 	 * so that we don't steal from recently used but inactive objects
 	 * (unless we are forced to ofc!)
-- 
2.10.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx