[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.

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

✗ Fi.CI.BAT: warning for drm/i915: Prune the reservation shared fence array

[Patchwork]

== Series Details ==

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

== Summary ==

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

Test drv_module_reload_basic:
                pass       -> DMESG-WARN (fi-skl-6770hq)

fi-bdw-5557u     total:244  pass:229  dwarn:0   dfail:0   fail:0   skip:15 
fi-bsw-n3050     total:244  pass:204  dwarn:0   dfail:0   fail:0   skip:40 
fi-byt-j1900     total:244  pass:216  dwarn:0   dfail:0   fail:0   skip:28 
fi-byt-n2820     total:244  pass:212  dwarn:0   dfail:0   fail:0   skip:32 
fi-hsw-4770      total:244  pass:224  dwarn:0   dfail:0   fail:0   skip:20 
fi-hsw-4770r     total:244  pass:224  dwarn:0   dfail:0   fail:0   skip:20 
fi-ilk-650       total:244  pass:191  dwarn:0   dfail:0   fail:0   skip:53 
fi-ivb-3520m     total:244  pass:222  dwarn:0   dfail:0   fail:0   skip:22 
fi-ivb-3770      total:244  pass:222  dwarn:0   dfail:0   fail:0   skip:22 
fi-kbl-7200u     total:244  pass:222  dwarn:0   dfail:0   fail:0   skip:22 
fi-skl-6260u     total:244  pass:230  dwarn:0   dfail:0   fail:0   skip:14 
fi-skl-6700hq    total:244  pass:223  dwarn:0   dfail:0   fail:0   skip:21 
fi-skl-6700k     total:244  pass:222  dwarn:1   dfail:0   fail:0   skip:21 
fi-skl-6770hq    total:244  pass:229  dwarn:1   dfail:0   fail:0   skip:14 
fi-snb-2520m     total:244  pass:212  dwarn:0   dfail:0   fail:0   skip:32 
fi-snb-2600      total:244  pass:211  dwarn:0   dfail:0   fail:0   skip:33 

020e291d72d4aaf3e0f8f5168a60ac05daa43a77 drm-intel-nightly: 2016y-11m-14d-08h-02m-22s UTC integration manifest
b04bdd5 drm/i915: Prune the reservation shared fence array

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_2977/
_______________________________________________
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

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

[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

[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

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