[PATCH 1/2] drm/i915: Use our singlethreaded wq for freeing objects

[PATCH 1/2] drm/i915: Use our singlethreaded wq for freeing objects

[Chris Wilson]

As freeing the objects require serialisation on struct_mutex, we should
prefer to use our singlethreaded driver wq that is dedicated to work
requiring struct_mutex (hence serialised).The benefit should be less
clutter on the system wq, allowing it to make progress even when the
driver/struct_mutex is heavily contended.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 1135a77b383a..87937c4f9dff 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4732,7 +4732,7 @@ static void __i915_gem_free_object_rcu(struct rcu_head *head)
 	 * detour through a worker.
 	 */
 	if (llist_add(&obj->freed, &i915->mm.free_list))
-		schedule_work(&i915->mm.free_work);
+		queue_work(i915->wq, &i915->mm.free_work);
 }
 
 void i915_gem_free_object(struct drm_gem_object *gem_obj)
-- 
2.15.1

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

[PATCH 2/2] drm/i915: Only attempt to scan the requested number of shrinker slabs

[Chris Wilson]

Since commit 4e773c3a8a69 ("drm/i915: Wire up shrinkctl->nr_scanned"),
we track the number of objects we scan and do not wish to exceed that as
it will overly penalise our own slabs under mempressure. Given that we
now know the target number of objects to scan, use that as our guide for
deciding to shrink as opposed to the number of objects we manage to
shrink (which doesn't correspond to the numbers we report to shrinkctl).

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_gem_shrinker.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_shrinker.c b/drivers/gpu/drm/i915/i915_gem_shrinker.c
index 9029ed04879c..0e158f9287c4 100644
--- a/drivers/gpu/drm/i915/i915_gem_shrinker.c
+++ b/drivers/gpu/drm/i915/i915_gem_shrinker.c
@@ -363,13 +363,13 @@ i915_gem_shrinker_scan(struct shrinker *shrinker, struct shrink_control *sc)
 				I915_SHRINK_BOUND |
 				I915_SHRINK_UNBOUND |
 				I915_SHRINK_PURGEABLE);
-	if (freed < sc->nr_to_scan)
+	if (sc->nr_scanned < sc->nr_to_scan)
 		freed += i915_gem_shrink(i915,
 					 sc->nr_to_scan - sc->nr_scanned,
 					 &sc->nr_scanned,
 					 I915_SHRINK_BOUND |
 					 I915_SHRINK_UNBOUND);
-	if (freed < sc->nr_to_scan && current_is_kswapd()) {
+	if (sc->nr_scanned < sc->nr_to_scan && current_is_kswapd()) {
 		intel_runtime_pm_get(i915);
 		freed += i915_gem_shrink(i915,
 					 sc->nr_to_scan - sc->nr_scanned,
-- 
2.15.1

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

✓ Fi.CI.BAT: success for series starting with [1/2] drm/i915: Use our singlethreaded wq for freeing objects

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/i915: Use our singlethreaded wq for freeing objects
URL   : https://patchwork.freedesktop.org/series/36480/
State : success

== Summary ==

Series 36480v1 series starting with [1/2] drm/i915: Use our singlethreaded wq for freeing objects
https://patchwork.freedesktop.org/api/1.0/series/36480/revisions/1/mbox/

Test debugfs_test:
        Subgroup read_all_entries:
                fail       -> DMESG-WARN (fi-elk-e7500) fdo#103989
Test gem_mmap_gtt:
        Subgroup basic-small-bo-tiledx:
                pass       -> FAIL       (fi-gdg-551) fdo#102575

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

fi-bdw-5557u     total:288  pass:267  dwarn:0   dfail:0   fail:0   skip:21  time:418s
fi-bdw-gvtdvm    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:426s
fi-blb-e6850     total:288  pass:223  dwarn:1   dfail:0   fail:0   skip:64  time:372s
fi-bsw-n3050     total:288  pass:242  dwarn:0   dfail:0   fail:0   skip:46  time:482s
fi-bwr-2160      total:288  pass:183  dwarn:0   dfail:0   fail:0   skip:105 time:283s
fi-bxt-dsi       total:288  pass:258  dwarn:0   dfail:0   fail:0   skip:30  time:484s
fi-bxt-j4205     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:486s
fi-byt-j1900     total:288  pass:253  dwarn:0   dfail:0   fail:0   skip:35  time:464s
fi-byt-n2820     total:288  pass:249  dwarn:0   dfail:0   fail:0   skip:39  time:452s
fi-elk-e7500     total:224  pass:168  dwarn:10  dfail:0   fail:0   skip:45 
fi-gdg-551       total:288  pass:179  dwarn:0   dfail:0   fail:1   skip:108 time:278s
fi-glk-1         total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:511s
fi-hsw-4770      total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:390s
fi-hsw-4770r     total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:400s
fi-ivb-3520m     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:459s
fi-ivb-3770      total:288  pass:255  dwarn:0   dfail:0   fail:0   skip:33  time:410s
fi-kbl-7560u     total:288  pass:269  dwarn:0   dfail:0   fail:0   skip:19  time:499s
fi-kbl-7567u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:454s
fi-kbl-r         total:288  pass:260  dwarn:1   dfail:0   fail:0   skip:27  time:502s
fi-pnv-d510      total:288  pass:222  dwarn:1   dfail:0   fail:0   skip:65  time:583s
fi-skl-6260u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:428s
fi-skl-6600u     total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:505s
fi-skl-6700hq    total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:526s
fi-skl-6700k2    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:493s
fi-skl-6770hq    total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:495s
fi-snb-2520m     total:288  pass:248  dwarn:0   dfail:0   fail:0   skip:40  time:528s
fi-snb-2600      total:288  pass:248  dwarn:0   dfail:0   fail:0   skip:40  time:397s
Blacklisted hosts:
fi-cfl-s2        total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:563s
fi-glk-dsi       total:288  pass:258  dwarn:0   dfail:0   fail:0   skip:30  time:466s

cc2f290ac3bc91cd0488de5f0d4c0937bbd498d0 drm-tip: 2018y-01m-15d-12h-58m-19s UTC integration manifest
2ff60763c6fd drm/i915: Only attempt to scan the requested number of shrinker slabs
b87733ff529e drm/i915: Use our singlethreaded wq for freeing objects

== Logs ==

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

Re: [PATCH 1/2] drm/i915: Use our singlethreaded wq for freeing objects

[Tvrtko Ursulin]

On 15/01/2018 12:28, Chris Wilson wrote:
> As freeing the objects require serialisation on struct_mutex, we should
> prefer to use our singlethreaded driver wq that is dedicated to work
> requiring struct_mutex (hence serialised).The benefit should be less
> clutter on the system wq, allowing it to make progress even when the
> driver/struct_mutex is heavily contended.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>   drivers/gpu/drm/i915/i915_gem.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 1135a77b383a..87937c4f9dff 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -4732,7 +4732,7 @@ static void __i915_gem_free_object_rcu(struct rcu_head *head)
>   	 * detour through a worker.
>   	 */

This comment which is only partially visible here is a bit wonky...

>   	if (llist_add(&obj->freed, &i915->mm.free_list))
> -		schedule_work(&i915->mm.free_work);
> +		queue_work(i915->wq, &i915->mm.free_work);
>   }
>   
>   void i915_gem_free_object(struct drm_gem_object *gem_obj)
> 

.. but the logic seems sound.

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

In general it is a bit funky to call_rcu to schedule worker - what would 
be the difference to just queueing the worker which would have 
synchronize rcu in it?

Or would it be feasible to do multi-pass - 1st pass directly from 
call_rcu does the ones which can be done wo/ struct mutex, leaves the 
rest on the list and queues more thorough 2nd pass. Haven't really 
investigated it, just throwing ideas around.

Regards,

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

✓ Fi.CI.IGT: success for series starting with [1/2] drm/i915: Use our singlethreaded wq for freeing objects

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/i915: Use our singlethreaded wq for freeing objects
URL   : https://patchwork.freedesktop.org/series/36480/
State : success

== Summary ==

Test kms_flip:
        Subgroup vblank-vs-suspend:
                skip       -> PASS       (shard-snb) fdo#102365
        Subgroup plain-flip-ts-check-interruptible:
                pass       -> FAIL       (shard-hsw) fdo#100368
Test kms_frontbuffer_tracking:
        Subgroup fbc-1p-offscren-pri-shrfb-draw-render:
                fail       -> PASS       (shard-snb) fdo#101623 +1

fdo#102365 https://bugs.freedesktop.org/show_bug.cgi?id=102365
fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
fdo#101623 https://bugs.freedesktop.org/show_bug.cgi?id=101623

shard-hsw        total:2723 pass:1545 dwarn:1   dfail:0   fail:11  skip:1166 time:9128s
shard-snb        total:2723 pass:1315 dwarn:1   dfail:0   fail:12  skip:1395 time:7941s
Blacklisted hosts:
shard-apl        total:2723 pass:1697 dwarn:1   dfail:0   fail:22  skip:1003 time:13566s
shard-kbl        total:2713 pass:1815 dwarn:3   dfail:0   fail:24  skip:870 time:10311s

== Logs ==

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

Re: [PATCH 1/2] drm/i915: Use our singlethreaded wq for freeing objects

[Chris Wilson]

Quoting Tvrtko Ursulin (2018-01-15 16:54:57)
>
> In general it is a bit funky to call_rcu to schedule worker - what would
> be the difference to just queueing the worker which would have
> synchronize rcu in it?

The complication came from introducing the direct cleanup path to
penalise frequent reallocations, and to avoid deferring unbounded work
to the rcu task (which shows up in our microbenchmarks as a significant
drawback).

> Or would it be feasible to do multi-pass - 1st pass directly from
> call_rcu does the ones which can be done wo/ struct mutex, leaves the
> rest on the list and queues more thorough 2nd pass. Haven't really
> investigated it, just throwing ideas around.

We need to take the mutex first in the release pass (to release the GPU
binding before we return the resources back to the system). There is a
nicety in doing most of the operations in the worker context rather than
in process (deferring the work to later, with some penalty applied to
frequent reallocation) or rcu context.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915: Use our singlethreaded wq for freeing objects

[Chris Wilson]

Quoting Tvrtko Ursulin (2018-01-15 16:54:57)
> 
> On 15/01/2018 12:28, Chris Wilson wrote:
> > As freeing the objects require serialisation on struct_mutex, we should
> > prefer to use our singlethreaded driver wq that is dedicated to work
> > requiring struct_mutex (hence serialised).The benefit should be less
> > clutter on the system wq, allowing it to make progress even when the
> > driver/struct_mutex is heavily contended.
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> >   drivers/gpu/drm/i915/i915_gem.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index 1135a77b383a..87937c4f9dff 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -4732,7 +4732,7 @@ static void __i915_gem_free_object_rcu(struct rcu_head *head)
> >        * detour through a worker.
> >        */
> 
> This comment which is only partially visible here is a bit wonky...
> 
> >       if (llist_add(&obj->freed, &i915->mm.free_list))
> > -             schedule_work(&i915->mm.free_work);
> > +             queue_work(i915->wq, &i915->mm.free_work);
> >   }
> >   
> >   void i915_gem_free_object(struct drm_gem_object *gem_obj)
> > 
> 
> .. but the logic seems sound.
> 
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Thanks for the review and questions. Sent a patch to hopefully improve
the commentary here, and pushed this simple patch in the meantime.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx