[PATCH] drm/i915: Prefer to reschedule the free_object worker rather than block

[PATCH] drm/i915: Prefer to reschedule the free_object worker rather than block

[Chris Wilson]

Avoid blocking the kworker by putting back the freed object list if we
cannot immediately take the mutex. We will try again shortly, and flush
the work when desperate.

References: https://bugs.freedesktop.org/show_bug.cgi?id=100434
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index ab77e38ec264..c2e5cb529b0f 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4200,7 +4200,19 @@ static void __i915_gem_free_objects(struct drm_i915_private *i915,
 {
 	struct drm_i915_gem_object *obj, *on;
 
-	mutex_lock(&i915->drm.struct_mutex);
+	if (!mutex_trylock(&i915->drm.struct_mutex)) {
+		/* If we fail to acquire the struct_mutex, put back the
+		 * freed list and we will try again in the future. By
+		 * rescheduling the task we prevent us from blocking
+		 * the worker indefinitely on a prolonged wait for
+		 * struct_mutex.
+		 */
+		if (llist_add_batch(llist_reverse_order(freed), freed,
+				    &i915->mm.free_list))
+			schedule_work(&i915->mm.free_work);
+		return;
+	}
+
 	intel_runtime_pm_get(i915);
 	llist_for_each_entry(obj, freed, freed) {
 		struct i915_vma *vma, *vn;
-- 
2.11.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: Prefer to reschedule the free_object worker rather than block

[Patchwork]

== Series Details ==

Series: drm/i915: Prefer to reschedule the free_object worker rather than block
URL   : https://patchwork.freedesktop.org/series/22020/
State : failure

== Summary ==

Series 22020v1 drm/i915: Prefer to reschedule the free_object worker rather than block
https://patchwork.freedesktop.org/api/1.0/series/22020/revisions/1/mbox/

Test gem_exec_flush:
        Subgroup basic-batch-kernel-default-uc:
                pass       -> FAIL       (fi-snb-2600) fdo#100007
Test gem_exec_parallel:
        Subgroup basic:
                pass       -> INCOMPLETE (fi-bsw-n3050)
Test gem_exec_suspend:
        Subgroup basic-s3:
                incomplete -> PASS       (fi-skl-6260u)
        Subgroup basic-s4-devices:
                dmesg-warn -> PASS       (fi-kbl-7560u) fdo#100428

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

fi-bdw-5557u     total:278  pass:267  dwarn:0   dfail:0   fail:0   skip:11  time: 457s
fi-bdw-gvtdvm    total:278  pass:256  dwarn:8   dfail:0   fail:0   skip:14  time: 467s
fi-bsw-n3050     total:68   pass:59   dwarn:0   dfail:0   fail:0   skip:8   time: 0s
fi-bxt-j4205     total:278  pass:259  dwarn:0   dfail:0   fail:0   skip:19  time: 537s
fi-bxt-t5700     total:278  pass:258  dwarn:0   dfail:0   fail:0   skip:20  time: 577s
fi-byt-j1900     total:278  pass:251  dwarn:0   dfail:0   fail:0   skip:27  time: 515s
fi-byt-n2820     total:278  pass:247  dwarn:0   dfail:0   fail:0   skip:31  time: 507s
fi-hsw-4770      total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time: 440s
fi-hsw-4770r     total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time: 433s
fi-ilk-650       total:278  pass:228  dwarn:0   dfail:0   fail:0   skip:50  time: 441s
fi-ivb-3520m     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time: 522s
fi-ivb-3770      total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time: 489s
fi-kbl-7500u     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time: 484s
fi-kbl-7560u     total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time: 589s
fi-skl-6260u     total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time: 482s
fi-skl-6700hq    total:278  pass:261  dwarn:0   dfail:0   fail:0   skip:17  time: 602s
fi-skl-6700k     total:278  pass:256  dwarn:4   dfail:0   fail:0   skip:18  time: 490s
fi-skl-6770hq    total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time: 523s
fi-skl-gvtdvm    total:278  pass:265  dwarn:0   dfail:0   fail:0   skip:13  time: 471s
fi-snb-2520m     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time: 551s
fi-snb-2600      total:278  pass:248  dwarn:0   dfail:0   fail:1   skip:29  time: 421s

dabd992961047cf26698036f563aa86a083284ac drm-tip: 2017y-03m-28d-15h-25m-54s UTC integration manifest
db93ee5 drm/i915: Prefer to reschedule the free_object worker rather than block

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4327/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Prefer to reschedule the free_object worker rather than block

[Mika Kuoppala]

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

> Avoid blocking the kworker by putting back the freed object list if we
> cannot immediately take the mutex. We will try again shortly, and flush
> the work when desperate.
>
> References: https://bugs.freedesktop.org/show_bug.cgi?id=100434
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/i915_gem.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index ab77e38ec264..c2e5cb529b0f 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -4200,7 +4200,19 @@ static void __i915_gem_free_objects(struct drm_i915_private *i915,
>  {
>  	struct drm_i915_gem_object *obj, *on;
>  
> -	mutex_lock(&i915->drm.struct_mutex);
> +	if (!mutex_trylock(&i915->drm.struct_mutex)) {
> +		/* If we fail to acquire the struct_mutex, put back the
> +		 * freed list and we will try again in the future. By
> +		 * rescheduling the task we prevent us from blocking
> +		 * the worker indefinitely on a prolonged wait for
> +		 * struct_mutex.

I don't understand the last part of the comment. If we don't
want a prolonged block due to mutex, should we limit the amount
of work we do here, inside the mutex. By limiting how much we
free per aquiring the lock?

-Mika

> +		 */
> +		if (llist_add_batch(llist_reverse_order(freed), freed,
> +				    &i915->mm.free_list))
> +			schedule_work(&i915->mm.free_work);
> +		return;
> +	}
> +
>  	intel_runtime_pm_get(i915);
>  	llist_for_each_entry(obj, freed, freed) {
>  		struct i915_vma *vma, *vn;
> -- 
> 2.11.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Prefer to reschedule the free_object worker rather than block

[Chris Wilson]

On Wed, Mar 29, 2017 at 11:31:28AM +0300, Mika Kuoppala wrote:
> Chris Wilson <chris@chris-wilson.co.uk> writes:
> 
> > Avoid blocking the kworker by putting back the freed object list if we
> > cannot immediately take the mutex. We will try again shortly, and flush
> > the work when desperate.
> >
> > References: https://bugs.freedesktop.org/show_bug.cgi?id=100434
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> >  drivers/gpu/drm/i915/i915_gem.c | 14 +++++++++++++-
> >  1 file changed, 13 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index ab77e38ec264..c2e5cb529b0f 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -4200,7 +4200,19 @@ static void __i915_gem_free_objects(struct drm_i915_private *i915,
> >  {
> >  	struct drm_i915_gem_object *obj, *on;
> >  
> > -	mutex_lock(&i915->drm.struct_mutex);
> > +	if (!mutex_trylock(&i915->drm.struct_mutex)) {
> > +		/* If we fail to acquire the struct_mutex, put back the
> > +		 * freed list and we will try again in the future. By
> > +		 * rescheduling the task we prevent us from blocking
> > +		 * the worker indefinitely on a prolonged wait for
> > +		 * struct_mutex.
> 
> I don't understand the last part of the comment. If we don't
> want a prolonged block due to mutex, should we limit the amount
> of work we do here, inside the mutex. By limiting how much we
> free per aquiring the lock?

Possibly, but if we spend 20s inside here we get NMI lockup warnings
from this function. On the other hand, we regulary bug out and hold the
struct_mutex indefinitely -- if we wait on the stuck mutex here, we tie
up the kworker.

The worst part here is that we requeue the work for immediate execute.
If the worker is otherwise idle, it will spin. That's not great. (It
works for our other workers as they are on a delay, the delay here is
that we wait for an rcu tick before queuing the work in the first place.)

I like the idea of letting the kworker get on with something else
instead of blocking, but not yet happy.
-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