Re: [Intel-gfx] [PATCH 06/16] drm/gem/shrinker: Wait before acquiring struct_mutex under oom

[Chris Wilson]

On Thu, Aug 04, 2016 at 09:46:59AM +0300, Joonas Lahtinen wrote:
> On ma, 2016-08-01 at 19:22 +0100, Chris Wilson wrote:
> > We can now wait for the GPU (all engines) to become idle without
> > requiring the struct_mutex. Inside the shrinker, we need to currently
> > take the struct_mutex in order to purge objects and to purge the objects
> > we need the GPU to be idle - causing a stall whilst we hold the
> > struct_mutex. We can hide most of that stall by performing the wait
> > before taking the struct_mutex and only doing essential waits for
> > new rendering on objects to be freed.
> > 
> > Signed-off-by: Chris Wilson 
> > ---
> >  drivers/gpu/drm/i915/i915_gem_shrinker.c | 5 +
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem_shrinker.c 
> > b/drivers/gpu/drm/i915/i915_gem_shrinker.c
> > index 1341cb55b6f1..43e53e419982 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_shrinker.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_shrinker.c
> > @@ -326,9 +326,14 @@ i915_gem_shrinker_lock_uninterruptible(struct 
> > drm_i915_private *dev_priv,
> >     unsigned long timeout = msecs_to_jiffies(timeout_ms) + 1;
> >  
> >     while (!i915_gem_shrinker_lock(_priv->drm, >unlock)) {
> > +   if (i915_gem_wait_for_idle(dev_priv) == 0 &&
> 
> continue would be much cleaner here, to avoid repeating the lock
> calling line?

Yes. This was a small change, but adjusting the loop wasn't that much
bigger.

> Or how likely is it for engines to be idle but
> struct_mutex held for extended period?

Currently, quite possible. struct_mutex is the defacto bkl, we may be
causing memory pressure by swapping in gigabytes of an object on another
thread.
-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: [Intel-gfx] [PATCH 06/16] drm/gem/shrinker: Wait before acquiring struct_mutex under oom

[Joonas Lahtinen]

On ma, 2016-08-01 at 19:22 +0100, Chris Wilson wrote:
> We can now wait for the GPU (all engines) to become idle without
> requiring the struct_mutex. Inside the shrinker, we need to currently
> take the struct_mutex in order to purge objects and to purge the objects
> we need the GPU to be idle - causing a stall whilst we hold the
> struct_mutex. We can hide most of that stall by performing the wait
> before taking the struct_mutex and only doing essential waits for
> new rendering on objects to be freed.
> 
> Signed-off-by: Chris Wilson 
> ---
>  drivers/gpu/drm/i915/i915_gem_shrinker.c | 5 +
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_shrinker.c 
> b/drivers/gpu/drm/i915/i915_gem_shrinker.c
> index 1341cb55b6f1..43e53e419982 100644
> --- a/drivers/gpu/drm/i915/i915_gem_shrinker.c
> +++ b/drivers/gpu/drm/i915/i915_gem_shrinker.c
> @@ -326,9 +326,14 @@ i915_gem_shrinker_lock_uninterruptible(struct 
> drm_i915_private *dev_priv,
>   unsigned long timeout = msecs_to_jiffies(timeout_ms) + 1;
>  
>   while (!i915_gem_shrinker_lock(_priv->drm, >unlock)) {
> + if (i915_gem_wait_for_idle(dev_priv) == 0 &&

continue would be much cleaner here, to avoid repeating the lock
calling line? Or how likely is it for engines to be idle but
struct_mutex held for extended period?

Regards, Joonas

> + i915_gem_shrinker_lock(_priv->drm, >unlock))
> + break;
> +
>   schedule_timeout_killable(1);
>   if (fatal_signal_pending(current))
>   return false;
> +
>   if (--timeout == 0) {
>   pr_err("Unable to lock GPU to purge memory.\n");
>   return false;
-- 
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

[Intel-gfx] [PATCH 06/16] drm/gem/shrinker: Wait before acquiring struct_mutex under oom

[Chris Wilson]

We can now wait for the GPU (all engines) to become idle without
requiring the struct_mutex. Inside the shrinker, we need to currently
take the struct_mutex in order to purge objects and to purge the objects
we need the GPU to be idle - causing a stall whilst we hold the
struct_mutex. We can hide most of that stall by performing the wait
before taking the struct_mutex and only doing essential waits for
new rendering on objects to be freed.

Signed-off-by: Chris Wilson 
---
 drivers/gpu/drm/i915/i915_gem_shrinker.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_gem_shrinker.c 
b/drivers/gpu/drm/i915/i915_gem_shrinker.c
index 1341cb55b6f1..43e53e419982 100644
--- a/drivers/gpu/drm/i915/i915_gem_shrinker.c
+++ b/drivers/gpu/drm/i915/i915_gem_shrinker.c
@@ -326,9 +326,14 @@ i915_gem_shrinker_lock_uninterruptible(struct 
drm_i915_private *dev_priv,
unsigned long timeout = msecs_to_jiffies(timeout_ms) + 1;
 
while (!i915_gem_shrinker_lock(_priv->drm, >unlock)) {
+   if (i915_gem_wait_for_idle(dev_priv) == 0 &&
+   i915_gem_shrinker_lock(_priv->drm, >unlock))
+   break;
+
schedule_timeout_killable(1);
if (fatal_signal_pending(current))
return false;
+
if (--timeout == 0) {
pr_err("Unable to lock GPU to purge memory.\n");
return false;
-- 
2.8.1

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